[PATCH 0/3] MEI VSC fixes and cleanups

[PATCH 0/3] MEI VSC fixes and cleanups

[Sakari Ailus]

Hi folks,

These patches address sleeping in atomic context. It wasn't obvious at
first this was taking place since the callback sleeps while the caller (a
different driver) called it in a threaded IRQ handler.

Sakari Ailus (3):
  mei: vsc: Call wake_up() and event handler in a workqueue
  mei: vsc: Don't use sleeping condition in wait_event_timeout()
  mei: vsc: Assign pinfo fields in variable declaration

 drivers/misc/mei/vsc-tp.c | 64 ++++++++++++++++++++++++---------------
 1 file changed, 40 insertions(+), 24 deletions(-)

-- 
2.39.2

[PATCH 1/3] mei: vsc: Call wake_up() and event handler in a workqueue

[Sakari Ailus]

The event handler, in this case that of mei_vsc_event_cb() of
platform-vsc.c, is called from a threaded interrupt handler in
uninterruptible context. However there are multiple places where the
handler may sleep. This patch creates a per-device workqueue and calls
wake_up() and the event callback from queued work where sleeping is
allowed.

Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/misc/mei/vsc-tp.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index 6f4a4be6ccb5..264ece72f0bf 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -72,6 +72,8 @@ struct vsc_tp {
 	atomic_t assert_cnt;
 	wait_queue_head_t xfer_wait;
 
+	struct workqueue_struct *event_workqueue;
+	struct work_struct event_work;
 	vsc_tp_event_cb_t event_notify;
 	void *event_notify_context;
 
@@ -416,19 +418,19 @@ static irqreturn_t vsc_tp_isr(int irq, void *data)
 
 	atomic_inc(&tp->assert_cnt);
 
-	wake_up(&tp->xfer_wait);
+	queue_work(tp->event_workqueue, &tp->event_work);
 
-	return IRQ_WAKE_THREAD;
+	return IRQ_HANDLED;
 }
 
-static irqreturn_t vsc_tp_thread_isr(int irq, void *data)
+static void vsc_tp_event_work(struct work_struct *work)
 {
-	struct vsc_tp *tp = data;
+	struct vsc_tp *tp = container_of(work, struct vsc_tp, event_work);;
+
+	wake_up(&tp->xfer_wait);
 
 	if (tp->event_notify)
 		tp->event_notify(tp->event_notify_context);
-
-	return IRQ_HANDLED;
 }
 
 static int vsc_tp_match_any(struct acpi_device *adev, void *data)
@@ -481,13 +483,18 @@ static int vsc_tp_probe(struct spi_device *spi)
 	init_waitqueue_head(&tp->xfer_wait);
 	tp->spi = spi;
 
+	tp->event_workqueue = create_singlethread_workqueue(dev_name(dev));
+	if (!tp->event_workqueue)
+		return -ENOMEM;
+
+	INIT_WORK(&tp->event_work, vsc_tp_event_work);
+
 	irq_set_status_flags(spi->irq, IRQ_DISABLE_UNLAZY);
-	ret = devm_request_threaded_irq(dev, spi->irq, vsc_tp_isr,
-					vsc_tp_thread_isr,
-					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-					dev_name(dev), tp);
+	ret = devm_request_irq(dev, spi->irq, vsc_tp_isr,
+			       IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+			       dev_name(dev), tp);
 	if (ret)
-		return ret;
+		goto err_destroy_workqueue;
 
 	mutex_init(&tp->mutex);
 
@@ -516,6 +523,10 @@ static int vsc_tp_probe(struct spi_device *spi)
 
 	return 0;
 
+err_destroy_workqueue:
+	destroy_workqueue(tp->event_workqueue);
+	kfree(tp->event_workqueue);
+
 err_destroy_lock:
 	mutex_destroy(&tp->mutex);
 
@@ -528,6 +539,8 @@ static void vsc_tp_remove(struct spi_device *spi)
 
 	platform_device_unregister(tp->pdev);
 
+	destroy_workqueue(tp->event_workqueue);
+	kfree(tp->event_workqueue);
 	mutex_destroy(&tp->mutex);
 }
 
