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

[Sanjay Chitroda <sanjayembeddedse@gmail.com>]

On 6 April 2026 9:39:08 pm IST, Hans de Goede <hansg@kernel.org> wrote:
>Hi Sanjay,
>
>On 5-Apr-26 12:16, Sanjay Chitroda wrote:
>> 
>> 
>> On 2 April 2026 12:36:33 am IST, Hans de Goede <hansg@kernel.org> wrote:
>>> Hi,
>>>
>>> On 1-Apr-26 20:16, Sanjay Chitroda wrote:
>>>> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
>>>>
>>>> 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
>

Hi Hans,

Thanks for the input and clarification l.

Understood. Since "gc0310_remove()" is always safe and simple to call will keep it.

As suggested, patch 3/3 is already in good shape, so I won't send a v2 series.
Please consider v1 of patch 3/3 for merging.

Regards,
Sanjay Chitroda

>
>
>
>> 
>>>
>