[v2] USB: cdc-wdm: don't enable interrupts in USB-giveback

[v2] USB: cdc-wdm: don't enable interrupts in USB-giveback

[Sebastian Andrzej Siewior]

In the code path
  __usb_hcd_giveback_urb()
  -> wdm_in_callback()
   -> service_outstanding_interrupt()

The function service_outstanding_interrupt() will unconditionally enable
interrupts during unlock and invoke usb_submit_urb() with GFP_KERNEL.
If the HCD completes in BH (like ehci does) then the context remains
atomic due local_bh_disable() and enabling interrupts does not change
this.

Defer the error case handling to a workqueue as suggested by Oliver
Neukum. In case of an error the worker performs the read out and wakes
the user.

Fixes: c1da59dad0eb ("cdc-wdm: Clear read pipeline in case of error")
Cc: Robert Foss <robert.foss@collabora.com>
Cc: Oliver Neukum <oneukum@suse.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
On 2018-06-13 10:30:18 [+0200], Oliver Neukum wrote:
> It seems to me that the core of the problem is handling an error
> in irq context potentially. How about shifting it to a work queue?

Something like this then?

 drivers/usb/class/cdc-wdm.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index a0d284ef3f40..ddf55f93d4ca 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -96,6 +96,7 @@ struct wdm_device {
 	struct mutex		rlock;
 	wait_queue_head_t	wait;
 	struct work_struct	rxwork;
+	struct work_struct	service_outs_intr;
 	int			werr;
 	int			rerr;
 	int                     resp_count;
@@ -151,9 +152,6 @@ static void wdm_out_callback(struct urb *urb)
 	wake_up(&desc->wait);
 }
 
-/* forward declaration */
-static int service_outstanding_interrupt(struct wdm_device *desc);
-
 static void wdm_in_callback(struct urb *urb)
 {
 	struct wdm_device *desc = urb->context;
@@ -210,7 +208,6 @@ static void wdm_in_callback(struct urb *urb)
 	}
 skip_error:
 	set_bit(WDM_READ, &desc->flags);
-	wake_up(&desc->wait);
 
 	if (desc->rerr) {
 		/*
@@ -219,9 +216,10 @@ static void wdm_in_callback(struct urb *urb)
 		 * We should respond to further attempts from the device to send
 		 * data, so that we can get unstuck.
 		 */
-		service_outstanding_interrupt(desc);
+		schedule_work(&desc->service_outs_intr);
+	} else {
+		wake_up(&desc->wait);
 	}
-
 	spin_unlock(&desc->iuspin);
 }
 
@@ -758,6 +756,18 @@ static void wdm_rxwork(struct work_struct *work)
 	}
 }
 
