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