Re: [PATCH 2/3] media: ivsc: csi: add separate lock for v4l2 control handler

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Wentong,

Thanks for the patchset.

On Mon, Jun 03, 2024 at 04:26:13PM +0800, Wentong Wu wrote:
> There're possibilities that privacy status change notification happens
> in the middle of the ongoing mei command which already takes the command
> lock, but v4l2_ctrl_s_ctrl() would also need the same lock prior to this
> patch, so this may results in circular locking problem. This patch adds
> one dedicated lock for v4l2 control handler to avoid described issue.

Before this patch, wouldn't the ongoing MEI command simply complete before
v4l2_ctrl_s_ctrl() could proceed?

> 
> Reported-by: Hao Yao <hao.yao@intel.com>
> Signed-off-by: Wentong Wu <wentong.wu@intel.com>
> Tested-by: Jason Chen <jason.z.chen@intel.com>
> ---
>  drivers/media/pci/intel/ivsc/mei_csi.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/pci/intel/ivsc/mei_csi.c b/drivers/media/pci/intel/ivsc/mei_csi.c
> index 004ebab0b814..d6ba0d9efca1 100644
> --- a/drivers/media/pci/intel/ivsc/mei_csi.c
> +++ b/drivers/media/pci/intel/ivsc/mei_csi.c
> @@ -126,6 +126,8 @@ struct mei_csi {
>  	struct v4l2_ctrl_handler ctrl_handler;
>  	struct v4l2_ctrl *freq_ctrl;
>  	struct v4l2_ctrl *privacy_ctrl;
> +	/* lock for v4l2 controls */
> +	struct mutex ctrl_lock;
>  	unsigned int remote_pad;
>  	/* start streaming or not */
>  	int streaming;
> @@ -563,11 +565,13 @@ static int mei_csi_init_controls(struct mei_csi *csi)
>  	u32 max;
>  	int ret;
>  
> +	mutex_init(&csi->ctrl_lock);
> +
>  	ret = v4l2_ctrl_handler_init(&csi->ctrl_handler, 2);
>  	if (ret)
>  		return ret;
>  
> -	csi->ctrl_handler.lock = &csi->lock;
> +	csi->ctrl_handler.lock = &csi->ctrl_lock;
>  
>  	max = ARRAY_SIZE(link_freq_menu_items) - 1;
>  	csi->freq_ctrl = v4l2_ctrl_new_int_menu(&csi->ctrl_handler,
> @@ -756,6 +760,7 @@ static int mei_csi_probe(struct mei_cl_device *cldev,
>  
>  err_ctrl_handler:
>  	v4l2_ctrl_handler_free(&csi->ctrl_handler);
> +	mutex_destroy(&csi->ctrl_lock);
>  	v4l2_async_nf_unregister(&csi->notifier);
>  	v4l2_async_nf_cleanup(&csi->notifier);
>  
> @@ -775,6 +780,7 @@ static void mei_csi_remove(struct mei_cl_device *cldev)
>  	v4l2_async_nf_unregister(&csi->notifier);
>  	v4l2_async_nf_cleanup(&csi->notifier);
>  	v4l2_ctrl_handler_free(&csi->ctrl_handler);
> +	mutex_destroy(&csi->ctrl_lock);
>  	v4l2_async_unregister_subdev(&csi->subdev);
>  	v4l2_subdev_cleanup(&csi->subdev);
>  	media_entity_cleanup(&csi->subdev.entity);

-- 
Kind regards,

Sakari Ailus