+static void service_interrupt_work(struct work_struct *work)
+{
+	struct wdm_device *desc;
+
+	desc = container_of(work, struct wdm_device, service_outs_intr);
+
+	spin_lock_irq(&desc->iuspin);
+	service_outstanding_interrupt(desc);
+	wake_up(&desc->wait);
+	spin_unlock_irq(&desc->iuspin);
+}
+
 /* --- hotplug --- */
 
 static int wdm_create(struct usb_interface *intf, struct usb_endpoint_descriptor *ep,
@@ -779,6 +789,7 @@ static int wdm_create(struct usb_interface *intf, struct usb_endpoint_descriptor
 	desc->inum = cpu_to_le16((u16)intf->cur_altsetting->desc.bInterfaceNumber);
 	desc->intf = intf;
 	INIT_WORK(&desc->rxwork, wdm_rxwork);
+	INIT_WORK(&desc->service_outs_intr, service_interrupt_work);
 
 	rv = -EINVAL;
 	if (!usb_endpoint_is_int_in(ep))
@@ -964,6 +975,7 @@ static void wdm_disconnect(struct usb_interface *intf)
 	mutex_lock(&desc->wlock);
 	kill_urbs(desc);
 	cancel_work_sync(&desc->rxwork);
+	cancel_work_sync(&desc->service_outs_intr);
 	mutex_unlock(&desc->wlock);
 	mutex_unlock(&desc->rlock);
 
@@ -1065,6 +1077,7 @@ static int wdm_pre_reset(struct usb_interface *intf)
 	mutex_lock(&desc->wlock);
 	kill_urbs(desc);
 	cancel_work_sync(&desc->rxwork);
+	cancel_work_sync(&desc->service_outs_intr);
 	return 0;
 }
 

[v2] USB: cdc-wdm: don't enable interrupts in USB-giveback

[Oliver Neukum]

On Mi, 2018-06-13 at 18:28 +0200, Sebastian Andrzej Siewior wrote:
> The function service_outstanding_interrupt() will unconditionally enable
> interrupts during unlock and invoke usb_submit_urb() with GFP_KERNEL.
> If the HCD completes in BH (like ehci does) then the context remains
> atomic due local_bh_disable() and enabling interrupts does not change
> this.

Hi,

I am just looking at your patch and I am wondering why
wdm_in_callback() won't just call service_outstanding_interrupt()
again and again? OK, maybe I am dense and it may well be present now,
but it just looks to me that way.

	Regards
		Oliver
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[v2] USB: cdc-wdm: don't enable interrupts in USB-giveback

[Sebastian Andrzej Siewior]

On 2018-06-13 19:43:55 [+0200], Oliver Neukum wrote:
> On Mi, 2018-06-13 at 18:28 +0200, Sebastian Andrzej Siewior wrote:
> > The function service_outstanding_interrupt() will unconditionally enable
> > interrupts during unlock and invoke usb_submit_urb() with GFP_KERNEL.
> > If the HCD completes in BH (like ehci does) then the context remains
> > atomic due local_bh_disable() and enabling interrupts does not change
> > this.
> 
> Hi,
Hi Oliver,

> I am just looking at your patch and I am wondering why
> wdm_in_callback() won't just call service_outstanding_interrupt()
> again and again? OK, maybe I am dense and it may well be present now,
> but it just looks to me that way.

But this part didn't change, did it? The user blocks in wdmw_read()
waiting for the WDM_READ to be set and a wakeup. After rhe wakeup it
clears the ->rerr.

| static ssize_t wdm_read
| (struct file *file, char __user *buffer, size_t count, loff_t *ppos)
| {
|…
|                        rv = wait_event_interruptible(desc->wait,
|                                 test_bit(WDM_READ, &desc->flags));
|… 
|
|                if (desc->rerr) { /* read completed, error happened */
|                         rv = usb_translate_errors(desc->rerr);
|                         desc->rerr = 0;
|                         spin_unlock_irq(&desc->iuspin);
|                         goto err;
|                 }

Maybe we should delay the WDM_READ flag in the error case until the
worker is done (before the wakeup).

> 	Regards
> 		Oliver

Sebastian
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[v2] USB: cdc-wdm: don't enable interrupts in USB-giveback

[Oliver Neukum]

On Mi, 2018-06-13 at 22:28 +0200, Sebastian Andrzej Siewior wrote:
> On 2018-06-13 19:43:55 [+0200], Oliver Neukum wrote:
> > 
> 
> Hi Oliver,

Hi Sebastian,

> > I am just looking at your patch and I am wondering why
> > wdm_in_callback() won't just call service_outstanding_interrupt()
> > again and again? OK, maybe I am dense and it may well be present now,
> > but it just looks to me that way.
> 
> But this part didn't change, did it?

Right, it didn't change, but that does not make it correct.

> The user blocks in wdmw_read()

We can only hope that he does. The wait is interruptible.
If a signal comes at the wrong time, nobody will be waiting.

> Maybe we should delay the WDM_READ flag in the error case until the
> worker is done (before the wakeup).

I don't think that will help. It seems like we need to make sure
that error recovery is a one shot activity.

	Regards
		Oliver
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[v2] USB: cdc-wdm: don't enable interrupts in USB-giveback

[Sebastian Andrzej Siewior]

On 2018-06-14 10:44:20 [+0200], Oliver Neukum wrote:
> On Mi, 2018-06-13 at 22:28 +0200, Sebastian Andrzej Siewior wrote:
> > On 2018-06-13 19:43:55 [+0200], Oliver Neukum wrote:
> Hi Sebastian,
Hi Oliver,

> > > I am just looking at your patch and I am wondering why
> > > wdm_in_callback() won't just call service_outstanding_interrupt()
> > > again and again? OK, maybe I am dense and it may well be present now,
> > > but it just looks to me that way.
> > 
> > But this part didn't change, did it?
> 
> Right, it didn't change, but that does not make it correct.

Yes, I just wanted make sure that the breakage is not part of the patch.

> > The user blocks in wdmw_read()
> 
> We can only hope that he does. The wait is interruptible.
> If a signal comes at the wrong time, nobody will be waiting.
> 
> > Maybe we should delay the WDM_READ flag in the error case until the
> > worker is done (before the wakeup).
> 
> I don't think that will help. 

Why not? If it won't see WDM_READ flag set then it will return with
-EBUSY.

> It seems like we need to make sure
> that error recovery is a one shot activity.

I think it will be if we delay the WDM_READ because the user will not be
aware of the situation until the error recovery is done and not after
the URB has been submitted:

--->8

Subject: [PATCH v3] USB: cdc-wdm: don't enable interrupts in USB-giveback

In the code path
  __usb_hcd_giveback_urb()
  -> wdm_in_callback()
   -> service_outstanding_interrupt()

The function service_outstanding_interrupt() will unconditionally enable
interrupts during unlock and invoke usb_submit_urb() with GFP_KERNEL.
If the HCD completes in BH (like ehci does) then the context remains
atomic due local_bh_disable() and enabling interrupts does not change
this.

Defer the error case handling to a workqueue as suggested by Oliver
Neukum. In case of an error the worker performs the read out and wakes
the user.

Fixes: c1da59dad0eb ("cdc-wdm: Clear read pipeline in case of error")
Cc: Robert Foss <robert.foss@collabora.com>
Cc: Oliver Neukum <oneukum@suse.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v2…v3: set WDM_READ once error recovery is done so the user does not see
       WDM_READ before the _last_ submitted.

v1…v2: invoke service_outstanding_interrupt() from a worker

 drivers/usb/class/cdc-wdm.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index a0d284ef3f40..ddf55f93d4ca 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -96,6 +96,7 @@ struct wdm_device {
 	struct mutex		rlock;
 	wait_queue_head_t	wait;
 	struct work_struct	rxwork;
+	struct work_struct	service_outs_intr;
 	int			werr;
 	int			rerr;
 	int                     resp_count;
@@ -151,9 +152,6 @@ static void wdm_out_callback(struct urb *urb)
 	wake_up(&desc->wait);
 }
 
-/* forward declaration */
-static int service_outstanding_interrupt(struct wdm_device *desc);
-
 static void wdm_in_callback(struct urb *urb)
 {
 	struct wdm_device *desc = urb->context;
@@ -210,7 +208,6 @@ static void wdm_in_callback(struct urb *urb)
 	}
 skip_error:
 	set_bit(WDM_READ, &desc->flags);
-	wake_up(&desc->wait);
 
 	if (desc->rerr) {
 		/*
@@ -219,9 +216,10 @@ static void wdm_in_callback(struct urb *urb)
 		 * We should respond to further attempts from the device to send
 		 * data, so that we can get unstuck.
 		 */
-		service_outstanding_interrupt(desc);
+		schedule_work(&desc->service_outs_intr);
+	} else {
+		wake_up(&desc->wait);
 	}
-
 	spin_unlock(&desc->iuspin);
 }
 
@@ -758,6 +756,18 @@ static void wdm_rxwork(struct work_struct *work)
 	}
 }
 
