From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Thu, 12 Nov 2015 15:08:41 +0100 Subject: [U-Boot] [PATCH v2 18/26] dm: usb: Remove inactive children after a bus scan In-Reply-To: References: <1447051688-24936-1-git-send-email-sjg@chromium.org> <1447051688-24936-19-git-send-email-sjg@chromium.org> <564057DC.1000809@redhat.com> <564374A4.5080009@gmail.com> Message-ID: <56449D69.2020904@redhat.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi, On 11-11-15 19:15, Simon Glass wrote: > Hi Hans, > > On 11 November 2015 at 10:02, Hans de Goede wrote: >> Ok, I've ran a whole battery of tests on your u-boot-dm/usb-working branch. > > Thanks! > >> >> 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: >> >> --- 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 >> >> The breakage is that without this usb_scan_device() returns -96 >> (EPFNOSUPPORT) for usb keyboards. > > OK. I am a bit concerned this might affect other boards too. Still, if > we get this series applied soon there is plenty of time for testing. > I'll look a bit closer. Ok. >> 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. > > I wonder if it really is more complex. My preferred algorithm is to > remove any devices that have not been probed after a scan. Yours is to > remove and unbind any USB devices before a scan. What is the > difference? If you unbind everything before (re)scanning you always start with a clean slate, making things more reproducible and simpler. As for it being more complex, your approach needs a whole new helper function and needs to walk the list of devices twice, where as mine is just a single extra function call + error checking. As an added bonus my approach allows adding an ifdef to usb_find_child turning it into a nop like this #if defined CONFIG_SANDBOX || !defined CONFIG_DM_DEVICE_REMOVE ... ... complex code to find child ... #else return -ENODEV; #endif So besides not needing the extra helper function your solution needs, my version also removes a bunch of extra code (from the resulting binary) in almost all cases. So your solution is definitely more complex, needlessly so IMHO. > I much prefer not unbinding and rebinding devices. To my mind, devices > should be bound once if possible, and most of the time it is possible. Well in case of a re-scan we are discovering all devices from scratch, not just looking for changed devices, simply starting with a clean device list reflects this. > This is different from the Linux approach of combining 'bind' and > 'probe'. At present sandbox cannot support 'usb reset'. I would like > sandbox to provide full support so that we can use tests to verify > functionality rather than manual testing. > > Re the ordering above, I suppose I could fix that command easily > enough. This is not a common problem though. Does it really matter? Nope, as said this is a cosmetic issue, and for the record I'm fine with your approach of always re-using existing devices on a re-scan. I prefer the start with a clean slate approach but either is fine with me. >> ### >> >> 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 > > Sounds good. I am not suggesting that we encourage people to build USB > with out DM_DEVICE_REMOVE. I just want to make sure that we don't > create unnecessary dependencies such that DM_DEVICE_REMOVE becomes > impossible to use. > > We have a note in the Kconfig for that, but I agree we need to make it > more explicit. With the #if approach it makes the pitfall much more > obvious. Ack, so we need to have patches for this for at least ohci, ehci, xhci and I guess also uhci ? Who is going to write these patches ? Regards, Hans