Re: [PATCH 1/3] media: i2c: gc0310: fix probe error handling and unwind resources properly

[Hans de Goede]

Hi Sanjay,

On 5-Apr-26 12:16, Sanjay Chitroda wrote:
> 
> 
> On 2 April 2026 12:36:33 am IST, Hans de Goede <hansg@xxxxxxxxxx> wrote:
>> Hi,
>>
>> On 1-Apr-26 20:16, Sanjay Chitroda wrote:
>>> From: Sanjay Chitroda <sanjayembeddedse@xxxxxxxxx>
>>>
>>> The GC0310 probe path currently performs error cleanup by jumping to a
>>> common label that mirrors the driver's remove() callback. This is unsafe,
>>> as remove() assumes that the subdevice has been fully registered with
>>> the V4L2 framework, media and control resources have been initialized.
>>
>> That is simply not true, all functions called in remove() internally
>> check if their init counter-part has succeeded and if not are a no-op.
>>
>> If you're aware of any specific calls in remove() where this is not
>> the case, please explicitly describe these cases and describe an
>> example exit-error path from probe() where things actually go wrong.
>>
> 
> Hi Hans,
> 
> Thanks for the clarification - agreed, the remove helpers are defensively implemented and the existing code is not incorrect for a functional point. I should not have stated gc0310_remov() from a probe failure is unsafe.
> 
>>> Calling remove() from probe can result in unregistering or cleaning up
>>> subdevice, leading to resource leaks and subtle lifecycle bugs.
>>>
>>> Rewrite the probe() error handling to unwind resources explicitly, using
>>> fine‑grained goto labels along with appropriate error logs. Each failure
>>> path now frees only successfully acquired resource, without remove().
>>>
>>> This aligns the driver with standard V4L2 sensor lifecycle expectations
>>> and avoids incorrect teardown during probe failures.
>>
>> The rest of this reads very much like this was AI generated.
>>
>> Did you use AI to generate these patches ? If so please:
>>
>> Make sure you actually understand what the patch is doing and
>> very yourself that it actually is correct, which in this case
>> I believe it is not.
>>
>> Regards,
>>
>> Hans
>>
> 
> Yes, I did use AI assistance to help draft the commit message, but the patch logic itself was written and reviewed by me. However, your feedback makes it clear that i did not sufficiently validate internals of existing remove() based cleanup.
> 
> 
> I would like to propose commit message that align with change and existing kernel internals:
> 
> -------
> media: i2c: gc0310: make probe error unwinding explicit
> 
> The gc0310 probe path unwinds failures by jumping to a single label remove-style cleanup.
> 
> Refactor the probe error handling so that resources are unwound explicitly and in reverse order of initialization using fine-grained goto labels.
> 
> This improves clarity and maintains symmetry with the probe initialization path.
> 
> No functional change intended.
> -------
> 
> Kindly share your input on the same, according I will plan to resend v2 with an updated commit message as above.

The problem is that your patch makes the code more complicated.

Since we know that gc0310_remove() is always safe to call
the simplest and cleanest code is to simply keep calling
gc0310_remove().

If you plan to do a v2 of this series please drop this patch.

Note I see no need for a v2. Patch 3/3 is good to have and
v1 of that can be merged. The other 2 patches should be dropped.

Regards,

Hans




> 
>>