-- 
2.39.2

RE: [PATCH 1/3] mei: vsc: Call wake_up() and event handler in a workqueue

[Wu, Wentong]

Hi Sakari,

Thanks, and sorry for the late response because I'm in vacation this week.

> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> The event handler, in this case that of mei_vsc_event_cb() of platform-vsc.c,
> is called from a threaded interrupt handler in uninterruptible context.

But why this thread is uninterruptible?

https://github.com/torvalds/linux/blob/master/kernel/irq/manage.c#L1294

https://lwn.net/Articles/302043/

BR,
Wentong
> However there are multiple places where the handler may sleep. This patch
> creates a per-device workqueue and calls
> wake_up() and the event callback from queued work where sleeping is
> allowed.
> 
> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/misc/mei/vsc-tp.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c index
> 6f4a4be6ccb5..264ece72f0bf 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -72,6 +72,8 @@ struct vsc_tp {
>  	atomic_t assert_cnt;
>  	wait_queue_head_t xfer_wait;
> 
> +	struct workqueue_struct *event_workqueue;
> +	struct work_struct event_work;
>  	vsc_tp_event_cb_t event_notify;
>  	void *event_notify_context;
> 
> @@ -416,19 +418,19 @@ static irqreturn_t vsc_tp_isr(int irq, void *data)
> 
>  	atomic_inc(&tp->assert_cnt);
> 
> -	wake_up(&tp->xfer_wait);
> +	queue_work(tp->event_workqueue, &tp->event_work);
> 
> -	return IRQ_WAKE_THREAD;
> +	return IRQ_HANDLED;
>  }
> 
> -static irqreturn_t vsc_tp_thread_isr(int irq, void *data)
> +static void vsc_tp_event_work(struct work_struct *work)
>  {
> -	struct vsc_tp *tp = data;
> +	struct vsc_tp *tp = container_of(work, struct vsc_tp, event_work);;
> +
> +	wake_up(&tp->xfer_wait);
> 
>  	if (tp->event_notify)
>  		tp->event_notify(tp->event_notify_context);
> -
> -	return IRQ_HANDLED;
>  }
> 
>  static int vsc_tp_match_any(struct acpi_device *adev, void *data) @@ -
> 481,13 +483,18 @@ static int vsc_tp_probe(struct spi_device *spi)
>  	init_waitqueue_head(&tp->xfer_wait);
>  	tp->spi = spi;
> 
> +	tp->event_workqueue =
> create_singlethread_workqueue(dev_name(dev));
> +	if (!tp->event_workqueue)
> +		return -ENOMEM;
> +
> +	INIT_WORK(&tp->event_work, vsc_tp_event_work);
> +
>  	irq_set_status_flags(spi->irq, IRQ_DISABLE_UNLAZY);
> -	ret = devm_request_threaded_irq(dev, spi->irq, vsc_tp_isr,
> -					vsc_tp_thread_isr,
> -					IRQF_TRIGGER_FALLING |
> IRQF_ONESHOT,
> -					dev_name(dev), tp);
> +	ret = devm_request_irq(dev, spi->irq, vsc_tp_isr,
> +			       IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +			       dev_name(dev), tp);
>  	if (ret)
> -		return ret;
> +		goto err_destroy_workqueue;
> 
>  	mutex_init(&tp->mutex);
> 
> @@ -516,6 +523,10 @@ static int vsc_tp_probe(struct spi_device *spi)
> 
>  	return 0;
> 
> +err_destroy_workqueue:
> +	destroy_workqueue(tp->event_workqueue);
> +	kfree(tp->event_workqueue);
> +
>  err_destroy_lock:
>  	mutex_destroy(&tp->mutex);
> 
> @@ -528,6 +539,8 @@ static void vsc_tp_remove(struct spi_device *spi)
> 
>  	platform_device_unregister(tp->pdev);
> 
> +	destroy_workqueue(tp->event_workqueue);
> +	kfree(tp->event_workqueue);
>  	mutex_destroy(&tp->mutex);
>  }
> 
> --
> 2.39.2

Re: [PATCH 1/3] mei: vsc: Call wake_up() and event handler in a workqueue

[Sakari Ailus]

