Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755610AbdCTVRD (ORCPT ); Mon, 20 Mar 2017 17:17:03 -0400 Received: from bh-25.webhostbox.net ([208.91.199.152]:56334 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755475AbdCTVQ7 (ORCPT ); Mon, 20 Mar 2017 17:16:59 -0400 Date: Mon, 20 Mar 2017 14:16:56 -0700 From: Guenter Roeck To: Alan Stern Cc: Greg Kroah-Hartman , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, Douglas Anderson , Brian Norris Subject: Re: [PATCH v2] usb: hub: Do not attempt to autosuspend disconnected devices Message-ID: <20170320211656.GA1308@roeck-us.net> References: <1490025036-6578-1-git-send-email-linux@roeck-us.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) X-Authenticated_sender: guenter@roeck-us.net X-OutGoing-Spam-Status: No, score=-1.0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - bh-25.webhostbox.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - roeck-us.net X-Get-Message-Sender-Via: bh-25.webhostbox.net: authenticated_id: guenter@roeck-us.net X-Authenticated-Sender: bh-25.webhostbox.net: guenter@roeck-us.net X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3036 Lines: 82 On Mon, Mar 20, 2017 at 05:12:20PM -0400, Alan Stern wrote: > On Mon, 20 Mar 2017, Guenter Roeck wrote: > > > While running a bind/unbind stress test with the dwc3 usb driver on rk3399, > > the following crash was observed. > > > > Unable to handle kernel NULL pointer dereference at virtual address 00000218 > > pgd = ffffffc00165f000 > > [00000218] *pgd=000000000174f003, *pud=000000000174f003, > > *pmd=0000000001750003, *pte=00e8000001751713 > > ... > > > Code analysis shows that intf is set to NULL in usb_disable_device() prior > > to setting actconfig to NULL. At the same time, usb_runtime_idle() does not > > lock the usb device, and neither does any of the functions in the > > traceback. This means that there is no protection against a race condition > > where usb_disable_device() is removing dev->actconfig->interface[] pointers > > while those are being accessed from autosuspend_check(). > > > > To solve the problem, synchronize and validate device state between > > autosuspend_check() and usb_disconnect(). > > > > Signed-off-by: Guenter Roeck > > --- > > v2: Do not disable autosuspend in usb_disconnect(). Instead, check the > > usb device state in autosuspend_check(). > > In usb_disconnect(), call pm_runtime_barrier() earlier, immediately > > after changing the USB device state. > > > > drivers/usb/core/driver.c | 3 +++ > > drivers/usb/core/hub.c | 7 +++++++ > > 2 files changed, 10 insertions(+) > > > > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c > > index 7ebdf2a4e8fe..eb87a259d55c 100644 > > --- a/drivers/usb/core/driver.c > > +++ b/drivers/usb/core/driver.c > > @@ -1781,6 +1781,9 @@ static int autosuspend_check(struct usb_device *udev) > > int w, i; > > struct usb_interface *intf; > > > > + if (udev->state == USB_STATE_NOTATTACHED) > > + return -ENODEV; > > + > > /* Fail if autosuspend is disabled, or any interfaces are in use, or > > * any interface drivers require remote wakeup but it isn't available. > > */ > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > > index 57bb427c8878..fd80513aaeff 100644 > > --- a/drivers/usb/core/hub.c > > +++ b/drivers/usb/core/hub.c > > @@ -2084,6 +2084,13 @@ void usb_disconnect(struct usb_device **pdev) > > * this quiesces everything except pending urbs. > > */ > > usb_set_device_state(udev, USB_STATE_NOTATTACHED); > > + > > + /* > > + * Ensure that the pm runtime code knows that the USB device > > + * is in the process of being disconnected. > > + */ > > + pm_runtime_barrier(&udev->dev); > > + > > dev_info(&udev->dev, "USB disconnect, device number %d\n", > > udev->devnum); > > > > Very minor nit: I would place the pm_runtime_barrier() after the > dev_info(), so that we would know the device is going away if anything > should go wrong during the barrier call. > Makes sense. I'll change that and resend. Thanks, Guenter > Aside from that, > > Acked-by: Alan Stern > > Alan Stern >