On Tue, 20 Jun 2023 at 12:20, Xavier Drudis Ferran <xdru...@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 <s...@chromium.org>