Hi Wentong,

On Sun, Feb 18, 2024 at 01:23:30AM +0000, Wu, Wentong wrote:
> Hi Sakari,
> 
> Thanks, and sorry for the late response because I'm in vacation this week.

No worries and thanks for the review.

> 
> > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > 
> > The event handler, in this case that of mei_vsc_event_cb() of platform-vsc.c,
> > is called from a threaded interrupt handler in uninterruptible context.
> 
> But why this thread is uninterruptible?
> 
> https://github.com/torvalds/linux/blob/master/kernel/irq/manage.c#L1294
> 
> https://lwn.net/Articles/302043/

I guess I sent this too hastily. You can indeed sleep there.

Moving wake_up() to the threaded handler should thus be enough. I'll send
v2.

-- 
Regards,

Sakari Ailus

[PATCH 2/3] mei: vsc: Don't use sleeping condition in wait_event_timeout()

[Sakari Ailus]

vsc_tp_wakeup_request() called wait_event_timeout() with
gpiod_get_value_cansleep() which may sleep, and does so as the
implementation is that of gpio-ljca.

Move the GPIO state check outside the call.

Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/misc/mei/vsc-tp.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index 264ece72f0bf..200af14490d7 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -25,7 +25,8 @@
 #define VSC_TP_ROM_BOOTUP_DELAY_MS		10
 #define VSC_TP_ROM_XFER_POLL_TIMEOUT_US		(500 * USEC_PER_MSEC)
 #define VSC_TP_ROM_XFER_POLL_DELAY_US		(20 * USEC_PER_MSEC)
-#define VSC_TP_WAIT_FW_ASSERTED_TIMEOUT		(2 * HZ)
+#define VSC_TP_WAIT_FW_POLL_TIMEOUT		(2 * HZ)
+#define VSC_TP_WAIT_FW_POLL_DELAY_US		(20 * USEC_PER_MSEC)
 #define VSC_TP_MAX_XFER_COUNT			5
 
 #define VSC_TP_PACKET_SYNC			0x31
@@ -103,13 +104,15 @@ static int vsc_tp_wakeup_request(struct vsc_tp *tp)
 	gpiod_set_value_cansleep(tp->wakeupfw, 0);
 
 	ret = wait_event_timeout(tp->xfer_wait,
-				 atomic_read(&tp->assert_cnt) &&
-				 gpiod_get_value_cansleep(tp->wakeuphost),
-				 VSC_TP_WAIT_FW_ASSERTED_TIMEOUT);
+				 atomic_read(&tp->assert_cnt),
+				 VSC_TP_WAIT_FW_POLL_TIMEOUT);
 	if (!ret)
 		return -ETIMEDOUT;
 
-	return 0;
+	return read_poll_timeout(gpiod_get_value_cansleep, ret, ret,
+				 VSC_TP_WAIT_FW_POLL_DELAY_US,
+				 VSC_TP_WAIT_FW_POLL_TIMEOUT, false,
+				 tp->wakeuphost);
 }
 
 static void vsc_tp_wakeup_release(struct vsc_tp *tp)
-- 
2.39.2

[PATCH 3/3] mei: vsc: Assign pinfo fields in variable declaration

[Sakari Ailus]

Assign all possible fields of pinfo in variable declaration, instead of
just zeroing it there.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/misc/mei/vsc-tp.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index 200af14490d7..1eda2860f63b 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -447,11 +447,16 @@ static int vsc_tp_match_any(struct acpi_device *adev, void *data)
 
 static int vsc_tp_probe(struct spi_device *spi)
 {
-	struct platform_device_info pinfo = { 0 };
+	struct vsc_tp *tp;
+	struct platform_device_info pinfo = {
+		.name = "intel_vsc",
+		.data = &tp,
+		.size_data = sizeof(tp),
+		.id = PLATFORM_DEVID_NONE,
+	};
 	struct device *dev = &spi->dev;
 	struct platform_device *pdev;
 	struct acpi_device *adev;
-	struct vsc_tp *tp;
 	int ret;
 
 	tp = devm_kzalloc(dev, sizeof(*tp), GFP_KERNEL);
@@ -508,13 +513,8 @@ static int vsc_tp_probe(struct spi_device *spi)
 		ret = -ENODEV;
 		goto err_destroy_lock;
 	}
