Hi Simon, thanks for replying!
Am 23.06.2016 um 15:12 schrieb Simon Glass:
Hi Benhard,
On 22 June 2016 at 03:05, Bernhard Nortmann bernhard.nortmann@web.de wrote:
[...]
I'm not sure why this particular problem didn't manifest earlier and only now became apparent with the change in SPL header size / CONFIG_SPL_TEXT_BASE. But it seems that accessing a NULL hub with device_get_uclass_id() is clearly wrong and should be avoided. Which brings me to two questions:
- Is getting a NULL hub value legit when iterating UCLASS_USB devices the way that usb_device_info() does? If yes, the code seems to be lacking protection against passing it to device_get_uclass_id().
Well if there are no child devices of a USB controller, then yes. Normally there is at least one (a USB hub). But this code is wrong - it should not assume that.
In fact. now, the new uclass_first_device_err() function is probably a better choice, since it returns an error if nothing is found.
uclass_first_device_err() won't help here. It's not the outer UCLASS_USB enumeration that is at fault - getting a NULL "bus" would simply terminate the loop. In fact this loop correctly processes all four 'top level' devices available on sun7i-a20:
uclass 60: usb - * usb@01c14000 @ 7af37148, seq 0, (req -1) - * usb@01c14400 @ 7af371b0, seq 1, (req -1) - * usb@01c1c000 @ 7af37218, seq 2, (req -1) - * usb@01c1c400 @ 7af37280, seq 3, (req -1)
The root cause of the usb_device_info() problem here is that two of these controllers (ohci0: usb@01c14400, ohci1: usb@01c1c400) remain dormant / 'invisible' to DM as long as no actual USB1-only peripheral is attached, and device_find_first_child(bus, &hub) will return a NULL hub subsequently. ("usb tree" doesn't show them either in this state.)
I have a suspicion that this might easily happen with other EHCI-OHCI controller combinations too.
- Why does usb_device_info() choose to enumerate hubs this way at all? If the routine is aiming at UCLASS_USB_HUB - which seems to be the purpose of the subsequent device_get_uclass_id(hub) test - and the device tree already provides this information (as suggested by the output of "dm uclass"), then why not enumerate UCLASS_USB_HUB directly?
It was probably trying to duplicate the operation of the old code: [...]
But I'm not sure that the ordering would change in any case, or even if it matters. Feel free to change it to enumerate USB_HUB.
Another explanation is that originally I was not sure if USB hubs should have their own uclass. With PCI bridges we don't do it that way. But I decided in the end to go ahead, and I think it has worked ouit. So perhaps the code was converted mid-stream.
Yes, I figured it might be something along those "historical" lines. But if I understand you correctly, then any "usb_hub" (uclass 62) child of UCLASS_USB should also show up when iterating UCLASS_USB_HUB with uclass_first_device() and uclass_next_device(). If that's guaranteed, my preferred solution would actually be to do away with the "bus" enumeration and replace it with a more straightforward "show_info(hub) over all the hubs" solution.
I'll submit a patch for that shortly.
Regards, B. Nortmann