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

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

El Tue, Jun 20, 2023 at 11:03:57AM +0100, Simon Glass deia:
> Hi Xavier,
>

Hi Simon,

> >
> > 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.
>

I don't think it's a separate issue. If parent_priv is present it
could be a usb_device (most likely) or not, but if it's null there's
no way the recursive call can succeed.

> 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.
>

But things keep appearing that have a UCLASS_USB* parent and no
parent_priv.
in 2017 Suneel Garapati already fixed the issue of UCLASS_BLK
being a child of a device. Now it's UCLASS_BOOTDEV, and tomorrow
may be something else.

The most direct check will miss future cases as the devices tend to
become more abstract instead of mapping one to one to physical stuff.

> 
> >From my memory, I think you can check for a USB hub instead, but I'm
> not completely sure.
>

On second thoughts I didn't find it so easy. There's the root hub,
UCLASS_USB, I think and a UCLASS_USB_EMUL may also be a hub, so I
don't know anymore how more elegant that could be, so I left it be.

> 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.
>

Makes sense. But I don't have any more time for that, sorry.

I think we'll have to leave it at this unless someone else has the time.

Bye.