| Message ID | 20230525173818.219633-1-royluo@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v1] usb: core: add sysfs entry for usb device state | expand |
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@rowland.harvard.edu> 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@google.com> > > > --- > > > > > 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
On Thu, May 25, 2023 at 12:10 PM Alan Stern <stern@rowland.harvard.edu> 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@rowland.harvard.edu> 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@google.com> > > > > --- > > > > > > > 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.
On Thu, May 25, 2023 at 01:31:17PM -0700, Roy Luo wrote: > On Thu, May 25, 2023 at 12:10 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Thu, May 25, 2023 at 11:46:23AM -0700, Roy Luo wrote: > > > 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. Well, you could remove that assumption by adding a "child_is_registered" flag and explicitly checking it. Alan Stern
On Thu, May 25, 2023 at 5:50 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Thu, May 25, 2023 at 01:31:17PM -0700, Roy Luo wrote: > > On Thu, May 25, 2023 at 12:10 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > On Thu, May 25, 2023 at 11:46:23AM -0700, Roy Luo wrote: > > > > 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. > > Well, you could remove that assumption by adding a "child_is_registered" > flag and explicitly checking it. > > Alan Stern Agree that's doable, with the following overheads: 1. We can no longer safely access port_dev->child without checking "child_is_registered", and there are plenty of places in the usb core that touch port_dev->child. The implicit assumption could also hurt code maintainability. 2. In the worst case where enumeration keeps failing, the retry loop in hub_port_connect() would frequently hold device_state_lock in order to link/unlink the usb_device to port_dev->child. This would definitely make sense if more places need port_dev->child early. However, we only need port_dev->child->state at this point, so it does not seem like a good deal to me. On Fri, May 26, 2023 at 12:42 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, May 25, 2023 at 05:38:18PM +0000, Roy Luo wrote: > > + * @work: workqueue for sysfs_notify() > > Do you really need this? This should be possible to call in any context > as kernfs_notify() says that it is safe to do that, right? > Thanks for pointing this out! Yes, kernfs_notify() or sysfs_notify_dirent() should work, will take that into the next patch. > Also, what userspace code is now calling poll() on your new sysfs file? > > thanks, > > greg k-h We are looking at adding the code to the generic userspace components if not hardware abstraction layer in the userspace. Thanks, Roy
On Fri, May 26, 2023 at 11:34:44AM -0700, Roy Luo wrote: > On Thu, May 25, 2023 at 5:50 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Thu, May 25, 2023 at 01:31:17PM -0700, Roy Luo wrote: > > > 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. > > > > Well, you could remove that assumption by adding a "child_is_registered" > > flag and explicitly checking it. > > > > Alan Stern > > Agree that's doable, with the following overheads: > 1. We can no longer safely access port_dev->child without checking > "child_is_registered", and there are plenty of places in the usb core that > touch port_dev->child. The implicit assumption could also hurt code > maintainability. That doesn't sound like a big deal. Currently you can't safely access port_dev->child without checking whether it is non-NULL. You would just have to replace one check with another. The fact that plenty of places touch port_dev->child indicates someone must have given some thought to protection against racing accesses. Your modifications should be able to benefit from that thought. > 2. In the worst case where enumeration keeps failing, the retry loop in > hub_port_connect() would frequently hold device_state_lock in order > to link/unlink the usb_device to port_dev->child. > This would definitely make sense if more places need port_dev->child early. > However, we only need port_dev->child->state at this point, so it does not > seem like a good deal to me. Another alternative -- possibly a much simpler one -- is to replicate port_dev->child->state in port_dev->state, purely for use by the new sysfs routine. In other words, keep the actual value there instead of a pointer to some other location that might get deallocated at any time. Since presumably you don't care about precise synchronization (that is, it doesn't matter if the value shown in the sysfs file is a few tens of milliseconds out of date) you could do this without extra locking. Just use WRITE_ONCE() for updates and READ_ONCE() to see what the current value is. Alan Stern
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..69de87c6ae9d 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -2018,6 +2018,18 @@ bool usb_device_is_owned(struct usb_device *udev) return !!hub->ports[udev->portnum - 1]->port_owner; } +static void notify_port_device_state(struct usb_device *udev) +{ + struct usb_port *port_dev = NULL; + struct usb_hub *hub = NULL; + + if (udev->parent) { + hub = usb_hub_to_struct_hub(udev->parent); + port_dev = hub->ports[udev->portnum - 1]; + schedule_work(&port_dev->work); + } +} + static void recursively_mark_NOTATTACHED(struct usb_device *udev) { struct usb_hub *hub = usb_hub_to_struct_hub(udev); @@ -2030,6 +2042,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; + notify_port_device_state(udev); } /** @@ -2086,6 +2099,7 @@ void usb_set_device_state(struct usb_device *udev, udev->state != USB_STATE_SUSPENDED) udev->active_duration += jiffies; udev->state = new_state; + notify_port_device_state(udev); } else recursively_mark_NOTATTACHED(udev); spin_unlock_irqrestore(&device_state_lock, flags); @@ -2252,6 +2266,8 @@ void usb_disconnect(struct usb_device **pdev) */ if (!test_and_set_bit(port1, hub->child_usage_bits)) pm_runtime_get_sync(&port_dev->dev); + + port_dev->state = NULL; } usb_remove_ep_devs(&udev->ep0); @@ -5315,6 +5331,7 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus, goto done; } + port_dev->state = &udev->state; usb_set_device_state(udev, USB_STATE_POWERED); udev->bus_mA = hub->mA_per_port; udev->level = hdev->level + 1; @@ -5437,6 +5454,7 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus, mutex_unlock(hcd->address0_mutex); usb_unlock_port(port_dev); } + port_dev->state = NULL; usb_put_dev(udev); if ((status == -ENOTCONN) || (status == -ENOTSUPP)) break; 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 * @location: opaque representation of platform connector location * @status_lock: synchronize port_event() vs usb_port_{suspend|resume} + * @work: workqueue for sysfs_notify() * @portnum: port index num based one * @is_superspeed cache super-speed status * @usb3_lpm_u1_permit: whether USB3 U1 LPM is permitted. @@ -100,8 +102,10 @@ 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; + struct work_struct work; u32 over_current_count; u8 portnum; u32 quirks; @@ -114,6 +118,7 @@ struct usb_port { #define to_usb_port(_dev) \ container_of(_dev, struct usb_port, dev) +#define work_to_usb_port(w) (container_of((w), struct usb_port, work)) extern int usb_hub_create_port_device(struct usb_hub *hub, int port1); diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c index 06a8f1f84f6f..7f3430170115 100644 --- a/drivers/usb/core/port.c +++ b/drivers/usb/core/port.c @@ -160,6 +160,19 @@ 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 = USB_STATE_NOTATTACHED; + + if (port_dev->state) + state = *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 +272,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, @@ -666,6 +680,13 @@ static const struct component_ops connector_ops = { .unbind = connector_unbind, }; +static void usb_port_state_work(struct work_struct *work) +{ + struct usb_port *port_dev = work_to_usb_port(work); + + sysfs_notify(&port_dev->dev.kobj, NULL, "state"); +} + int usb_hub_create_port_device(struct usb_hub *hub, int port1) { struct usb_port *port_dev; @@ -699,6 +720,7 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1) dev_set_name(&port_dev->dev, "%s-port%d", dev_name(&hub->hdev->dev), port1); mutex_init(&port_dev->status_lock); + INIT_WORK(&port_dev->work, usb_port_state_work); retval = device_register(&port_dev->dev); if (retval) { put_device(&port_dev->dev); @@ -764,6 +786,7 @@ void usb_hub_remove_port_device(struct usb_hub *hub, int port1) peer = port_dev->peer; if (peer) unlink_peers(port_dev, peer); + flush_work(&port_dev->work); component_del(&port_dev->dev, &connector_ops); device_unregister(&port_dev->dev); }
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@google.com> --- Documentation/ABI/testing/sysfs-bus-usb | 9 +++++++++ drivers/usb/core/hub.c | 18 ++++++++++++++++++ drivers/usb/core/hub.h | 5 +++++ drivers/usb/core/port.c | 23 +++++++++++++++++++++++ 4 files changed, 55 insertions(+) base-commit: 933174ae28ba72ab8de5b35cb7c98fc211235096