Hi Xavier, On Wed, 14 Jun 2023 at 09:40, Xavier Drudis Ferran <xdru...@tinet.cat> 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 <xdru...@tinet.cat> > > 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 memory, I think you can check for a USB hub instead, but I'm not completely sure. 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. Regards, Simon