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