+static void service_interrupt_work(struct work_struct *work)
+{
+	struct wdm_device *desc;
+
+	desc = container_of(work, struct wdm_device, service_outs_intr);
+
+	spin_lock_irq(&desc->iuspin);
+	service_outstanding_interrupt(desc);
+	wake_up(&desc->wait);
+	spin_unlock_irq(&desc->iuspin);
+}
+
 /* --- hotplug --- */
 
 static int wdm_create(struct usb_interface *intf, struct usb_endpoint_descriptor *ep,
@@ -779,6 +789,7 @@ static int wdm_create(struct usb_interface *intf, struct usb_endpoint_descriptor
 	desc->inum = cpu_to_le16((u16)intf->cur_altsetting->desc.bInterfaceNumber);
 	desc->intf = intf;
 	INIT_WORK(&desc->rxwork, wdm_rxwork);
+	INIT_WORK(&desc->service_outs_intr, service_interrupt_work);
 
 	rv = -EINVAL;
 	if (!usb_endpoint_is_int_in(ep))
@@ -964,6 +975,7 @@ static void wdm_disconnect(struct usb_interface *intf)
 	mutex_lock(&desc->wlock);
 	kill_urbs(desc);
 	cancel_work_sync(&desc->rxwork);
+	cancel_work_sync(&desc->service_outs_intr);
 	mutex_unlock(&desc->wlock);
 	mutex_unlock(&desc->rlock);
 
@@ -1065,6 +1077,7 @@ static int wdm_pre_reset(struct usb_interface *intf)
 	mutex_lock(&desc->wlock);
 	kill_urbs(desc);
 	cancel_work_sync(&desc->rxwork);
+	cancel_work_sync(&desc->service_outs_intr);
 	return 0;
 }
 

[v2] USB: cdc-wdm: don't enable interrupts in USB-giveback

[Oliver Neukum]

On Do, 2018-06-14 at 13:17 +0200, Sebastian Andrzej Siewior wrote:
> @@ -964,6 +975,7 @@ static void wdm_disconnect(struct usb_interface *intf)
>         mutex_lock(&desc->wlock);
>         kill_urbs(desc);
>         cancel_work_sync(&desc->rxwork);
> +       cancel_work_sync(&desc->service_outs_intr);
>         mutex_unlock(&desc->wlock);
>         mutex_unlock(&desc->rlock);
>  
> @@ -1065,6 +1077,7 @@ static int wdm_pre_reset(struct usb_interface *intf)
>         mutex_lock(&desc->wlock);
>         kill_urbs(desc);
>         cancel_work_sync(&desc->rxwork);
> +       cancel_work_sync(&desc->service_outs_intr);
>         return 0;

Almost good. I am afraid the work also needs to be canceled in
wdm_suspend()

	Regards
		Oliver
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html