Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command
On 6/20/23 12:43, Xavier Drudis Ferran wrote: El Tue, Jun 20, 2023 at 11:49:36AM +0200, Marek Vasut deia: Default, see: $ git grep CONFIG_BOOTCOMMAND configs/ I'm lost. I called default what Kconfig used as default. You seem to call default what's in board specific config files. Whatever. Fix the wording in the commit message if you like. All I am really asking for is minimal test case. That is all I was asking for since the very beginning. So to answer my own question, this is the minimal test case: => usb reset ; bootflow scan ; usb info Can you please add it to the commit message, collect RB from Simon, add Reviewed-by: Marek Vasut Tested-by: Marek Vasut And send V2 so I can pick it for this release ? Thank you
Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command
On Tue, 20 Jun 2023 at 12:20, Xavier Drudis Ferran wrote: > > El Tue, Jun 20, 2023 at 11:03:57AM +0100, Simon Glass deia: > > Hi Xavier, > > > > Hi Simon, > > > > > > > It is also possible that one day a device that is not UCLASS_BLK, > > > UCLASS_BOOTDEV or UCLASS_USB_EMUL is put as children of a usb storage > > > device (just imagine a future system similar to bootstd for firmware > > > updates, trust material, etc.). Is it likely to have a struct in > > > parent_priv_ that is not a usb_device ? > > > > > > So which is more likely to survive future changes ? > > > > > > - checking for parent_priv_ not null and not UCLASS_USB_EMUL > > > > > > - checking for parent_priv_ not null and not UCLASS_USB_EMUL and not > > > UCLASS_BLK > > > (my patch, overcautious ?) > > > > > > - checking for not (UCLASS_BLK, UCLASS_BOOTDEV or UCLASS_USB_EMUL) > > > (Simon Glass' idea) > > > > > > - checking for not UCLASS_BLK and not UCLASS_BOOTDEV and not > > > UCLASS_USB_EMUL > > >and parent_priv_ not null > > > > Really the parent_priv thing is a separate issue, a side effect of > > something having a UCLASS_USB parent. > > > > I don't think it's a separate issue. If parent_priv is present it > could be a usb_device (most likely) or not, but if it's null there's > no way the recursive call can succeed. > > > The key point here is that we cannot iterate down into a bootdev > > device looking for USB devices. So we should use that as the check, > > since it is the most direct check. > > > > But things keep appearing that have a UCLASS_USB* parent and no > parent_priv. > in 2017 Suneel Garapati already fixed the issue of UCLASS_BLK > being a child of a device. Now it's UCLASS_BOOTDEV, and tomorrow > may be something else. > > The most direct check will miss future cases as the devices tend to > become more abstract instead of mapping one to one to physical stuff. > > > > > >From my memory, I think you can check for a USB hub instead, but I'm > > not completely sure. > > > > On second thoughts I didn't find it so easy. There's the root hub, > UCLASS_USB, I think and a UCLASS_USB_EMUL may also be a hub, so I > don't know anymore how more elegant that could be, so I left it be. > > > I suggest adding a test for the command (see test/dm/acpi.c for an > > example) since a test is the best way to ensure this doesn't happen > > again. > > > > Makes sense. But I don't have any more time for that, sorry. > > I think we'll have to leave it at this unless someone else has the time. Reviewed-by: Simon Glass
Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command
El Tue, Jun 20, 2023 at 11:03:57AM +0100, Simon Glass deia: > Hi Xavier, > Hi Simon, > > > > It is also possible that one day a device that is not UCLASS_BLK, > > UCLASS_BOOTDEV or UCLASS_USB_EMUL is put as children of a usb storage > > device (just imagine a future system similar to bootstd for firmware > > updates, trust material, etc.). Is it likely to have a struct in > > parent_priv_ that is not a usb_device ? > > > > So which is more likely to survive future changes ? > > > > - checking for parent_priv_ not null and not UCLASS_USB_EMUL > > > > - checking for parent_priv_ not null and not UCLASS_USB_EMUL and not > > UCLASS_BLK > > (my patch, overcautious ?) > > > > - checking for not (UCLASS_BLK, UCLASS_BOOTDEV or UCLASS_USB_EMUL) > > (Simon Glass' idea) > > > > - checking for not UCLASS_BLK and not UCLASS_BOOTDEV and not > > UCLASS_USB_EMUL > >and parent_priv_ not null > > Really the parent_priv thing is a separate issue, a side effect of > something having a UCLASS_USB parent. > I don't think it's a separate issue. If parent_priv is present it could be a usb_device (most likely) or not, but if it's null there's no way the recursive call can succeed. > The key point here is that we cannot iterate down into a bootdev > device looking for USB devices. So we should use that as the check, > since it is the most direct check. > But things keep appearing that have a UCLASS_USB* parent and no parent_priv. in 2017 Suneel Garapati already fixed the issue of UCLASS_BLK being a child of a device. Now it's UCLASS_BOOTDEV, and tomorrow may be something else. The most direct check will miss future cases as the devices tend to become more abstract instead of mapping one to one to physical stuff. > > >From my memory, I think you can check for a USB hub instead, but I'm > not completely sure. > On second thoughts I didn't find it so easy. There's the root hub, UCLASS_USB, I think and a UCLASS_USB_EMUL may also be a hub, so I don't know anymore how more elegant that could be, so I left it be. > I suggest adding a test for the command (see test/dm/acpi.c for an > example) since a test is the best way to ensure this doesn't happen > again. > Makes sense. But I don't have any more time for that, sorry. I think we'll have to leave it at this unless someone else has the time. Bye.
Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command
El Tue, Jun 20, 2023 at 11:49:36AM +0200, Marek Vasut deia: > > Default, see: > $ git grep CONFIG_BOOTCOMMAND configs/ > I'm lost. I called default what Kconfig used as default. You seem to call default what's in board specific config files. Whatever. Fix the wording in the commit message if you like. > > So, what is the minimal test case ? > I have been asking for that for a while. > I sent a minimal test case last week. https://lists.denx.de/pipermail/u-boot/2023-June/520109.html You build for a Rock Pi 4 board, boot with usb stick and no boot media and run usb info and you get a reset. I won't send it again because I can't guess what you consider minimal. > > I would really like a minimal test case. Empty your env and figure out the > commands which need to be executed to trigger this. Without any interference > from other commands/scripting/... > I'm sorry but if what I sent isn't enough I don't think I'll have time to help you any further. Find your minimal test case yourself or ignore my patch. > > If it's just that you can't reproduce it, can you try to ?: > > > > - set up a board with no boot media (I tested like this but it might > >not be needed), > > > > - put usb in boot_targets (if you put only usb there you may not need > >the previous step): > >setenv boot_targets usb > > Here you assume distro bootcommand or some such . Can we remove that > assumption ? (I think yes, and we should) > I don't think I'm assuming anything about bootcommand. That's precisely why I wrote these steps instead of the "just boot a Rock Pi 4" scenario last week. The commit message talks about bootcmd because it justifies that the bootflow scan will be called automatically in some cases, so the bug has more impact that it would otherwise have. But the bug should appear whether or not you have bootcmd. The bug should be an interaction between what bootflow scan does and what usb info or usb tree do. I'm assuming bootflow reads the boot_targets environment variable to know where it searches for boot devices, and therefore to which devices it will attach a UCLASS_BOOTDEV child to some devices, in particular to usb mass storage devices if any is present, that will later break usb info/usb tree. Whether bootflow is called from bootcmd or not should be irrelevant. If you follow the code from the bootflow command you may find yourself that the boot_targets variable is involved. I did it last week or sometime and won't do it again now for you, sorry. I know I may have misunderstood something, and I'm sorry for the noise if so. > > - plug a non-booting usb mass storage device to an usb port, > > > > - run usb reset in case you already had usb initialized at boot, or > >skip this if usb is not initialized yet. If in doubt, run usb reset. > > > > - run bootflow scan > > > > - run usb info > > > > It should list some devices, but give you a reset just after listing the > > usb mass storage device without my patch, and it should just list all > > usb devices and go back to the prompt with my patch. > > Does it crash if you empty your env and run simply > > => usb reset ; bootflow scan ; usb info > > ? I guess it won't crash if environment var boot_targets is absent or empty. Or even if it's full but has no "usb" in it. But I'm not sure anymore at what time the variable is read, so it might be that emptying it when bootflow structures are already set up wouldn't change things, I don't know, I don't remember. But I won't try it, sorry. I found a bug, I sent a 4 line patch. Since I didn't justify it enough I sent followup mails on how to reproduce. This week I sent a second version, with redundant measures to seek consensus. It's a 6 line patch or 8 or whatever. I didn't write bootflow, and I didn't write cmd/usb.c And I don't have time to keep writing long emails. I'm sorry. Not even to count how many lines of text I wrote compared to the 8 lines of code or whatever. If someone has the bureaucratic skills and patience to pursue this further, they can do what they want with my patch (under GPL2 as implied by the sign off). If not, you all can keep your bugs, I won't try anymore to steal them. Bye and sorry for any disturbances.
Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command
Hi Xavier,
On Wed, 14 Jun 2023 at 09:40, Xavier Drudis Ferran wrote:
>
>
> Thanks for explaining, Simon Glass.
>
> Can someone please stop me if I'm splitting hairs or bikeshedding or
> something ? I guess we agree both checking for null or UCLASS_BOOTDEV
> would work right now but we are looking for what's more future-proof.
>
> El Tue, Jun 13, 2023 at 09:12:30PM +0100, Simon Glass deia:
> > Hi Xavier,
> >
> > On Tue, 13 Jun 2023 at 17:04, Xavier Drudis Ferran
> > wrote:
> > >
> > > El Tue, Jun 13, 2023 at 03:58:22PM +0100, Simon Glass deia:
> > > >
> > > > Yes that's right. So 'usb info' should ignore UCLASS_BOOTDEV devices.
> > >
> > > That's a possibility, yes. It should work until someone adds another
> > > device there for some other purpose.
> > >
> > > > That is better than checking for the NULL pointer.
> > > >
> > >
> > > Why? What's wrong about checking for null ?
> > > Or maybe checking for both not null and not UCLASS_BOOTDEV ?
> > >
> > > Not that I care too much, just to understand your reasoning.
> >
> > Well, devices may have something attached there, so it may be
> > non-NULL, but point to a different struct from the expected one.
> >
>
> Yes. That is possible. How likely ?
>
> That "there" is dev_get_parent_priv(). If I understand the driver
> model, those devices shouldn't put things there themselves, it should
> be their parent who puts stuff there for them. And the parent should
> be an usb_device->dev (because of the recursive code). So what other
> struct would such a parent put in a child parent_priv_ ? Yes, whatever
> it wants, but I mean, isn't it likely to assume the child would be
> "adopted" with null as parent_priv_ or a "natural child" with a usb_device
> in parent_priv_ ?
>
> I did a very rough search
>
> grep -rIE 'UCLASS_(BLK|BOOTDEV|USB_EMUL)' . |grep -F .id |cut -f1 -d: |xargs
> -n 1 grep -l per_child_auto
> ./drivers/usb/emul/usb-emul-uclass.c
>
> Which seems to suggest only UCLASS_USB_EMUL would have parent_priv_,
> and that would be an usb_device, which the current code does not want
> to recurse anyway. (dev_set_parent_priv() is almost never called).
>
> It is also possible that one day a device that is not UCLASS_BLK,
> UCLASS_BOOTDEV or UCLASS_USB_EMUL is put as children of a usb storage
> device (just imagine a future system similar to bootstd for firmware
> updates, trust material, etc.). Is it likely to have a struct in
> parent_priv_ that is not a usb_device ?
>
> So which is more likely to survive future changes ?
>
> - checking for parent_priv_ not null and not UCLASS_USB_EMUL
>
> - checking for parent_priv_ not null and not UCLASS_USB_EMUL and not
> UCLASS_BLK
> (my patch, overcautious ?)
>
> - checking for not (UCLASS_BLK, UCLASS_BOOTDEV or UCLASS_USB_EMUL)
> (Simon Glass' idea)
>
> - checking for not UCLASS_BLK and not UCLASS_BOOTDEV and not UCLASS_USB_EMUL
>and parent_priv_ not null
Really the parent_priv thing is a separate issue, a side effect of
something having a UCLASS_USB parent.
The key point here is that we cannot iterate down into a bootdev
device looking for USB devices. So we should use that as the check,
since it is the most direct check.
>
> > >
> > > Or can we check for recursible devices somehow ?
> > > Maybe flag them or something ?
> > >
> > > Why is better to state all devices are recursible except some UCLASSes
> > > (meaning they can have stuff that needs listed in usb info - or
> > > likewise usb tree -) instead of stating that some closed set are
> > > recursible ?
> > >
> >
> > For USB we can have lots of different types of devices as children, so
> > it would be a pain to enumerate them all. At least that's how I see
> > it.
> >
>
> We can have lots of devices as children, but those do get listed
> before the recursive call. How many of those can have children that
> we have to list too, i.e. how many of those do we want to recurse
> into ?
>
> I can only think of usb hubs (maybe some node for multifunction
> devices too ???). Maybe I'm not understanding how U-Boot works with
> USB devices... but it looks like it would be enough to look for
> UCLASS_USB_HUB, then recursive call, else no recursion. I mean instead of
>
> cmd/usb.c : usb_show_tree_graph() :
>
> if ((device_get_uclass_id( child ) != UCLASS_USB_EMUL) &&
> (device_get_uclass_id( child ) != UCLASS_BOOTDEV) &&
> (device_get_uclass_id( child ) != UCLASS_BLK)) {
> usb_show_tree_graph(udev, pre);
> pre[index] = 0;
> }
>
> we could simply have
>
> if (device_get_uclass_id( dev->dev ) == UCLASS_USB_HUB) {
> usb_show_tree_graph(udev, pre);
> pre[index] = 0;
> }
>
> (or put the condition directly before the for loop)
>
> Or can we have an UCLASS_USB_EMUL, UCLASS_BLK or UCLASS_BOOTDEV as
> direct child of an UCLASS_USB_HUB ???
>
> Am I opening any cans of worms ?
>From my
Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command
On 6/20/23 09:03, Xavier Drudis Ferran wrote: El Tue, Jun 20, 2023 at 02:50:48AM +0200, Marek Vasut deia: On 6/13/23 08:52, Xavier Drudis Ferran wrote: U-Boot TPL 2023.07-rc2-00144-g497967f1ee (Jun 12 2023 - 11:15:47) Next is already at rc4 , what's this rc2 ? It's a test I did last week (June 12th). What do you think has changed in next between rc2 and rc4 so that this shouldn't happen now ? The beginning of the email states: " Ok. New test. This uses yesterday morning's next branch. commit 5b589e139620214f " So, that is not the case ? Also, commit 497967f1ee does not exist in upstream U-Boot, was this some patched U-Boot tree ? My last patch (sent yesterday) was tested with the next branch as of yesterday morning. But you can test it yourself with any version you like. Do I have to send the logs of each test I do to the list every time ? Ideally the claims you make about what you tested and logs from the test should match, that would be a good starting point. I thought Simon Glass reply last week to the mail you quoted already showed agreement that the issue exists, and the adding of the UCLASS_BOOTDEV device is per design, so cmd/usb.c needs fixing to deal with this change. I am not disputing that. What bothers me is the still missing consistent test case, esp. if this is a fix which should be added this late in release.
Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command
El Tue, Jun 20, 2023 at 02:50:48AM +0200, Marek Vasut deia: > On 6/13/23 08:52, Xavier Drudis Ferran wrote: > > > > U-Boot TPL 2023.07-rc2-00144-g497967f1ee (Jun 12 2023 - 11:15:47) > > Next is already at rc4 , what's this rc2 ? It's a test I did last week (June 12th). What do you think has changed in next between rc2 and rc4 so that this shouldn't happen now ? My last patch (sent yesterday) was tested with the next branch as of yesterday morning. But you can test it yourself with any version you like. Do I have to send the logs of each test I do to the list every time ? I thought Simon Glass reply last week to the mail you quoted already showed agreement that the issue exists, and the adding of the UCLASS_BOOTDEV device is per design, so cmd/usb.c needs fixing to deal with this change.
Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command
On 6/13/23 08:52, Xavier Drudis Ferran wrote: Ok. New test. This uses yesterday morning's next branch. commit 5b589e139620214f Merge: cc5a940923 32d2461e04 Merge branch 'next_net/phy_connect_dev' USB2 does not work for rk3399 in next (fixes are in master, thanks), but USB3 is enough. I compiled for rock-pi-4-rk3399_defconfig flashed to a new microSD card as per doc/board/rockchip/rockchip.rst : dd if=u-boot-rockchip.bin of=/dev/sda seek=64 sync Put this microSD card in a Rock Pi 4 B+ Put a new USB stick in the USB3 port (center blue port closer to board). (the microSD card and USB stick come from factory, I guess they were partitioned with a single FAT partition) (make sure emmc and spi are blank) Connected only serial console and power. Got this: U-Boot TPL 2023.07-rc2-00144-g497967f1ee (Jun 12 2023 - 11:15:47) Next is already at rc4 , what's this rc2 ?
Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command
Thanks for explaining, Simon Glass.
Can someone please stop me if I'm splitting hairs or bikeshedding or
something ? I guess we agree both checking for null or UCLASS_BOOTDEV
would work right now but we are looking for what's more future-proof.
El Tue, Jun 13, 2023 at 09:12:30PM +0100, Simon Glass deia:
> Hi Xavier,
>
> On Tue, 13 Jun 2023 at 17:04, Xavier Drudis Ferran wrote:
> >
> > El Tue, Jun 13, 2023 at 03:58:22PM +0100, Simon Glass deia:
> > >
> > > Yes that's right. So 'usb info' should ignore UCLASS_BOOTDEV devices.
> >
> > That's a possibility, yes. It should work until someone adds another
> > device there for some other purpose.
> >
> > > That is better than checking for the NULL pointer.
> > >
> >
> > Why? What's wrong about checking for null ?
> > Or maybe checking for both not null and not UCLASS_BOOTDEV ?
> >
> > Not that I care too much, just to understand your reasoning.
>
> Well, devices may have something attached there, so it may be
> non-NULL, but point to a different struct from the expected one.
>
Yes. That is possible. How likely ?
That "there" is dev_get_parent_priv(). If I understand the driver
model, those devices shouldn't put things there themselves, it should
be their parent who puts stuff there for them. And the parent should
be an usb_device->dev (because of the recursive code). So what other
struct would such a parent put in a child parent_priv_ ? Yes, whatever
it wants, but I mean, isn't it likely to assume the child would be
"adopted" with null as parent_priv_ or a "natural child" with a usb_device
in parent_priv_ ?
I did a very rough search
grep -rIE 'UCLASS_(BLK|BOOTDEV|USB_EMUL)' . |grep -F .id |cut -f1 -d: |xargs
-n 1 grep -l per_child_auto
./drivers/usb/emul/usb-emul-uclass.c
Which seems to suggest only UCLASS_USB_EMUL would have parent_priv_,
and that would be an usb_device, which the current code does not want
to recurse anyway. (dev_set_parent_priv() is almost never called).
It is also possible that one day a device that is not UCLASS_BLK,
UCLASS_BOOTDEV or UCLASS_USB_EMUL is put as children of a usb storage
device (just imagine a future system similar to bootstd for firmware
updates, trust material, etc.). Is it likely to have a struct in
parent_priv_ that is not a usb_device ?
So which is more likely to survive future changes ?
- checking for parent_priv_ not null and not UCLASS_USB_EMUL
- checking for parent_priv_ not null and not UCLASS_USB_EMUL and not UCLASS_BLK
(my patch, overcautious ?)
- checking for not (UCLASS_BLK, UCLASS_BOOTDEV or UCLASS_USB_EMUL)
(Simon Glass' idea)
- checking for not UCLASS_BLK and not UCLASS_BOOTDEV and not UCLASS_USB_EMUL
and parent_priv_ not null
> >
> > Or can we check for recursible devices somehow ?
> > Maybe flag them or something ?
> >
> > Why is better to state all devices are recursible except some UCLASSes
> > (meaning they can have stuff that needs listed in usb info - or
> > likewise usb tree -) instead of stating that some closed set are
> > recursible ?
> >
>
> For USB we can have lots of different types of devices as children, so
> it would be a pain to enumerate them all. At least that's how I see
> it.
>
We can have lots of devices as children, but those do get listed
before the recursive call. How many of those can have children that
we have to list too, i.e. how many of those do we want to recurse
into ?
I can only think of usb hubs (maybe some node for multifunction
devices too ???). Maybe I'm not understanding how U-Boot works with
USB devices... but it looks like it would be enough to look for
UCLASS_USB_HUB, then recursive call, else no recursion. I mean instead of
cmd/usb.c : usb_show_tree_graph() :
if ((device_get_uclass_id( child ) != UCLASS_USB_EMUL) &&
(device_get_uclass_id( child ) != UCLASS_BOOTDEV) &&
(device_get_uclass_id( child ) != UCLASS_BLK)) {
usb_show_tree_graph(udev, pre);
pre[index] = 0;
}
we could simply have
if (device_get_uclass_id( dev->dev ) == UCLASS_USB_HUB) {
usb_show_tree_graph(udev, pre);
pre[index] = 0;
}
(or put the condition directly before the for loop)
Or can we have an UCLASS_USB_EMUL, UCLASS_BLK or UCLASS_BOOTDEV as
direct child of an UCLASS_USB_HUB ???
Am I opening any cans of worms ?
Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command
Hi Xavier, On Tue, 13 Jun 2023 at 17:04, Xavier Drudis Ferran wrote: > > El Tue, Jun 13, 2023 at 03:58:22PM +0100, Simon Glass deia: > > > > Yes that's right. So 'usb info' should ignore UCLASS_BOOTDEV devices. > > That's a possibility, yes. It should work until someone adds another > device there for some other purpose. > > > That is better than checking for the NULL pointer. > > > > Why? What's wrong about checking for null ? > Or maybe checking for both not null and not UCLASS_BOOTDEV ? > > Not that I care too much, just to understand your reasoning. Well, devices may have something attached there, so it may be non-NULL, but point to a different struct from the expected one. > > Or can we check for recursible devices somehow ? > Maybe flag them or something ? > > Why is better to state all devices are recursible except some UCLASSes > (meaning they can have stuff that needs listed in usb info - or > likewise usb tree -) instead of stating that some closed set are > recursible ? > For USB we can have lots of different types of devices as children, so it would be a pain to enumerate them all. At least that's how I see it. Regards, Simon
Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command
El Tue, Jun 13, 2023 at 03:58:22PM +0100, Simon Glass deia: > > Yes that's right. So 'usb info' should ignore UCLASS_BOOTDEV devices. That's a possibility, yes. It should work until someone adds another device there for some other purpose. > That is better than checking for the NULL pointer. > Why? What's wrong about checking for null ? Or maybe checking for both not null and not UCLASS_BOOTDEV ? Not that I care too much, just to understand your reasoning. Or can we check for recursible devices somehow ? Maybe flag them or something ? Why is better to state all devices are recursible except some UCLASSes (meaning they can have stuff that needs listed in usb info - or likewise usb tree -) instead of stating that some closed set are recursible ?
Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command
Hi Xavier,
On Tue, 13 Jun 2023 at 07:52, Xavier Drudis Ferran wrote:
>
> Ok. New test.
>
> This uses yesterday morning's next branch.
> commit 5b589e139620214f
> Merge: cc5a940923 32d2461e04
> Merge branch 'next_net/phy_connect_dev'
>
> USB2 does not work for rk3399 in next (fixes are in master, thanks),
> but USB3 is enough.
>
> I compiled for rock-pi-4-rk3399_defconfig
>
> flashed to a new microSD card as per doc/board/rockchip/rockchip.rst :
>
>dd if=u-boot-rockchip.bin of=/dev/sda seek=64
>sync
>
> Put this microSD card in a Rock Pi 4 B+
> Put a new USB stick in the USB3 port (center blue port closer to board).
>
> (the microSD card and USB stick come from factory, I guess they were
> partitioned with a single FAT partition)
>
> (make sure emmc and spi are blank)
>
> Connected only serial console and power.
>
> Got this:
>
> U-Boot TPL 2023.07-rc2-00144-g497967f1ee (Jun 12 2023 - 11:15:47)
> lpddr4_set_rate: change freq to 400MHz 0, 1
> Channel 0: LPDDR4, 400MHz
> BW=32 Col=10 Bk=8 CS0 Row=15 CS1 Row=15 CS=2 Die BW=16 Size=2048MB
> Channel 1: LPDDR4, 400MHz
> BW=32 Col=10 Bk=8 CS0 Row=15 CS1 Row=15 CS=2 Die BW=16 Size=2048MB
> 256B stride
> lpddr4_set_rate: change freq to 800MHz 1, 0
> Trying to boot from BOOTROM
> Returning to boot ROM...
>
> U-Boot SPL 2023.07-rc2-00144-g497967f1ee (Jun 12 2023 - 11:15:47 +0200)
> Trying to boot from MMC1
> NOTICE: BL31: v2.1(release):v2.1-728-ged01e0c4-dirty
> NOTICE: BL31: Built : 18:29:11, Mar 22 2022
>
>
> U-Boot 2023.07-rc2-00144-g497967f1ee (Jun 12 2023 - 11:15:47 +0200)
>
> SoC: Rockchip rk3399
> Reset cause: POR
> Model: Radxa ROCK Pi 4B
> DRAM: 4 GiB (effective 3.9 GiB)
> PMIC: RK808
> Core: 283 devices, 29 uclasses, devicetree: separate
> MMC: mmc@fe31: 2, mmc@fe32: 1, mmc@fe33: 0
> Loading Environment from MMC... *** Warning - bad CRC, using default
> environment
>
> In:serial
> Out: serial
> Err: serial
> Model: Radxa ROCK Pi 4B
> Net: eth0: ethernet@fe30
> Hit any key to stop autoboot: 2 1 0
> rockchip_pcie pcie@f800: PCIe link training gen1 timeout!
> Bus usb@fe38: ehci_generic usb@fe38: Failed to get clocks (ret=-19)
> Port not available.
> Bus usb@fe3c: ehci_generic usb@fe3c: Failed to get clocks (ret=-19)
> Port not available.
> Bus usb@fe80: Register 2000140 NbrPorts 2
> Starting the controller
> USB XHCI 1.10
> Bus usb@fe90: Register 2000140 NbrPorts 2
> Starting the controller
> USB XHCI 1.10
> scanning bus usb@fe80 for devices... 1 USB Device(s) found
> scanning bus usb@fe90 for devices... cannot reset port 1!?
> 2 USB Device(s) found
> rockchip_pcie pcie@f800: failed to find ep-gpios property
> ethernet@fe30 Waiting for PHY auto negotiation to complete.
> TIMEOUT !
> Could not initialize PHY ethernet@fe30
> rockchip_pcie pcie@f800: failed to find ep-gpios property
> ethernet@fe30 Waiting for PHY auto negotiation to complete.
> TIMEOUT !
> Could not initialize PHY ethernet@fe30
> => printenv preboot
> ## Error: "preboot" not defined
> => printenv
> arch=arm
> baudrate=150
> board=evb_rk3399
> board_name=evb_rk3399
> boot_targets=mmc1 mmc0 nvme scsi usb pxe dhcp spi
>
> bootcmd=bootflow scan
>
> bootdelay=2
> cpu=armv8
> cpuid#=[something]
> eth1addr=[:so:me:th:in:g]
> ethact=ethernet@fe30
> ethaddr=[:so:me:th:in:g]
> fdt_addr_r=0x01f0
> fdtcontroladdr=f1ef9170
> fdtfile=rockchip/rk3399-rock-pi-4b.dtb
> fdtoverlay_addr_r=0x0200
> kernel_addr_r=0x0208
> kernel_comp_addr_r=0x0800
> kernel_comp_size=0x200
> loadaddr=0x800800
> partitions=uuid_disk=${uuid_gpt_disk};name=loader1,start=32K,size=4000K,uuid=${uuid_gpt_loader1};name=loader2,start=8MB,size=4MB,uuid=${uuid_gpt_loader2};name=trust,size=4M,uuid=${uuid_gpt_atf};name=boot,size=112M,bootable,uuid=${uuid_gpt_boot};name=rootfs,size=-,uuid=[something];
> pxefile_addr_r=0x0060
> ramdisk_addr_r=0x0600
> script_offset_f=0xffe000
> script_size_f=0x2000
> scriptaddr=0x0050
> serial#=[something]
> soc=rk3399
> stderr=serial,vidconsole
> stdin=serial,usbkbd
> stdout=serial,vidconsole
> vendor=rockchip
>
> Environment size: 1041/32764 bytes
> => usb info
> 1: Hub, USB Revision 3.0
> - U-Boot XHCI Host Controller
> - Class: Hub
> - PacketSize: 512 Configurations: 1
> - Vendor: 0x Product 0x Version 1.0
>Configuration: 1
>- Interfaces: 1 Self Powered 0mA
> Interface: 0
> - Alternate Setting 0, Endpoints: 1
> - Class Hub
> - Endpoint 1 In Interrupt MaxPacket 8 Interval 255ms
>
> 1: Hub, USB Revision 3.0
> - U-Boot XHCI Host Controller
> - Class: Hub
> - PacketSize: 512 Configurations: 1
> - Vendor: 0x Product 0x Version 1.0
>Configuration: 1
>- Interfaces: 1 Self Powered 0mA
> Interface: 0
> - Alternate Setting 0, Endpoints: 1
> - Class Hub
> - Endpoint 1 In Interrupt MaxPacket 8 Interval 255ms
>
> 2: Mass Storage, USB Revision 3.20
> - USB SanDisk
Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command
Ok. New test.
This uses yesterday morning's next branch.
commit 5b589e139620214f
Merge: cc5a940923 32d2461e04
Merge branch 'next_net/phy_connect_dev'
USB2 does not work for rk3399 in next (fixes are in master, thanks),
but USB3 is enough.
I compiled for rock-pi-4-rk3399_defconfig
flashed to a new microSD card as per doc/board/rockchip/rockchip.rst :
dd if=u-boot-rockchip.bin of=/dev/sda seek=64
sync
Put this microSD card in a Rock Pi 4 B+
Put a new USB stick in the USB3 port (center blue port closer to board).
(the microSD card and USB stick come from factory, I guess they were
partitioned with a single FAT partition)
(make sure emmc and spi are blank)
Connected only serial console and power.
Got this:
U-Boot TPL 2023.07-rc2-00144-g497967f1ee (Jun 12 2023 - 11:15:47)
lpddr4_set_rate: change freq to 400MHz 0, 1
Channel 0: LPDDR4, 400MHz
BW=32 Col=10 Bk=8 CS0 Row=15 CS1 Row=15 CS=2 Die BW=16 Size=2048MB
Channel 1: LPDDR4, 400MHz
BW=32 Col=10 Bk=8 CS0 Row=15 CS1 Row=15 CS=2 Die BW=16 Size=2048MB
256B stride
lpddr4_set_rate: change freq to 800MHz 1, 0
Trying to boot from BOOTROM
Returning to boot ROM...
U-Boot SPL 2023.07-rc2-00144-g497967f1ee (Jun 12 2023 - 11:15:47 +0200)
Trying to boot from MMC1
NOTICE: BL31: v2.1(release):v2.1-728-ged01e0c4-dirty
NOTICE: BL31: Built : 18:29:11, Mar 22 2022
U-Boot 2023.07-rc2-00144-g497967f1ee (Jun 12 2023 - 11:15:47 +0200)
SoC: Rockchip rk3399
Reset cause: POR
Model: Radxa ROCK Pi 4B
DRAM: 4 GiB (effective 3.9 GiB)
PMIC: RK808
Core: 283 devices, 29 uclasses, devicetree: separate
MMC: mmc@fe31: 2, mmc@fe32: 1, mmc@fe33: 0
Loading Environment from MMC... *** Warning - bad CRC, using default environment
In:serial
Out: serial
Err: serial
Model: Radxa ROCK Pi 4B
Net: eth0: ethernet@fe30
Hit any key to stop autoboot: 2 1 0
rockchip_pcie pcie@f800: PCIe link training gen1 timeout!
Bus usb@fe38: ehci_generic usb@fe38: Failed to get clocks (ret=-19)
Port not available.
Bus usb@fe3c: ehci_generic usb@fe3c: Failed to get clocks (ret=-19)
Port not available.
Bus usb@fe80: Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.10
Bus usb@fe90: Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.10
scanning bus usb@fe80 for devices... 1 USB Device(s) found
scanning bus usb@fe90 for devices... cannot reset port 1!?
2 USB Device(s) found
rockchip_pcie pcie@f800: failed to find ep-gpios property
ethernet@fe30 Waiting for PHY auto negotiation to complete. TIMEOUT
!
Could not initialize PHY ethernet@fe30
rockchip_pcie pcie@f800: failed to find ep-gpios property
ethernet@fe30 Waiting for PHY auto negotiation to complete. TIMEOUT
!
Could not initialize PHY ethernet@fe30
=> printenv preboot
## Error: "preboot" not defined
=> printenv
arch=arm
baudrate=150
board=evb_rk3399
board_name=evb_rk3399
boot_targets=mmc1 mmc0 nvme scsi usb pxe dhcp spi
bootcmd=bootflow scan
bootdelay=2
cpu=armv8
cpuid#=[something]
eth1addr=[:so:me:th:in:g]
ethact=ethernet@fe30
ethaddr=[:so:me:th:in:g]
fdt_addr_r=0x01f0
fdtcontroladdr=f1ef9170
fdtfile=rockchip/rk3399-rock-pi-4b.dtb
fdtoverlay_addr_r=0x0200
kernel_addr_r=0x0208
kernel_comp_addr_r=0x0800
kernel_comp_size=0x200
loadaddr=0x800800
partitions=uuid_disk=${uuid_gpt_disk};name=loader1,start=32K,size=4000K,uuid=${uuid_gpt_loader1};name=loader2,start=8MB,size=4MB,uuid=${uuid_gpt_loader2};name=trust,size=4M,uuid=${uuid_gpt_atf};name=boot,size=112M,bootable,uuid=${uuid_gpt_boot};name=rootfs,size=-,uuid=[something];
pxefile_addr_r=0x0060
ramdisk_addr_r=0x0600
script_offset_f=0xffe000
script_size_f=0x2000
scriptaddr=0x0050
serial#=[something]
soc=rk3399
stderr=serial,vidconsole
stdin=serial,usbkbd
stdout=serial,vidconsole
vendor=rockchip
Environment size: 1041/32764 bytes
=> usb info
1: Hub, USB Revision 3.0
- U-Boot XHCI Host Controller
- Class: Hub
- PacketSize: 512 Configurations: 1
- Vendor: 0x Product 0x Version 1.0
Configuration: 1
- Interfaces: 1 Self Powered 0mA
Interface: 0
- Alternate Setting 0, Endpoints: 1
- Class Hub
- Endpoint 1 In Interrupt MaxPacket 8 Interval 255ms
1: Hub, USB Revision 3.0
- U-Boot XHCI Host Controller
- Class: Hub
- PacketSize: 512 Configurations: 1
- Vendor: 0x Product 0x Version 1.0
Configuration: 1
- Interfaces: 1 Self Powered 0mA
Interface: 0
- Alternate Setting 0, Endpoints: 1
- Class Hub
- Endpoint 1 In Interrupt MaxPacket 8 Interval 255ms
2: Mass Storage, USB Revision 3.20
- USB SanDisk 3.2Gen1 05017d2e4d7b4ea0c5822c90c51e0b7
- Class: (from Interface) Mass Storage
- PacketSize: 512 Configurations: 1
- Vendor: 0x0781 Product 0x5591 Version 1.0
Configuration: 1
- Interfaces: 1 Bus Powered 224mA
Interface: 0
- Alternate Setting 0, Endpoints: 2
- Class Mass Storage, Transp. SCSI, Bulk only
- Endpoint 1 In
Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command
El Mon, Jun 12, 2023 at 10:17:38PM +0100, Simon Glass deia: > > I'm not sure what is going on here. Which version are you testing? Do > you have these two commits? > > 8c29b73278d6 bootstd: usb: Avoid initing USB twice > 9fea3a799dde usb: Tidy up the usb_start flag > > Regards, > Simon Yes, I have both. I'm testing the next branch. I'll send shortly a reply to Marek Vasut with a cleaner test I did yesterday. Doesn't look hard to reproduce for me... I'm still trying to understand when the UCLASS_BOOTDEV device should be added under an usb mass storage device, and when (if ever) removed.
Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command
Hi, On Sun, 11 Jun 2023 at 13:29, Marek Vasut wrote: > > 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@fe30 > >>> Hit any key to stop autoboot: 2 1 0 > >>> rockchip_pcie pcie@f800: PCIe link training gen1 timeout! > >>> Bus usb@fe38: USB EHCI 1.00 > >>> Bus usb@fe3c: USB EHCI 1.00 > >>> Bus usb@fe80: Register 2000140 NbrPorts 2 > >>> Starting the controller > >>> USB XHCI 1.10 > >>> Bus usb@fe90: Register 2000140 NbrPorts 2 > >>> Starting the controller > >>> USB XHCI 1.10 > >>> scanning bus usb@fe38 for devices... 1 USB Device(s) found > >>> scanning bus usb@fe3c for devices... 2 USB Device(s) found > >>> scanning bus usb@fe80 for devices... 1 USB Device(s) found > >>> scanning bus usb@fe90 for devices... 1 USB Device(s) found > >>> rockchip_pcie pcie@f800: failed to find ep-gpios property > >>> ethernet@fe30 Waiting for PHY auto negotiation to complete. > >>> TIMEOUT ! > >>> Could not initialize PHY ethernet@fe30 > >>> rockchip_pcie pcie@f800: failed to find ep-gpios property > >>> ethernet@fe30 Waiting for PHY auto negotiation to complete. > >>> TIMEOUT ! > >>> Could not initialize PHY ethernet@fe30 > >> > >> 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 0x9610, far 0x948 > >>> elr: 002157d4 lr : 002157d4 (reloc) > >>> elr: f3f4f7d4 lr : f3f4f7d4 > >> > >> 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
Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command
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@fe30 Hit any key to stop autoboot: 2 1 0 rockchip_pcie pcie@f800: PCIe link training gen1 timeout! Bus usb@fe38: USB EHCI 1.00 Bus usb@fe3c: USB EHCI 1.00 Bus usb@fe80: Register 2000140 NbrPorts 2 Starting the controller USB XHCI 1.10 Bus usb@fe90: Register 2000140 NbrPorts 2 Starting the controller USB XHCI 1.10 scanning bus usb@fe38 for devices... 1 USB Device(s) found scanning bus usb@fe3c for devices... 2 USB Device(s) found scanning bus usb@fe80 for devices... 1 USB Device(s) found scanning bus usb@fe90 for devices... 1 USB Device(s) found rockchip_pcie pcie@f800: failed to find ep-gpios property ethernet@fe30 Waiting for PHY auto negotiation to complete. TIMEOUT ! Could not initialize PHY ethernet@fe30 rockchip_pcie pcie@f800: failed to find ep-gpios property ethernet@fe30 Waiting for PHY auto negotiation to complete. TIMEOUT ! Could not initialize PHY ethernet@fe30 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 0x9610, far 0x948 elr: 002157d4 lr : 002157d4 (reloc) elr: f3f4f7d4 lr : f3f4f7d4 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
Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command
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@fe30 > > Hit any key to stop autoboot: 2 1 0 > > rockchip_pcie pcie@f800: PCIe link training gen1 timeout! > > Bus usb@fe38: USB EHCI 1.00 > > Bus usb@fe3c: USB EHCI 1.00 > > Bus usb@fe80: Register 2000140 NbrPorts 2 > > Starting the controller > > USB XHCI 1.10 > > Bus usb@fe90: Register 2000140 NbrPorts 2 > > Starting the controller > > USB XHCI 1.10 > > scanning bus usb@fe38 for devices... 1 USB Device(s) found > > scanning bus usb@fe3c for devices... 2 USB Device(s) found > > scanning bus usb@fe80 for devices... 1 USB Device(s) found > > scanning bus usb@fe90 for devices... 1 USB Device(s) found > > rockchip_pcie pcie@f800: failed to find ep-gpios property > > ethernet@fe30 Waiting for PHY auto negotiation to complete. > > TIMEOUT ! > > Could not initialize PHY ethernet@fe30 > > rockchip_pcie pcie@f800: failed to find ep-gpios property > > ethernet@fe30 Waiting for PHY auto negotiation to complete. > > TIMEOUT ! > > Could not initialize PHY ethernet@fe30 > > 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... > > => 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 0x9610, far 0x948 > > elr: 002157d4 lr : 002157d4 (reloc) > > elr: f3f4f7d4 lr : f3f4f7d4 > > 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
