From patchwork Wed Jun 13 16:28:06 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sebastian Andrzej Siewior X-Patchwork-Id: 10462635 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 735836020F for ; Wed, 13 Jun 2018 16:28:17 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 632F728DBE for ; Wed, 13 Jun 2018 16:28:17 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 5716F28DDC; Wed, 13 Jun 2018 16:28:17 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id DB2C628DBE for ; Wed, 13 Jun 2018 16:28:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935195AbeFMQ2P (ORCPT ); Wed, 13 Jun 2018 12:28:15 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:44706 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934316AbeFMQ2O (ORCPT ); Wed, 13 Jun 2018 12:28:14 -0400 Received: from bigeasy by Galois.linutronix.de with local (Exim 4.80) (envelope-from ) id 1fT8cw-0008JS-Pz; Wed, 13 Jun 2018 18:28:06 +0200 Date: Wed, 13 Jun 2018 18:28:06 +0200 From: Sebastian Andrzej Siewior To: Oliver Neukum Cc: =?utf-8?B?QmrDuHJu?= Mork , Robert Foss , Greg Kroah-Hartman , Alan Stern , linux-usb@vger.kernel.org, tglx@linutronix.de Subject: [PATCH v2] USB: cdc-wdm: don't enable interrupts in USB-giveback Message-ID: <20180613162806.6brovyan4yvz7m43@linutronix.de> References: <20180612163132.a7up32fngdk5e3si@linutronix.de> <20180612174849.hbv7gxko7d7yd4dv@linutronix.de> <87fu1rsvuc.fsf@miraculix.mork.no> <20180612195741.xdambsuiavcrtptp@linutronix.de> <1528878618.28212.17.camel@suse.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1528878618.28212.17.camel@suse.com> User-Agent: NeoMutt/20180512 Sender: linux-usb-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 Cc: Oliver Neukum Signed-off-by: Sebastian Andrzej Siewior --- 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; }