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

[Eugen Hristev]

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@xxxxxxxxxxxxxxx
> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@xxxxxxxxxxxxx>
> ---
>  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 ?

>  
>  	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 [...] ?

>  }
>  
>