From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 9E7D5C7EE2E for ; Sun, 11 Jun 2023 12:29:35 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 3A5E785DE2; Sun, 11 Jun 2023 14:29:33 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=denx.de Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=denx.de; s=phobos-20191101; t=1686486573; bh=2riwpZeMev6Oo7K5hzqTjrusgi+M5adkhiKTeAP7+z4=; h=Date:Subject:To:Cc:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=RQYe2ikZHYlwrMW/m4dAmG61JkaDB1vEMZQcm/fOY768iVjYeJLaB++307Q/icKol KtlQ0Mi1e3dXZSe6HFZM4BOjxRJmC2POym9rUsAU5JgyybhXqQhdXZ83kSNXe68+DA QfbCDt07qIqqqcYBTtYKEtrtMY5n3AjIXelp8piJde613ZY+mlWZsT+qfGgeQMNBAk ainl/3ibQ7lsFmc9sDTfV5rPonJC9L8oQVfDKj2fE7m7fOdhtstFb9uMCRuFbJzKSX 2azta6wI7ZA/nyrGRYP0KPPBMOGig0e4C7wY7vdztvTSfwiVark40pAsQ2aV5N7hgi EYXdPAS8GpVuA== Received: from [127.0.0.1] (p578adb1c.dip0.t-ipconnect.de [87.138.219.28]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: marex@denx.de) by phobos.denx.de (Postfix) with ESMTPSA id 94CA78469B; Sun, 11 Jun 2023 14:29:30 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=denx.de; s=phobos-20191101; t=1686486571; bh=2riwpZeMev6Oo7K5hzqTjrusgi+M5adkhiKTeAP7+z4=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=uBSl7eBB3bE2j91gOn57nO/mmrfr2pt15+y552I8zoB3SEmPQ5lv4d9Zzh47HkBiY 4nOlF7r6VswCJ13AaUIZvc2HF9ZgUWQM1qLuImTQvepHm/LHc1/5vtkoa9kc74/kkS s7OHxMRikeNETULvd4V7s3sSd95m2dMteJbxMRpIC+zRTAHmUIaYnUVAReqJXniTA0 2L1NH4CDM115ufTjUKev5xsx4AmZCHo+tS5iAhnFmCHEfWoAn6NY32b6aETzCSDBWN Qc+jbDSocor4jTQycWttnIt3HTj9FsXGk6sO42UQ+EXYJyMgAz/FUNyhOrPviMrdWX YN1d3s8vs5RzQ== Message-ID: Date: Sun, 11 Jun 2023 14:29:29 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Subject: Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command To: Xavier Drudis Ferran Cc: u-boot@lists.denx.de, Simon Glass , Lukasz Majewski , Sean Anderson References: <78623f65-0cb5-fad8-7a85-f7370dda022c@denx.de> Content-Language: en-US From: Marek Vasut In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 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" X-Virus-Scanned: clamav-milter 0.103.8 at phobos.denx.de X-Virus-Status: Clean On 6/9/23 20:52, Xavier Drudis Ferran wrote: > Sorry, I replied to Marek only but meant to reply to all. > > El Fri, Jun 09, 2023 at 03:20:33AM +0200, Marek Vasut deia: > >>> No. Well, in some tests yes and some no, but I got the error in all cases. >> >> This is doubtful. It is mandatory to run 'usb start' or 'usb reset' before >> you would get any meaningful result out of 'usb info'. Without either, you >> would get 'USB is stopped.' message. Could it be there are some extra >> scripts in your environment that manipulate the USB ? >> > > I saw usb_boootdev_hunt() calls usb_init() in drivers/usb/host/usb_bootdev.c > > But maybe I don't get that called and it's really something silly in > my setup as you say later... Maybe it doesn't get called unless it > finds nothing else useful to boot. > >> >> Can you test with stock U-Boot ? >> > > I don't know. I'll see if I have time ... > I'd rather read the code to understand what's the condition for finding bootdevices... > >> Can you test with another USB stick, i.e. is the issue specific to this USB >> stick ? >> > > I could test this, yes. > >> Is the issue specific to this partition layout of this USB stick, i.e. if >> you clone (dd if=... of=...) the content of the USB stick to another USB >> stick, does the error still occur. >> > > I'll try to partition and flash a new USB. > >> [...] >> >>> Model: Radxa ROCK Pi 4B >>> Net: eth0: ethernet@fe300000 >>> Hit any key to stop autoboot: 2  1  0 >>> rockchip_pcie pcie@f8000000: PCIe link training gen1 timeout! >>> Bus usb@fe380000: USB EHCI 1.00 >>> Bus usb@fe3c0000: USB EHCI 1.00 >>> Bus usb@fe800000: Register 2000140 NbrPorts 2 >>> Starting the controller >>> USB XHCI 1.10 >>> Bus usb@fe900000: Register 2000140 NbrPorts 2 >>> Starting the controller >>> USB XHCI 1.10 >>> scanning bus usb@fe380000 for devices... 1 USB Device(s) found >>> scanning bus usb@fe3c0000 for devices... 2 USB Device(s) found >>> scanning bus usb@fe800000 for devices... 1 USB Device(s) found >>> scanning bus usb@fe900000 for devices... 1 USB Device(s) found >>> rockchip_pcie pcie@f8000000: failed to find ep-gpios property >>> ethernet@fe300000 Waiting for PHY auto negotiation to complete......... TIMEOUT ! >>> Could not initialize PHY ethernet@fe300000 >>> rockchip_pcie pcie@f8000000: failed to find ep-gpios property >>> ethernet@fe300000 Waiting for PHY auto negotiation to complete......... TIMEOUT ! >>> Could not initialize PHY ethernet@fe300000 >> >> Is this some $preboot script which initializes your hardware ? >> > > Mmm... yes, I used to have it... I thought not in this test, but I'd better recheck > > Anyway, one should be allowed to stop the boot, call usb start and usb tree > and don't get a reset, shouldn't one? > >> => printenv preboot >> > > I'll send this later when I repeat the test. I'd like to find a > minimal test case or something... Thank you >>> => usb tree >>> USB device tree: >>> 1 Hub (480 Mb/s, 0mA) >>> u-boot EHCI Host Controller >>> 1 Hub (480 Mb/s, 0mA) >>> | u-boot EHCI Host Controller >>> | >>> uclass_id=64 >>> |+-2 Mass Storage (480 Mb/s, 200mA) >>> TDK LoR TF10 07032B6B1D0ACB96 >>> uclass_id=22 >>> uclass_id=25 >>> "Synchronous Abort" handler, esr 0x96000010, far 0x948 >>> elr: 00000000002157d4 lr : 00000000002157d4 (reloc) >>> elr: 00000000f3f4f7d4 lr : 00000000f3f4f7d4 >> >> Take the u-boot (unstripped elf) which matches this binary, and run >> aarch64-...objdump -lSD on it, then search for the $lr value, see >> doc/develop/crash_dumps.rst for details. That should tell you where exactly >> the crash occurred. Where did it occur ? >> > > I didn't do it exactly so, but from u-boot.map I gathered that it was > in cmd/usb.c and the fact that my patch fixed it implies the problem > is the functions usb_show_tree_graph() or usb_show_info() get called > recursively with null as a first parameter. > > Now I don't have that u-boot.map anymore and would have to repeat the > experiment, to find out exactly as you say, so I won't do it right > now. But thanks, understood. > > The reason usb_show_tree_graph() gets called with a null usb_device * > is that the code in cmd/usb.c for usb info and usb tree assumes > everything a UCLASS_MASS_STORAGE device can have as children are > devices with some usb_device in their dev_get_parent_priv(). It > carves out exceptions to this general rule for UCLASS_USB_EMUL and > UCLASS_BLK, but not for UCLASS_BOOTDEV. When it finds a child that is > UCLASS_BOOTDEV it happily recurses on it passing its parent_priv as > usb_device, but the bootdev code did not put any usb_device there, > it's null. So the first access causes a null pointer dereference. > > I would have to wrap my mind around more code to start understanding > if it's better to give that UCLASS_BOOTDEV some usb_device as parent > priv data, or it is better to give USB devices that can be enumerated > for listing (usb tree or usb info) some RECURSIBLE flag that indicates > their priv parent data is reliably a usb_device. > > So checking that the alledged usb_device at least isn't null as in my > patch is possibly a partial solution. I'm sure if it's null we > shouldn't call, but if it points to something other than a usb_device we > shouldn't either, and it doesn't address why it is null (well, because > it's not really a USB internal node, not even a proper leaf, so it > shouldn't be recursed anyway). > > In usb_show_info() it is similar, usb_display_desc() gets called with a > null udev because that's what came in. Recursion is avoided for > UCLASS_USB_EMUL or UCLASS_BLK but not for UCLASS_BOOTDEV. > > A different solution could be to expand the exception to > UCLASS_BOOTDEV, but it still seems a wrong strategy to expect to know > everything you can find so you can list all the exceptions, instead of > checking that you have something expected that you can recurse on. Not > completely sure, just smells so to me. At least checking for null is > more general. > > Maybe the solution is to fix common/usb_storage.c when it calls > bootdev_setup_sibling_blk(), to ensure there's some useful usb_device > there as parent priv. Or something needs fixing in > drivers/usb/host/usb_bootdev.c ??? But I'm not sure. We really > shouldn't call recursively for usb info or tree anyway. > > For now I was trying to understand when that UCLASS_BOOTDEV is added > and why, and whether it should be removed at some time. This should > hint me at what is the minimum scenario to reproduce the issue. > >> >> The NAK is really only to prevent this from accidentally going on. >> > > The NAK it's OK. I like people to want to understand stuff. Don't worry. s@going on@going in@ ... typo. >> Please see above, maybe it could be narrowed down ? > > I'll see if I can test better and send more useful reports. > Not sure when. > > I'm not sure I'll have the time to learn all I need. I just hoped someone > else had it in mind... I reproducer would be a good starting point, i.e. something like "I run these ... commands and I get this crash" . But please make sure you use current codebase and no preboot or similar scripting .