-	pinfo.fwnode = acpi_fwnode_handle(adev);
-
-	pinfo.name = "intel_vsc";
-	pinfo.data = &tp;
-	pinfo.size_data = sizeof(tp);
-	pinfo.id = PLATFORM_DEVID_NONE;
 
+	pinfo.fwnode = acpi_fwnode_handle(adev);
 	pdev = platform_device_register_full(&pinfo);
 	if (IS_ERR(pdev)) {
 		ret = PTR_ERR(pdev);
-- 
2.39.2

Re: [PATCH 3/3] mei: vsc: Assign pinfo fields in variable declaration

[Greg Kroah-Hartman]

On Mon, Feb 12, 2024 at 11:46:18AM +0200, Sakari Ailus wrote:
> Assign all possible fields of pinfo in variable declaration, instead of
> just zeroing it there.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/misc/mei/vsc-tp.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
> index 200af14490d7..1eda2860f63b 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -447,11 +447,16 @@ static int vsc_tp_match_any(struct acpi_device *adev, void *data)
>  
>  static int vsc_tp_probe(struct spi_device *spi)
>  {
> -	struct platform_device_info pinfo = { 0 };
> +	struct vsc_tp *tp;
> +	struct platform_device_info pinfo = {
> +		.name = "intel_vsc",
> +		.data = &tp,
> +		.size_data = sizeof(tp),
> +		.id = PLATFORM_DEVID_NONE,
> +	};

But now you have potential stack data in the structure for the fields
that you aren't assigning here, right?  Is that acceptable, or will it
leak somewhere?

This is why we generally do not do this type of style.  So unless you
are fixing an issue here, please don't do it.

thanks,

greg k-h

Re: [PATCH 3/3] mei: vsc: Assign pinfo fields in variable declaration

[Arnd Bergmann]

On Mon, Feb 12, 2024, at 11:02, Greg Kroah-Hartman wrote:
> On Mon, Feb 12, 2024 at 11:46:18AM +0200, Sakari Ailus wrote:
>> Assign all possible fields of pinfo in variable declaration, instead of
>> just zeroing it there.
>> 
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>  drivers/misc/mei/vsc-tp.c | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
>> index 200af14490d7..1eda2860f63b 100644
>> --- a/drivers/misc/mei/vsc-tp.c
>> +++ b/drivers/misc/mei/vsc-tp.c
>> @@ -447,11 +447,16 @@ static int vsc_tp_match_any(struct acpi_device *adev, void *data)
>>  
>>  static int vsc_tp_probe(struct spi_device *spi)
>>  {
>> -	struct platform_device_info pinfo = { 0 };
>> +	struct vsc_tp *tp;
>> +	struct platform_device_info pinfo = {
>> +		.name = "intel_vsc",
>> +		.data = &tp,
>> +		.size_data = sizeof(tp),
>> +		.id = PLATFORM_DEVID_NONE,
>> +	};
>
> But now you have potential stack data in the structure for the fields
> that you aren't assigning here, right?  Is that acceptable, or will it
> leak somewhere?
>
> This is why we generally do not do this type of style.  So unless you
> are fixing an issue here, please don't do it.

If you have any initializer, all named fields in the structure
are zeroed. The only bits of the structure that may contain
stack data are for padding between fields, but that doesn't
actually change here from the previous version.

The old version you have here just skips the named fields
and otherwise would end up lookingn like

struct platform_device_info pinfo = {
      .parent = 0,
};

which is still a partial initializer and has the added
problem of relying on a literal '0' as a NULL pointer.
In modern compilers, one can write this as
struct platform_device_info pinfo = {}, but Sakari's
version looks best to me.

     Arnd

Re: [PATCH 3/3] mei: vsc: Assign pinfo fields in variable declaration

[Greg Kroah-Hartman]

On Mon, Feb 12, 2024 at 11:14:29AM +0100, Arnd Bergmann wrote:
> On Mon, Feb 12, 2024, at 11:02, Greg Kroah-Hartman wrote:
> > On Mon, Feb 12, 2024 at 11:46:18AM +0200, Sakari Ailus wrote:
> >> Assign all possible fields of pinfo in variable declaration, instead of
> >> just zeroing it there.
> >> 
> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> ---
> >>  drivers/misc/mei/vsc-tp.c | 16 ++++++++--------
> >>  1 file changed, 8 insertions(+), 8 deletions(-)
> >> 
> >> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
> >> index 200af14490d7..1eda2860f63b 100644
> >> --- a/drivers/misc/mei/vsc-tp.c
> >> +++ b/drivers/misc/mei/vsc-tp.c
> >> @@ -447,11 +447,16 @@ static int vsc_tp_match_any(struct acpi_device *adev, void *data)
> >>  
> >>  static int vsc_tp_probe(struct spi_device *spi)
> >>  {
> >> -	struct platform_device_info pinfo = { 0 };
> >> +	struct vsc_tp *tp;
> >> +	struct platform_device_info pinfo = {
> >> +		.name = "intel_vsc",
> >> +		.data = &tp,
> >> +		.size_data = sizeof(tp),
> >> +		.id = PLATFORM_DEVID_NONE,
> >> +	};
> >
> > But now you have potential stack data in the structure for the fields
> > that you aren't assigning here, right?  Is that acceptable, or will it
> > leak somewhere?
> >
> > This is why we generally do not do this type of style.  So unless you
> > are fixing an issue here, please don't do it.
> 
> If you have any initializer, all named fields in the structure
> are zeroed. The only bits of the structure that may contain
> stack data are for padding between fields, but that doesn't
> actually change here from the previous version.

I thought we had looked into that before and it would 0 out everything
if you just had the {0} initializer, including holes?  Or was it not, or
did it depend on the compiler/version?
Sorry, I never remember and so just recommend a memset which should be
the same overall.

> The old version you have here just skips the named fields
> and otherwise would end up lookingn like
> 
> struct platform_device_info pinfo = {
>       .parent = 0,
> };
> 
> which is still a partial initializer and has the added
> problem of relying on a literal '0' as a NULL pointer.
> In modern compilers, one can write this as
> struct platform_device_info pinfo = {}, but Sakari's
> version looks best to me.

Ok, as long as there's no stale stack data, I'm ok with it.

thanks,

greg k-h

Re: [PATCH 3/3] mei: vsc: Assign pinfo fields in variable declaration

[Sakari Ailus]

Hi Greg,

Thanks for the review.

On Mon, Feb 12, 2024 at 11:02:04AM +0100, Greg Kroah-Hartman wrote:
> On Mon, Feb 12, 2024 at 11:46:18AM +0200, Sakari Ailus wrote:
> > Assign all possible fields of pinfo in variable declaration, instead of
> > just zeroing it there.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/misc/mei/vsc-tp.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
> > index 200af14490d7..1eda2860f63b 100644
> > --- a/drivers/misc/mei/vsc-tp.c
> > +++ b/drivers/misc/mei/vsc-tp.c
> > @@ -447,11 +447,16 @@ static int vsc_tp_match_any(struct acpi_device *adev, void *data)
> >  
> >  static int vsc_tp_probe(struct spi_device *spi)
> >  {
> > -	struct platform_device_info pinfo = { 0 };
> > +	struct vsc_tp *tp;
> > +	struct platform_device_info pinfo = {
> > +		.name = "intel_vsc",
> > +		.data = &tp,
> > +		.size_data = sizeof(tp),
> > +		.id = PLATFORM_DEVID_NONE,
> > +	};
> 
> But now you have potential stack data in the structure for the fields
> that you aren't assigning here, right?  Is that acceptable, or will it
> leak somewhere?

Hmm. I'm not quite sure what you mean.

The patch changes where the fields are assigned but the variable is only
used when the assignment is done in any case, so this doesn't change
anything.

> 
> This is why we generally do not do this type of style.  So unless you
> are fixing an issue here, please don't do it.

The only caveat in initialising a struct is that the possible holes due to
ABI aren't initialised, which generally is a concern with UAPI or network
(i.e. not here, and the patch doesn't change that anyway).

-- 
Regards,

Sakari Ailus