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

[Simon Glass]

On Tue, 20 Jun 2023 at 12:20, Xavier Drudis Ferran <xdrudis at tinet.cat> wrote:
>
> 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.

Reviewed-by: Simon Glass <sjg at chromium.org>