diff mbox series

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

Message ID 20240603082614.1567712-3-wentong.wu@intel.com (mailing list archive)
State New
Headers show
Series Fix privacy issue for MEI CSI | expand

Commit Message

Wu, Wentong June 3, 2024, 8:26 a.m. UTC 
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.

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(-)

Comments

Sakari Ailus June 7, 2024, 12:41 p.m. UTC | #1 
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);
Wu, Wentong June 7, 2024, 12:53 p.m. UTC | #2 
> From: 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?

it can, but every function calling mei_csi_send() would check the return
value and call v4l2_ctrl_s_ctrl(), probably the code would be duplicated.

BR,
Wentong
> 
> >
> > 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
diff mbox series

Patch

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);