[PATCH 0/3] Fix privacy issue for MEI CSI

[PATCH 0/3] Fix privacy issue for MEI CSI

[Wentong Wu]

Currently if starting stream goes with privacy mode on, the whole
pipeline will fail because MEI CSI counts privacy mode on as error.
In face, the ongoing MEI command is well executed by firmware as
well even if privacy mode is turned on. The status (-1) of command
response is just to indicate current privacy on status instead of
failed to execute command by firmware. This patch set changes the
behavior to notify privacy mode on via v4l2 privacy control instead
of fail the ongoing MEI command.

This patch set adds one dedicated lock for v4l2 control handler to
avoid dead locking issue as well. And the useless privacy status in
struct mei_csi is removed to make code clean.

Wentong Wu (3):
  media: ivsc: csi: don't count privacy on as error
  media: ivsc: csi: add separate lock for v4l2 control handler
  media: ivsc: csi: remove privacy status in struct mei_csi

 drivers/media/pci/intel/ivsc/mei_csi.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

-- 
2.34.1

[PATCH 1/3] media: ivsc: csi: don't count privacy on as error

[Wentong Wu]

Prior to the ongoing command privacy is on, it would return -1 to
indicate the current privacy status, and the ongoing command would
be well executed by firmware as well, so this is not error. This
patch changes its behavior to notify privacy on directly by V4L2
privacy control instead of reporting error.

Fixes: 29006e196a56 ("media: pci: intel: ivsc: Add CSI submodule")
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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/pci/intel/ivsc/mei_csi.c b/drivers/media/pci/intel/ivsc/mei_csi.c
index 89b582a221ab..004ebab0b814 100644
--- a/drivers/media/pci/intel/ivsc/mei_csi.c
+++ b/drivers/media/pci/intel/ivsc/mei_csi.c
@@ -190,7 +190,11 @@ static int mei_csi_send(struct mei_csi *csi, u8 *buf, size_t len)
 
 	/* command response status */
 	ret = csi->cmd_response.status;
