| Message ID | 20230601230456.2234972-1-royluo@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] usb: core: add sysfs entry for usb device state | expand |
On Thu, Jun 01, 2023 at 11:04:56PM +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. > > Reviewed-by: Alan Stern <stern@rowland.harvard.edu> > Signed-off-by: Roy Luo <royluo@google.com> > --- > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index 97a0f8faea6e..35d94288726b 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -2018,6 +2018,23 @@ bool usb_device_is_owned(struct usb_device *udev) > return !!hub->ports[udev->portnum - 1]->port_owner; > } > > +static void update_port_device_state(struct usb_device *udev) > +{ > + struct usb_port *port_dev = NULL; > + struct usb_hub *hub = NULL; > + struct kernfs_node *state_node = NULL; > + > + if (udev->parent) { > + hub = usb_hub_to_struct_hub(udev->parent); > + port_dev = hub->ports[udev->portnum - 1]; > + WRITE_ONCE(port_dev->state, udev->state); > + state_node = sysfs_get_dirent(port_dev->dev.kobj.sd, "state"); > + if (state_node) { > + sysfs_notify_dirent(state_node); > + } > + } > +} I didn't notice the "= NULL" initializers before. You might want to remove them, since they are completely unnecessary. Alan Stern
On Thu, Jun 1, 2023 at 4:21 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Thu, Jun 01, 2023 at 11:04:56PM +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. > > > > Reviewed-by: Alan Stern <stern@rowland.harvard.edu> > > Signed-off-by: Roy Luo <royluo@google.com> > > --- > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > > index 97a0f8faea6e..35d94288726b 100644 > > --- a/drivers/usb/core/hub.c > > +++ b/drivers/usb/core/hub.c > > @@ -2018,6 +2018,23 @@ bool usb_device_is_owned(struct usb_device *udev) > > return !!hub->ports[udev->portnum - 1]->port_owner; > > } > > > > +static void update_port_device_state(struct usb_device *udev) > > +{ > > + struct usb_port *port_dev = NULL; > > + struct usb_hub *hub = NULL; > > + struct kernfs_node *state_node = NULL; > > + > > + if (udev->parent) { > > + hub = usb_hub_to_struct_hub(udev->parent); > > + port_dev = hub->ports[udev->portnum - 1]; > > + WRITE_ONCE(port_dev->state, udev->state); > > + state_node = sysfs_get_dirent(port_dev->dev.kobj.sd, "state"); > > + if (state_node) { > > + sysfs_notify_dirent(state_node); > > + } > > + } > > +} > > I didn't notice the "= NULL" initializers before. You might want to > remove them, since they are completely unnecessary. > > Alan Stern Ack, sending out v2. Thanks, Roy
diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb index cb172db41b34..155770f18f9c 100644 --- a/Documentation/ABI/testing/sysfs-bus-usb +++ b/Documentation/ABI/testing/sysfs-bus-usb @@ -292,6 +292,15 @@ Description: which is marked with early_stop has failed to initialize, it will ignore all future connections until this attribute is clear. +What: /sys/bus/usb/devices/.../<hub_interface>/port<X>/state +Date: May 2023 +Contact: Roy Luo <royluo@google.com> +Description: + Indicates current state of the USB device attached to the port. Valid + states are: 'not-attached', 'attached', 'powered', + 'reconnecting', 'unauthenticated', 'default', 'addressed', + 'configured', and 'suspended'. + What: /sys/bus/usb/devices/.../power/usb2_lpm_l1_timeout Date: May 2013 Contact: Mathias Nyman <mathias.nyman@linux.intel.com> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 97a0f8faea6e..35d94288726b 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -2018,6 +2018,23 @@ bool usb_device_is_owned(struct usb_device *udev) return !!hub->ports[udev->portnum - 1]->port_owner; } +static void update_port_device_state(struct usb_device *udev) +{ + struct usb_port *port_dev = NULL; + struct usb_hub *hub = NULL; + struct kernfs_node *state_node = NULL; + + if (udev->parent) { + hub = usb_hub_to_struct_hub(udev->parent); + port_dev = hub->ports[udev->portnum - 1]; + WRITE_ONCE(port_dev->state, udev->state); + state_node = sysfs_get_dirent(port_dev->dev.kobj.sd, "state"); + if (state_node) { + sysfs_notify_dirent(state_node); + } + } +} + static void recursively_mark_NOTATTACHED(struct usb_device *udev) { struct usb_hub *hub = usb_hub_to_struct_hub(udev); @@ -2030,6 +2047,7 @@ static void recursively_mark_NOTATTACHED(struct usb_device *udev) if (udev->state == USB_STATE_SUSPENDED) udev->active_duration -= jiffies; udev->state = USB_STATE_NOTATTACHED; + update_port_device_state(udev); } /** @@ -2086,6 +2104,7 @@ void usb_set_device_state(struct usb_device *udev, udev->state != USB_STATE_SUSPENDED) udev->active_duration += jiffies; udev->state = new_state; + update_port_device_state(udev); } else recursively_mark_NOTATTACHED(udev); spin_unlock_irqrestore(&device_state_lock, flags); diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h index e23833562e4f..cd13fe189726 100644 --- a/drivers/usb/core/hub.h +++ b/drivers/usb/core/hub.h @@ -84,6 +84,7 @@ 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 * @location: opaque representation of platform connector location * @status_lock: synchronize port_event() vs usb_port_{suspend|resume} * @portnum: port index num based one @@ -100,6 +101,7 @@ struct usb_port { struct usb_port *peer; struct dev_pm_qos_request *req; enum usb_port_connect_type connect_type; + enum usb_device_state state; usb_port_location_t location; struct mutex status_lock; u32 over_current_count; diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c index 06a8f1f84f6f..745f2a3aafba 100644 --- a/drivers/usb/core/port.c +++ b/drivers/usb/core/port.c @@ -160,6 +160,16 @@ static ssize_t connect_type_show(struct device *dev, } static DEVICE_ATTR_RO(connect_type); +static ssize_t state_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct usb_port *port_dev = to_usb_port(dev); + enum usb_device_state state = READ_ONCE(port_dev->state); + + return sprintf(buf, "%s\n", usb_state_string(state)); +} +static DEVICE_ATTR_RO(state); + static ssize_t over_current_count_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -259,6 +269,7 @@ static DEVICE_ATTR_RW(usb3_lpm_permit); static struct attribute *port_dev_attrs[] = { &dev_attr_connect_type.attr, + &dev_attr_state.attr, &dev_attr_location.attr, &dev_attr_quirks.attr, &dev_attr_over_current_count.attr,