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

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

[Oliver Neukum]

On Di, 2018-06-12 at 21:57 +0200, Sebastian Andrzej Siewior wrote:
> > Yes, the atomic case should be rare.  It will only happen on errors, and
> > IIUC that's only to work around issues caused by reporting errors back
> > to userspace without actually wanting to err out anyway.
> 
> Yup. The missing part is if this was done to workaround a specific
> userland application or most/all of them.

Yes, If possible we should not regress in that regard.

> > I believe it would be better to decide one an error policy and stick to
> > that.  Then you could just simplify away that whole mess, by either
> > ignoring the error and continue or bailing out and die.
> 
> "Bailing out and die" would be a revert of commit c1da59dad0eb
> ("cdc-wdm: Clear read pipeline in case of error")?
> And ignoring the error would be "not updating rerr" in
> wdm_in_callback().
> I don't care either way. I can do whatever works for you/users best.

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?

	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

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

[Sebastian Andrzej Siewior]

On 2018-06-12 20:28:27 [+0200], Bjørn Mork wrote:
> Yes, the atomic case should be rare.  It will only happen on errors, and
> IIUC that's only to work around issues caused by reporting errors back
> to userspace without actually wanting to err out anyway.

Yup. The missing part is if this was done to workaround a specific
userland application or most/all of them.

> I believe it would be better to decide one an error policy and stick to
> that.  Then you could just simplify away that whole mess, by either
> ignoring the error and continue or bailing out and die.

"Bailing out and die" would be a revert of commit c1da59dad0eb
("cdc-wdm: Clear read pipeline in case of error")?
And ignoring the error would be "not updating rerr" in
wdm_in_callback().
I don't care either way. I can do whatever works for you/users best.

> Bjørn

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

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

[Bjørn Mork]

Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes:

> On 2018-06-12 12:43:01 [-0400], Alan Stern wrote:
>> On Tue, 12 Jun 2018, Sebastian Andrzej Siewior wrote:
>> 
>> > 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.
>> > 
>> > Add an argument to service_outstanding_interrupt() which decides
>> > whether or not it is save to enable interrupts during unlocking and use
>> > GFP_KERNEL or not.
>> 
>> Wouldn't it be easier just to use spin_lock_irqsave() and GFP_ATOMIC
>> all the time?  That's what people normally do with code that can be 
>> called in both process and interrupt contexts.
>
> service_outstanding_interrupt() does unlock + lock instead lock +
> unlock. If you want to have this "always" working (without the
> argument), we could do the false case:
> +               spin_unlock(&desc->iuspin);
> +               rv = usb_submit_urb(desc->response, GFP_ATOMIC);
> +               spin_lock(&desc->iuspin);
>
> all the time. I wanted to preserve the GFP_KERNEL part which is probably
> used more often but fell that this is not needed I could drop that part.

Yes, the atomic case should be rare.  It will only happen on errors, and
IIUC that's only to work around issues caused by reporting errors back
to userspace without actually wanting to err out anyway.

I believe it would be better to decide one an error policy and stick to
that.  Then you could just simplify away that whole mess, by either
ignoring the error and continue or bailing out and die.


Bjørn
---
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

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

[Sebastian Andrzej Siewior]

On 2018-06-12 12:43:01 [-0400], Alan Stern wrote:
> On Tue, 12 Jun 2018, Sebastian Andrzej Siewior wrote:
> 
> > 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.
> > 
> > Add an argument to service_outstanding_interrupt() which decides
> > whether or not it is save to enable interrupts during unlocking and use
> > GFP_KERNEL or not.
> 
> Wouldn't it be easier just to use spin_lock_irqsave() and GFP_ATOMIC
> all the time?  That's what people normally do with code that can be 
> called in both process and interrupt contexts.

service_outstanding_interrupt() does unlock + lock instead lock +
unlock. If you want to have this "always" working (without the
argument), we could do the false case:
+               spin_unlock(&desc->iuspin);
+               rv = usb_submit_urb(desc->response, GFP_ATOMIC);
+               spin_lock(&desc->iuspin);

all the time. I wanted to preserve the GFP_KERNEL part which is probably
used more often but fell that this is not needed I could drop that part.

> Alan Stern
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

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

[Alan Stern]

On Tue, 12 Jun 2018, Sebastian Andrzej Siewior wrote:

> 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.
> 
> Add an argument to service_outstanding_interrupt() which decides
> whether or not it is save to enable interrupts during unlocking and use
> GFP_KERNEL or not.

Wouldn't it be easier just to use spin_lock_irqsave() and GFP_ATOMIC
all the time?  That's what people normally do with code that can be 
called in both process and interrupt contexts.

Alan Stern
---
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

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.

Add an argument to service_outstanding_interrupt() which decides
whether or not it is save to enable interrupts during unlocking and use
GFP_KERNEL or not.

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>
---
 drivers/usb/class/cdc-wdm.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index a0d284ef3f40..45076b9c7c5e 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -152,7 +152,7 @@ static void wdm_out_callback(struct urb *urb)
 }
 
 /* forward declaration */
-static int service_outstanding_interrupt(struct wdm_device *desc);
+static int service_outstanding_interrupt(struct wdm_device *desc, bool en_irq);
 
 static void wdm_in_callback(struct urb *urb)
 {
@@ -219,7 +219,7 @@ 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);
+		service_outstanding_interrupt(desc, false);
 	}
 
 	spin_unlock(&desc->iuspin);
@@ -448,7 +448,7 @@ static ssize_t wdm_write
  *
  * Called with desc->iuspin locked
  */
-static int service_outstanding_interrupt(struct wdm_device *desc)
+static int service_outstanding_interrupt(struct wdm_device *desc, bool en_irq)
 {
 	int rv = 0;
 
@@ -457,9 +457,15 @@ static int service_outstanding_interrupt(struct wdm_device *desc)
 		goto out;
 
 	set_bit(WDM_RESPONDING, &desc->flags);
-	spin_unlock_irq(&desc->iuspin);
-	rv = usb_submit_urb(desc->response, GFP_KERNEL);
-	spin_lock_irq(&desc->iuspin);
+	if (en_irq) {
+		spin_unlock_irq(&desc->iuspin);
+		rv = usb_submit_urb(desc->response, GFP_KERNEL);
+		spin_lock_irq(&desc->iuspin);
+	} else {
+		spin_unlock(&desc->iuspin);
+		rv = usb_submit_urb(desc->response, GFP_ATOMIC);
+		spin_lock(&desc->iuspin);
+	}
 	if (rv) {
 		dev_err(&desc->intf->dev,
 			"usb_submit_urb failed with result %d\n", rv);
@@ -544,7 +550,7 @@ static ssize_t wdm_read
 		if (!desc->reslength) { /* zero length read */
 			dev_dbg(&desc->intf->dev, "zero length - clearing WDM_READ\n");
 			clear_bit(WDM_READ, &desc->flags);
-			rv = service_outstanding_interrupt(desc);
+			rv = service_outstanding_interrupt(desc, true);
 			spin_unlock_irq(&desc->iuspin);
 			if (rv < 0)
 				goto err;
@@ -571,7 +577,7 @@ static ssize_t wdm_read
 	/* in case we had outstanding data */
 	if (!desc->length) {
 		clear_bit(WDM_READ, &desc->flags);
-		service_outstanding_interrupt(desc);
+		service_outstanding_interrupt(desc, true);
 	}
 	spin_unlock_irq(&desc->iuspin);
 	rv = cntr;