-	if (ret) {
+	if (ret == -1) {
+		/* notify privacy on instead of reporting error */
+		ret = 0;
+		v4l2_ctrl_s_ctrl(csi->privacy_ctrl, CSI_PRIVACY_ON);
+	} else if (ret) {
 		ret = -EINVAL;
 		goto out;
 	}
-- 
2.34.1

Re: [PATCH 1/3] media: ivsc: csi: don't count privacy on as error

[Sakari Ailus]

Hi Wentong,

On Mon, Jun 03, 2024 at 04:26:12PM +0800, Wentong Wu wrote:
> Prior to the ongoing command privacy is on, it would return -1 to
> indicate the current privacy status, and the ongoing command would
> be well executed by firmware as well, so this is not error. This
> patch changes its behavior to notify privacy on directly by V4L2
> privacy control instead of reporting error.
> 
> Fixes: 29006e196a56 ("media: pci: intel: ivsc: Add CSI submodule")
> 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 | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/pci/intel/ivsc/mei_csi.c b/drivers/media/pci/intel/ivsc/mei_csi.c
> index 89b582a221ab..004ebab0b814 100644
> --- a/drivers/media/pci/intel/ivsc/mei_csi.c
> +++ b/drivers/media/pci/intel/ivsc/mei_csi.c
> @@ -190,7 +190,11 @@ static int mei_csi_send(struct mei_csi *csi, u8 *buf, size_t len)
>  
>  	/* command response status */
>  	ret = csi->cmd_response.status;
> -	if (ret) {
> +	if (ret == -1) {
> +		/* notify privacy on instead of reporting error */
> +		ret = 0;
> +		v4l2_ctrl_s_ctrl(csi->privacy_ctrl, CSI_PRIVACY_ON);

s/CSI_PRIVACY_ON/1/

> +	} else if (ret) {
>  		ret = -EINVAL;
>  		goto out;
>  	}

-- 
Kind regards,

Sakari Ailus

RE: [PATCH 1/3] media: ivsc: csi: don't count privacy on as error

[Wu, Wentong]

> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Hi Wentong,
> 
> On Mon, Jun 03, 2024 at 04:26:12PM +0800, Wentong Wu wrote:
> > Prior to the ongoing command privacy is on, it would return -1 to
> > indicate the current privacy status, and the ongoing command would be
> > well executed by firmware as well, so this is not error. This patch
> > changes its behavior to notify privacy on directly by V4L2 privacy
> > control instead of reporting error.
> >
> > Fixes: 29006e196a56 ("media: pci: intel: ivsc: Add CSI submodule")
> > 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 | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/pci/intel/ivsc/mei_csi.c
> > b/drivers/media/pci/intel/ivsc/mei_csi.c
> > index 89b582a221ab..004ebab0b814 100644
> > --- a/drivers/media/pci/intel/ivsc/mei_csi.c
> > +++ b/drivers/media/pci/intel/ivsc/mei_csi.c
> > @@ -190,7 +190,11 @@ static int mei_csi_send(struct mei_csi *csi, u8
> > *buf, size_t len)
> >
> >  	/* command response status */
> >  	ret = csi->cmd_response.status;
> > -	if (ret) {
> > +	if (ret == -1) {
> > +		/* notify privacy on instead of reporting error */
> > +		ret = 0;
> > +		v4l2_ctrl_s_ctrl(csi->privacy_ctrl, CSI_PRIVACY_ON);
> 
> s/CSI_PRIVACY_ON/1/

Ack, thanks

BR,
Wentong
> 
> > +	} else if (ret) {
> >  		ret = -EINVAL;
> >  		goto out;
> >  	}
> 
> --
> Kind regards,
> 
> Sakari Ailus

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

[Wentong Wu]

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

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

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

[Sakari Ailus]

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

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

[Wu, Wentong]

> 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

[PATCH 3/3] media: ivsc: csi: remove privacy status in struct mei_csi

[Wentong Wu]

The privacy status is maintained by privacy_ctrl, on which all
of the privacy status changes will go through, so there is no
point in maintaining one more element any more.

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 | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/media/pci/intel/ivsc/mei_csi.c b/drivers/media/pci/intel/ivsc/mei_csi.c
index d6ba0d9efca1..1d1b9181a50a 100644
--- a/drivers/media/pci/intel/ivsc/mei_csi.c
+++ b/drivers/media/pci/intel/ivsc/mei_csi.c
@@ -138,9 +138,6 @@ struct mei_csi {
 	u32 nr_of_lanes;
 	/* frequency of the CSI-2 link */
 	u64 link_freq;
-
-	/* privacy status */
-	enum ivsc_privacy_status status;
 };
 
 static const struct v4l2_mbus_framefmt mei_csi_format_mbus_default = {
@@ -271,10 +268,8 @@ static void mei_csi_rx(struct mei_cl_device *cldev)
 
 	switch (notif.cmd_id) {
 	case CSI_PRIVACY_NOTIF:
-		if (notif.cont.cont < CSI_PRIVACY_MAX) {
-			csi->status = notif.cont.cont;
-			v4l2_ctrl_s_ctrl(csi->privacy_ctrl, csi->status);
-		}
+		if (notif.cont.cont < CSI_PRIVACY_MAX)
+			v4l2_ctrl_s_ctrl(csi->privacy_ctrl, notif.cont.cont);
 		break;
 	case CSI_SET_OWNER:
 	case CSI_SET_CONF:
-- 
2.34.1

Re: [PATCH 3/3] media: ivsc: csi: remove privacy status in struct mei_csi

[Sakari Ailus]

Hi Wentong,

Thanks for the patch.

On Mon, Jun 03, 2024 at 04:26:14PM +0800, Wentong Wu wrote:
> The privacy status is maintained by privacy_ctrl, on which all
> of the privacy status changes will go through, so there is no
> point in maintaining one more element any more.
> 
> 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 | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ivsc/mei_csi.c b/drivers/media/pci/intel/ivsc/mei_csi.c
> index d6ba0d9efca1..1d1b9181a50a 100644
> --- a/drivers/media/pci/intel/ivsc/mei_csi.c
> +++ b/drivers/media/pci/intel/ivsc/mei_csi.c
> @@ -138,9 +138,6 @@ struct mei_csi {
>  	u32 nr_of_lanes;
>  	/* frequency of the CSI-2 link */
>  	u64 link_freq;
> -
> -	/* privacy status */
> -	enum ivsc_privacy_status status;
>  };
>  
>  static const struct v4l2_mbus_framefmt mei_csi_format_mbus_default = {
> @@ -271,10 +268,8 @@ static void mei_csi_rx(struct mei_cl_device *cldev)
>  
>  	switch (notif.cmd_id) {
>  	case CSI_PRIVACY_NOTIF:
> -		if (notif.cont.cont < CSI_PRIVACY_MAX) {
> -			csi->status = notif.cont.cont;
> -			v4l2_ctrl_s_ctrl(csi->privacy_ctrl, csi->status);
> -		}
> +		if (notif.cont.cont < CSI_PRIVACY_MAX)
> +			v4l2_ctrl_s_ctrl(csi->privacy_ctrl, notif.cont.cont);

notif.cont.cont represents is MEI's idea of the privacy state which just
happens to be aligned with V4L2's.

While this issue is not related to this patch, it'd be nice to use e.g.

			v4l2_ctrl_s_ctrl(csi->privacy_ctrl,
					 notif.cont.cont == CSI_PRIVACY_ON);

So could you add one more patch to the set for v2?

>  		break;
>  	case CSI_SET_OWNER:
>  	case CSI_SET_CONF:

-- 
Kind regards,

Sakari Ailus

RE: [PATCH 3/3] media: ivsc: csi: remove privacy status in struct mei_csi

[Wu, Wentong]

> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Hi Wentong,
> 
> Thanks for the patch.
> 
> On Mon, Jun 03, 2024 at 04:26:14PM +0800, Wentong Wu wrote:
> > The privacy status is maintained by privacy_ctrl, on which all of the
> > privacy status changes will go through, so there is no point in
> > maintaining one more element any more.
> >
> > 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 | 9 ++-------
> >  1 file changed, 2 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/pci/intel/ivsc/mei_csi.c
> > b/drivers/media/pci/intel/ivsc/mei_csi.c
> > index d6ba0d9efca1..1d1b9181a50a 100644
> > --- a/drivers/media/pci/intel/ivsc/mei_csi.c
> > +++ b/drivers/media/pci/intel/ivsc/mei_csi.c
> > @@ -138,9 +138,6 @@ struct mei_csi {
> >  	u32 nr_of_lanes;
> >  	/* frequency of the CSI-2 link */
> >  	u64 link_freq;
> > -
> > -	/* privacy status */
> > -	enum ivsc_privacy_status status;
> >  };
> >
> >  static const struct v4l2_mbus_framefmt mei_csi_format_mbus_default =
> > { @@ -271,10 +268,8 @@ static void mei_csi_rx(struct mei_cl_device
> > *cldev)
> >
> >  	switch (notif.cmd_id) {
> >  	case CSI_PRIVACY_NOTIF:
> > -		if (notif.cont.cont < CSI_PRIVACY_MAX) {
> > -			csi->status = notif.cont.cont;
> > -			v4l2_ctrl_s_ctrl(csi->privacy_ctrl, csi->status);
> > -		}
> > +		if (notif.cont.cont < CSI_PRIVACY_MAX)
> > +			v4l2_ctrl_s_ctrl(csi->privacy_ctrl, notif.cont.cont);
> 
> notif.cont.cont represents is MEI's idea of the privacy state which just
> happens to be aligned with V4L2's.
> 
> While this issue is not related to this patch, it'd be nice to use e.g.
> 
> 			v4l2_ctrl_s_ctrl(csi->privacy_ctrl,
> 					 notif.cont.cont == CSI_PRIVACY_ON);
> 

Agree, thanks

> So could you add one more patch to the set for v2?

Sure, thanks

BR,
Wentong
> 
> >  		break;
> >  	case CSI_SET_OWNER:
> >  	case CSI_SET_CONF:
> 
> --
> Kind regards,
> 
> Sakari Ailus