> From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxx>
> > Signed-off-by: Wentong Wu <wentong.wu@xxxxxxxxx>
> > Tested-by: Jason Chen <jason.z.chen@xxxxxxxxx>
> > ---
> > 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
