Message ID | 6132eecf-af52-38ac-9942-b13fab829bcf@web.de |
---|---|
State | Changes Requested |
Delegated to: | Simon Glass |
Headers | show |
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: > > 1. 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. > > 2. 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
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: >> >> 1. 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. > >> 2. 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
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);