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 <[email protected]>
Signed-off-by: Roy Luo <[email protected]>
---
This patch comes directly from RFC v2 after being reviewed by Alan Stern
Link: https://lore.kernel.org/all/[email protected]/
More discussion about implementation options is in RFC v1
Link: https://lore.kernel.org/all/[email protected]/
---
Documentation/ABI/testing/sysfs-bus-usb | 9 +++++++++
drivers/usb/core/hub.c | 19 +++++++++++++++++++
drivers/usb/core/hub.h | 2 ++
drivers/usb/core/port.c | 11 +++++++++++
4 files changed, 41 insertions(+)
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 <[email protected]>
+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 <[email protected]>
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,
base-commit: 933174ae28ba72ab8de5b35cb7c98fc211235096
--
2.41.0.rc0.172.g3f132b7071-goog
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 <[email protected]>
> Signed-off-by: Roy Luo <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> > Signed-off-by: Roy Luo <[email protected]>
> > ---
>
> > 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