On Mon, Jun 05, 2023 at 09:55:28PM +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. > > Reported-by: kernel test robot <oliver.sang@xxxxxxxxx> > Closes: https://lore.kernel.org/oe-lkp/202306042228.e532af6e-oliver.sang@xxxxxxxxx > Signed-off-by: Roy Luo <royluo@xxxxxxxxxx> > --- > This patch comes directly from RFC v2 after being reviewed by Alan Stern > Link: https://lore.kernel.org/all/20230531010134.1092942-1-royluo@xxxxxxxxxx/ > More discussion about implementation options is in RFC v1 > Link: https://lore.kernel.org/all/20230525173818.219633-1-royluo@xxxxxxxxxx/ > > Changes since v1: > * Address Alan Stern's comment: remove redundant NULL initializers in > update_port_device_state(). > > Changes since v2: > * Fix "BUG: sleeping function called from invalid context" caught by > kernel test robot. Move sleeping function sysfs_get_dirent to port > initiailzation and keep the kernfs_node for future reference. > (Reviewed-by tag is reset as this patch involves more code changes) > --- > Documentation/ABI/testing/sysfs-bus-usb | 9 +++++++ > drivers/usb/core/hub.c | 15 ++++++++++++ > drivers/usb/core/hub.h | 4 ++++ > drivers/usb/core/port.c | 32 +++++++++++++++++++++---- > 4 files changed, 56 insertions(+), 4 deletions(-) > > 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 It's June :) > +Contact: Roy Luo <royluo@xxxxxxxxxx> > +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'. No mentioning that you can poll on this file? You went through a lot of work to add that feature, right? Or am I mistaking why you added the sysfs_notify_dirent() calls? Are they really not needed? Or are they needed? Actually why are they needed? This is a state change, can't you emit uevents saying the state changed and if userspace wants to re-read it, it will? Or is that too noisy? Or is any of this needed at all, and you just want to read the file every once in a while from userspace to see what is going on? Do you have a pointer to any userspace code that is using this new interface? > + > What: /sys/bus/usb/devices/.../power/usb2_lpm_l1_timeout > Date: May 2013 > Contact: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index 97a0f8faea6e..a739403a9e45 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -2018,6 +2018,19 @@ 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_hub *hub; > + struct usb_port *port_dev; > + > + 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); > + sysfs_notify_dirent(port_dev->state_kn); > + } > +} > + > static void recursively_mark_NOTATTACHED(struct usb_device *udev) > { > struct usb_hub *hub = usb_hub_to_struct_hub(udev); > @@ -2030,6 +2043,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 +2100,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..37897afd1b64 100644 > --- a/drivers/usb/core/hub.h > +++ b/drivers/usb/core/hub.h > @@ -84,6 +84,8 @@ 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 > + * @state_kn: kernfs_node of the sysfs attribute that accesses @state > * @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 +102,8 @@ 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; > + struct kernfs_node *state_kn; > 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..2f906e89054e 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)); I thought checkpatch would warn you that you should be using sysfs_emit() here, wonder why it didn't. thanks, greg k-h
