* [v4] USB: cdc-wdm: don't enable interrupts in USB-giveback
@ 2018-06-14 16:36 Sebastian Andrzej Siewior
0 siblings, 0 replies; 2+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-06-14 16:36 UTC (permalink / raw)
To: Oliver Neukum
Cc: Robert Foss, tglx, Greg Kroah-Hartman, Bjørn Mork,
Alan Stern, linux-usb
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>
---
v3…v4: also cancel ->service_outs_intr during suspend.
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 | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index a0d284ef3f40..203bbd378858 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;
@@ -209,8 +207,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 +215,11 @@ 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 {
+ set_bit(WDM_READ, &desc->flags);
+ wake_up(&desc->wait);
}
-
spin_unlock(&desc->iuspin);
}
@@ -758,6 +756,21 @@ 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);
+ if (!desc->resp_count) {
+ set_bit(WDM_READ, &desc->flags);
+ 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 +792,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 +978,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);
@@ -1006,6 +1021,7 @@ static int wdm_suspend(struct usb_interface *intf, pm_message_t message)
/* callback submits work - order is essential */
kill_urbs(desc);
cancel_work_sync(&desc->rxwork);
+ cancel_work_sync(&desc->service_outs_intr);
}
if (!PMSG_IS_AUTO(message)) {
mutex_unlock(&desc->wlock);
@@ -1065,6 +1081,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;
}
^ permalink raw reply related [flat|nested] 2+ messages in thread
* [v4] USB: cdc-wdm: don't enable interrupts in USB-giveback
@ 2018-06-19 13:58 Oliver Neukum
0 siblings, 0 replies; 2+ messages in thread
From: Oliver Neukum @ 2018-06-19 13:58 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Robert Foss, tglx, Greg Kroah-Hartman, Bjørn Mork,
Alan Stern, linux-usb
On Do, 2018-06-14 at 18:36 +0200, 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.
>
> 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>
<Acked-by: Oliver Neukum <oneukum@suse.com>
It fixes the identified issue.
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-06-19 13:58 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-14 16:36 [v4] USB: cdc-wdm: don't enable interrupts in USB-giveback Sebastian Andrzej Siewior
2018-06-19 13:58 Oliver Neukum
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.