| Message ID | 20180612163132.a7up32fngdk5e3si@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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;
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(-)