On Thu, May 25, 2023 at 12:10 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > > On Thu, May 25, 2023 at 11:46:23AM -0700, Roy Luo wrote: > > On Thu, May 25, 2023 at 11:02 AM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > > > > > > On Thu, May 25, 2023 at 05:38:18PM +0000, Roy Luo wrote: > > > > Expose usb device state to userland as the information is useful in > > > > detecting non-compliant setups and diagnosing enumeration failures. > > > > For example: > > > > - End-to-end signal integrity issues: the device would fail port reset > > > > repeatedly and thus be stuck in POWERED state. > > > > - Charge-only cables (missing D+/D- lines): the device would never enter > > > > POWERED state as the HC would not see any pullup. > > > > > > > > What's the status quo? > > > > We do have error logs such as "Cannot enable. Maybe the USB cable is bad?" > > > > to flag potential setup issues, but there's no good way to expose them to > > > > userspace. > > > > > > > > Why add a sysfs entry in struct usb_port instead of struct usb_device? > > > > The struct usb_device is not device_add() to the system until it's in > > > > ADDRESS state hence we would miss the first two states. The struct > > > > usb_port is a better place to keep the information because its life > > > > cycle is longer than the struct usb_device that is attached to the port. > > > > > > > > Signed-off-by: Roy Luo <royluo@xxxxxxxxxx> > > > > --- > > > > > > > diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h > > > > index e23833562e4f..110143568c77 100644 > > > > --- a/drivers/usb/core/hub.h > > > > +++ b/drivers/usb/core/hub.h > > > > @@ -84,8 +84,10 @@ struct usb_hub { > > > > * @peer: related usb2 and usb3 ports (share the same connector) > > > > * @req: default pm qos request for hubs without port power control > > > > * @connect_type: port's connect type > > > > + * @state: device state of the usb device attached to the port > > > > > > This member is essentially a duplicate of the .child member of the > > > usb_port structure. That is, it points to the .state member of the > > > child device instead of to the child device itself, but this is pretty > > > much the same thing. You could replace *(port_dev->state) with > > > port_dev->child->state. > > > > > Alan, thanks for the quick response! > > Yes, port_dev->state is indeed the same as port_dev->child->state. However, > > I still add port_dev->state because port_dev->child won't be assigned until > > the corresponding usb_device is in ADDRESS state. > > I wish I can assign get port_dev->child assigned earlier, but I think > > the current design - assign port_dev->child and device_add() after ADDRESS > > state - also makes sense because there are many ways that the enumeration > > could fail in the early stage. By adding port_dev->state, I can link > > usb_device->state to usb_port as soon as the usb_device is created to get > > around the limitation of port_dev->child. > > I would be very happy to hear other ideas. > > Is there any real reason not to set port_dev->child as soon as the > usb_device structure is created? If enumeration fails, the pointer can > be cleared. > > Alan Stern Currently the usb core assumes the usb_device that port_dev->child points to is enumerated and port_dev->child->dev is registered when port_dev->child is present. Setting port_dev->child early would break this fundamental assumption, hence I'm a bit reluctant to go this way.