Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command

[Xavier Drudis Ferran <xdrudis@tinet.cat>]

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 <xdrudis@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
	   
> >
> > 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 ?