Hi Benhard,
On 22 June 2016 at 03:05, Bernhard Nortmann bernhard.nortmann@web.de wrote:
Starting with commit b19236fd1c1ef289bab9e243ee5b50d658fcac3f I am observing a breakage of the "usb info" command on my BananaPi (Allwinner A20, sun7i), while "usb tree" and dm commands ("dm tree", "dm uclass") are fine. See attached usb-info-breakage.log
Tracing back the error positon from pc and lr registers pointed me to the device_get_uclass_id() call within usb_device_info(), and suggested the problem is caused by trying to dereference a NULL struct udevice *hub.
Therefore I added a diagnostic printf and a safeguard to this routine:
diff --git a/cmd/usb.c b/cmd/usb.c index b83d323..a5e2463 100644 --- a/cmd/usb.c +++ b/cmd/usb.c @@ -610,6 +610,12 @@ static int usb_device_info(void) struct udevice *hub;
device_find_first_child(bus, &hub);
printf("bus %p, uclass %d, hub %p: ",bus, device_get_uclass_id(bus), hub);if (!hub) {printf("<no hub>\n");continue;} if (device_get_uclass_id(hub) == UCLASS_USB_HUB && device_active(hub)) { show_info(hub);And it became apparent that hub actually receives NULL values during the device enumeration. The safeguard prevented the "data abort" error and got "usb info" working again - see attached usb-info-fixed.log
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.
- 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:
if (strncmp(argv[1], "inf", 3) == 0) { if (argc == 2) { #ifdef CONFIG_DM_USB usb_device_info(); #else int d; for (d = 0; d < USB_MAX_DEVICE; d++) { udev = usb_get_dev_index(d); if (udev == NULL) break; usb_display_desc(udev); usb_display_config(udev); } #endif
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.
Regards, B. Nortmann
Regards, Simon