From patchwork Wed Nov 11 17:02:28 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans de Goede X-Patchwork-Id: 543147 X-Patchwork-Delegate: sjg@chromium.org Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from theia.denx.de (theia.denx.de [85.214.87.163]) by ozlabs.org (Postfix) with ESMTP id 0C1B1140761 for ; Thu, 12 Nov 2015 09:56:58 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b=dgowursM; dkim-atps=neutral Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 1DF0B4B764; Wed, 11 Nov 2015 23:56:54 +0100 (CET) Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id of1y5_TBb8mj; Wed, 11 Nov 2015 23:56:54 +0100 (CET) Received: from theia.denx.de (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 875774B766; Wed, 11 Nov 2015 23:56:48 +0100 (CET) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id D26AF4B6CD for ; Wed, 11 Nov 2015 18:02:34 +0100 (CET) Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id g4rjEVK6R631 for ; Wed, 11 Nov 2015 18:02:34 +0100 (CET) X-policyd-weight: NOT_IN_SBL_XBL_SPAMHAUS=-1.5 NOT_IN_SPAMCOP=-1.5 NOT_IN_BL_NJABL=-1.5 (only DNSBL check requested) Received: from mail-wm0-f51.google.com (mail-wm0-f51.google.com [74.125.82.51]) by theia.denx.de (Postfix) with ESMTPS id 8DBA94B676 for ; Wed, 11 Nov 2015 18:02:30 +0100 (CET) Received: by wmec201 with SMTP id c201so55535207wme.1 for ; Wed, 11 Nov 2015 09:02:30 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-type:content-transfer-encoding; bh=LSVcShzZ3SAm7OUVJm3vEzbx6itoStlkSgu6GRs32nU=; b=dgowursM8Z/VGH7cZGCeWrWFb8Ny28aX+zFJKUV6OpdOKKvZ4ejylsEun/rdgmsfRC 8Kl7w8cH8Nr30JVrMO0/tYfcigVqp8WlMSYzExjE9TFlSHRSuLyjrfYJ2QC5I51OUlB6 nEZA4pIARJdmCtSq5wCu84hOCgo4aWBsXlvdgKIm+PG3a4FCpjtaj2uiwoslqzbQ6xcd YvfqT+M0kBV3xmhP0n0JmNHoGUJPNBqrrdVmWA7rv94F/0ezBLSarrw/JyJ+z5Pns3V/ iuJSjr9SV41JZQNYh3JBaqWDXVupuzUtJMuapoaYxeXfCKX7s9odB49x7QUWPwdjfdwv 2vDg== X-Received: by 10.28.183.215 with SMTP id h206mr4757238wmf.0.1447261350565; Wed, 11 Nov 2015 09:02:30 -0800 (PST) Received: from shalem.localdomain (546A55B7.cm-12-3b.dynamic.ziggo.nl. [84.106.85.183]) by smtp.googlemail.com with ESMTPSA id 20sm26205835wmh.8.2015.11.11.09.02.29 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 11 Nov 2015 09:02:29 -0800 (PST) To: Simon Glass References: <1447051688-24936-1-git-send-email-sjg@chromium.org> <1447051688-24936-19-git-send-email-sjg@chromium.org> <564057DC.1000809@redhat.com> From: Hans de Goede Message-ID: <564374A4.5080009@gmail.com> Date: Wed, 11 Nov 2015 18:02:28 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: X-Mailman-Approved-At: Wed, 11 Nov 2015 23:56:44 +0100 Cc: Marek Vasut , U-Boot Mailing List Subject: Re: [U-Boot] [PATCH v2 18/26] dm: usb: Remove inactive children after a bus scan X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.15 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" Hi, On 11-11-15 00:30, Simon Glass wrote: > Hi Hans, > > On 9 November 2015 at 12:25, Simon Glass wrote: >> Hi Hans, >> >> On 9 November 2015 at 00:22, Hans de Goede wrote: >>> Hi, >>> >>> On 09-11-15 07:48, Simon Glass wrote: >>>> >>>> Each scan of the USB bus may return different results. Existing >>>> driver-model >>>> devices are reused when found, but if a device no longer exists it will >>>> stay >>>> around, de-activated, but bound. >>>> >>>> Detect these devices and remove them after the scan completes. >>>> >>>> Signed-off-by: Simon Glass >>> >>> >>> I wonder how this is better then my original: >>> "dm: usb: Use device_unbind_children to clean up usb devs on stop" >>> >>> Patch, the end result of both patches is the same and both are >>> a NOP when DM_DEVICE_REMOVE is not set. Where as my code seems >>> to be a much more KISS approach to the problem (my approach is >>> just 3 lines vs 23 lines for yours). >>> >>> I know we will need usb_find_child in the DM_DEVICE_REMOVE not >>> set case, but why not only revert the: >>> >>> "dm: usb: Rename usb_find_child to usb_find_emul_child" >>> >>> commit, keep the other 2 you revert and drop this patch ? >>> >>> This drops 3 patches from your patch-set and the end result is >>> more clean IMHO. >> >> I would like to avoid binding/unbinding things when nothing changes if >> possible. Also I'd like to support attaching device tree >> nodes/properties to USB devices as necessary, as we do with PCI, and >> removing things breaks that. >> >> I still have to figure out one more test case, so I'll do that before >> commenting further. > > I've added the test case and pushed a new tree. However it turns out > that this doesn't behave differently. > > So please can you go ahead and run your original (manual) test case? > I'd like to make sure that my automated tests are correct and catch > the bug you reported and fixed. Ok, I've ran a whole battery of tests on your u-boot-dm/usb-working branch. There are 2 issues: 1) You need to add these change to the commit introducing usb-keyb dm support (or one of the related commits), otherwise usb-keyb support breaks: The breakage is that without this usb_scan_device() returns -96 (EPFNOSUPPORT) for usb keyboards. 2) With that branch there still is the purely cosmetical issue, that if one plugs a usb-stick + keyb into the same hub, then swaps their place and do "usb reset" and then "usb tree" looks like this: USB device tree: 1 Hub (480 Mb/s, 100mA) | USB2.0 Hub | +-3 Human Interface (1.5 Mb/s, 100mA) | SINO WEALTH USB Composite Device | +-2 Mass Storage (480 Mb/s, 100mA) USB Flash Disk 4C0E960F Notice the order of devices listed: 1 - 3 - 2, which is a bit weird, as said this is purely cosmetical. Note you can easily fix this by only reverting the: "dm: usb: Rename usb_find_child to usb_find_emul_child" commit, and keep the other 2 you revert and dropping this patch ? As I already suggested before, this is both more KISS and as you can see it solves some actually (admittedly very minor) issues. I'm not really buying your arguments for your more complex solution, as shown above re-using the devices actually causes issues. Your other argument of wanting to attach device-tree properties I also do not find a strong argument, for one there has never been a need to do so sofar, and if we ever need this we need a way to specify which usb-device the properties belong to based on the topology under the host / root-hub anyway, and match things up when first scanning the bus. And if we can match things up on the first scan we can also match them up on subsequent scans and attach the same of-node again. Anyways I'm fine with doing things your way, but I still have a preference for the more KISS and IMHO robust solution of simple unbinding all devices on usb-stop. ### Talking about usb-stop, there is still one (BIG!) problem after this patch set when building usb-support with DM_DEVICE_REMOVE not set. This means that the controllers dma engines will not be stopped when booting the actual OS and they will still be accessing parts of the main memory while the actual OS is booting, which is BAD. So I suggest adding something like this to all host drivers which use dma in this way: #if defined CONFIG_DM_USB && !defined CONFIG_DM_DEVICE_REMOVE #error The EHCI driver cannot be used without CONFIG_DM_DEVICE_REMOVE otherwise DMA stays active while booting the OS #endif Regards, Hans > >> >>> >>> >>>> Changes in v2: None >>>> >>>> drivers/usb/host/usb-uclass.c | 23 +++++++++++++++++++++++ >>>> 1 file changed, 23 insertions(+) >>>> >>>> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c >>>> index 4aa92f8..50538e0 100644 >>>> --- a/drivers/usb/host/usb-uclass.c >>>> +++ b/drivers/usb/host/usb-uclass.c >>>> @@ -202,6 +202,20 @@ static void usb_scan_bus(struct udevice *bus, bool >>>> recurse) >>>> printf("%d USB Device(s) found\n", priv->next_addr); >>>> } >>>> >>>> +static void remove_inactive_children(struct uclass *uc, struct udevice >>>> *bus) >>>> +{ >>>> + uclass_foreach_dev(bus, uc) { >>>> + struct udevice *dev, *next; >>>> + >>>> + if (!device_active(bus)) >>>> + continue; >>>> + device_foreach_child_safe(dev, next, bus) { >>>> + if (!device_active(dev)) >>>> + device_unbind(dev); >>>> + } >>>> + } >>>> +} >>>> + >>>> int usb_init(void) >>>> { >>>> int controllers_initialized = 0; >>>> @@ -270,6 +284,15 @@ int usb_init(void) >>>> } >>>> >>>> debug("scan end\n"); >>>> + >>>> + /* Remove any devices that were not found on this scan */ >>>> + remove_inactive_children(uc, bus); >>>> + >>>> + ret = uclass_get(UCLASS_USB_HUB, &uc); >>>> + if (ret) >>>> + return ret; >>>> + remove_inactive_children(uc, bus); >>>> + >>> >>> >>> Why do you need to call remove_inactive_children twice here? This seems >>> worthy of a comment explaining why this is necessary. >> >> One is removing the children of USB controllers, one is removing the >> children of USB hubs. I'll add a comment. >> >>> >>>> /* if we were not able to find at least one working bus, bail out >>>> */ >>>> if (!count) >>>> printf("No controllers found\n"); >> >> Regards, >> Simon --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -513,6 +513,7 @@ config ARCH_SUNXI select DM select DM_GPIO select DM_ETH + select DM_KEYBOARD select DM_SERIAL select DM_USB select OF_CONTROL