Re: [PATCH v3 01/10] media: microchip-isc: fix awb_mutex and lock lifecycle

[<Balakrishnan.S@microchip.com>]

Hi Eugen,

On 21/07/26 6:57 pm, Eugen Hristev wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 7/21/26 13:28, Balakrishnan Sambath wrote:
>> isc_async_complete() initialised awb_mutex and isc->lock only after an
>> early error return, and the teardown was inconsistent:
>>
>>   - isc_async_unbind() destroyed awb_mutex before cancelling awb_work,
>>     which takes it;
>>   - a failed .complete() destroyed both locks, then the v4l2-async core
>>     unbinds the subdev and isc_async_unbind() destroyed awb_mutex again;
>>   - isc->lock was destroyed only on the .complete() error path, so the
>>     normal unbind path leaked it.
>>
>> Initialise both locks before the first error return, make unbind the
>> single teardown site (cancel the work, then destroy both locks) and
>> drop the destroys from the .complete() error path.
>>
>> Fixes: 314c96e5203d ("media: atmel: atmel-isc-base: use mutex to lock awb workq from streaming")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
>> ---
>>   drivers/media/platform/microchip/microchip-isc-base.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
>> index a7cdc743fda7..45a7af779323 100644
>> --- a/drivers/media/platform/microchip/microchip-isc-base.c
>> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
>> @@ -1703,10 +1703,11 @@ static void isc_async_unbind(struct v4l2_async_notifier *notifier,
>>   {
>>        struct isc_device *isc = container_of(notifier->v4l2_dev,
>>                                              struct isc_device, v4l2_dev);
>> -     mutex_destroy(&isc->awb_mutex);
>>        cancel_work_sync(&isc->awb_work);
>> +     mutex_destroy(&isc->awb_mutex);
>>        video_unregister_device(&isc->video_dev);
>>        v4l2_ctrl_handler_free(&isc->ctrls.handler);
>> +     mutex_destroy(&isc->lock);
>>   }
>>
>>   struct isc_format *isc_find_format_by_code(struct isc_device *isc,
>> @@ -1758,6 +1759,8 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
>>        int ret = 0;
>>
>>        INIT_WORK(&isc->awb_work, isc_awb_work);
>> +     mutex_init(&isc->lock);
>> +     mutex_init(&isc->awb_mutex);
> 
> Does it make sense to reinit these mutexes again every time complete()
> is called , and not get destroyed here in case of an err ?
> 
> The way I understand it, is isc_async_complete() will init them, then
> some other function would destroy them, but that destroy is called in
> every single case ?
> It is odd that a failing function in this case would leave mutexes
> initialized and not destroyed, when the function itself would initialize
> them. It would not leave the device in the same state , after failing .
> Which is quite odd.
> I am thinking whether it's better to allocate these mutexes at probe and
> have them all the time, and teardown on remove(). Is that making more
> sense ?

Fair point. I had put the init in isc_async_complete() just because the
rest of the setup is there, wasn't really thinking of the fail path. 
Thanks for the valuable pointer, Eugen. Will address this in next version.

>>
>>        ret = v4l2_device_register_subdev_nodes(&isc->v4l2_dev);
>>        if (ret < 0) {
>> @@ -1767,8 +1770,6 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
>>
>>        isc->current_subdev = container_of(notifier,
>>                                           struct isc_subdev_entity, notifier);
>> -     mutex_init(&isc->lock);
>> -     mutex_init(&isc->awb_mutex);
>>
>>        init_completion(&isc->comp);
>>
>> @@ -1841,8 +1842,6 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
>>        video_unregister_device(vdev);
>>
>>   isc_async_complete_err:
>> -     mutex_destroy(&isc->awb_mutex);
>> -     mutex_destroy(&isc->lock);
>>        return ret;
> 
> If now isc_async_complete_err just returns ret, does it still make sense
> to have this label ? Or just return ret instead of goto [...] ?

Ah, indeed that's a no-op somehow I missed it. I'll drop it too.

Thanks,
Balakrishnan S
> 
>>   }
>>
>>
>