[PATCH v3] usb: core: add sysfs entry for usb device state

[PATCH v3] usb: core: add sysfs entry for usb device state

[Roy Luo]

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@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202306042228.e532af6e-oliver.sang@intel.com
Signed-off-by: Roy Luo <royluo@google.com>
---
This patch comes directly from RFC v2 after being reviewed by Alan Stern
Link: https://lore.kernel.org/all/20230531010134.1092942-1-royluo@google.com/
More discussion about implementation options is in RFC v1
Link: https://lore.kernel.org/all/20230525173818.219633-1-royluo@google.com/

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
+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..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));
+}
+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,
@@ -705,19 +716,24 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
 		return retval;
 	}
 
+	port_dev->state_kn = sysfs_get_dirent(port_dev->dev.kobj.sd, "state");
+	if (!port_dev->state_kn) {
+		dev_err(&port_dev->dev, "failed to sysfs_get_dirent 'state'\n");
+		retval = -ENODEV;
+		goto err_unregister;
+	}
+
 	/* Set default policy of port-poweroff disabled. */
 	retval = dev_pm_qos_add_request(&port_dev->dev, port_dev->req,
 			DEV_PM_QOS_FLAGS, PM_QOS_FLAG_NO_POWER_OFF);
 	if (retval < 0) {
-		device_unregister(&port_dev->dev);
-		return retval;
+		goto err_put_kn;
 	}
 
 	retval = component_add(&port_dev->dev, &connector_ops);
 	if (retval) {
 		dev_warn(&port_dev->dev, "failed to add component\n");
-		device_unregister(&port_dev->dev);
-		return retval;
+		goto err_put_kn;
 	}
 
 	find_and_link_peer(hub, port1);
@@ -754,6 +770,13 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
 		port_dev->req = NULL;
 	}
 	return 0;
+
+err_put_kn:
+	sysfs_put(port_dev->state_kn);
+err_unregister:
+	device_unregister(&port_dev->dev);
+
+	return retval;
 }
 
 void usb_hub_remove_port_device(struct usb_hub *hub, int port1)
@@ -765,5 +788,6 @@ void usb_hub_remove_port_device(struct usb_hub *hub, int port1)
 	if (peer)
 		unlink_peers(port_dev, peer);
 	component_del(&port_dev->dev, &connector_ops);
+	sysfs_put(port_dev->state_kn);
 	device_unregister(&port_dev->dev);
 }

base-commit: 933174ae28ba72ab8de5b35cb7c98fc211235096
-- 
2.41.0.rc0.172.g3f132b7071-goog

Re: [PATCH v3] usb: core: add sysfs entry for usb device state

[Alan Stern]

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@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202306042228.e532af6e-oliver.sang@intel.com
> Signed-off-by: Roy Luo <royluo@google.com>
> ---
> This patch comes directly from RFC v2 after being reviewed by Alan Stern
> Link: https://lore.kernel.org/all/20230531010134.1092942-1-royluo@google.com/
> More discussion about implementation options is in RFC v1
> Link: https://lore.kernel.org/all/20230525173818.219633-1-royluo@google.com/
> 
> 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)

As far as I am concerned, you can put my Reviewed-by: tag back in.  But 
Greg should look at the new kernfs_node stuff; that's more his area 
than mine.

> ---
>  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
> +Contact:	Roy Luo <royluo@google.com>
> +Description:
> +		Indicates current state of the USB device attached to the port. Valid

"Valid" should be moved from the end of this line to the start of the 
next one, to avoid going past 76 columns.  Not a big deal.

Alan Stern

> +		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..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));
> +}
> +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,
> @@ -705,19 +716,24 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
>  		return retval;
>  	}
>  
> +	port_dev->state_kn = sysfs_get_dirent(port_dev->dev.kobj.sd, "state");
> +	if (!port_dev->state_kn) {
> +		dev_err(&port_dev->dev, "failed to sysfs_get_dirent 'state'\n");
> +		retval = -ENODEV;
> +		goto err_unregister;
> +	}
> +
>  	/* Set default policy of port-poweroff disabled. */
>  	retval = dev_pm_qos_add_request(&port_dev->dev, port_dev->req,
>  			DEV_PM_QOS_FLAGS, PM_QOS_FLAG_NO_POWER_OFF);
>  	if (retval < 0) {
> -		device_unregister(&port_dev->dev);
> -		return retval;
> +		goto err_put_kn;
>  	}
>  
>  	retval = component_add(&port_dev->dev, &connector_ops);
>  	if (retval) {
>  		dev_warn(&port_dev->dev, "failed to add component\n");
> -		device_unregister(&port_dev->dev);
> -		return retval;
> +		goto err_put_kn;
>  	}
>  
>  	find_and_link_peer(hub, port1);
> @@ -754,6 +770,13 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
>  		port_dev->req = NULL;
>  	}
>  	return 0;
> +
> +err_put_kn:
> +	sysfs_put(port_dev->state_kn);
> +err_unregister:
> +	device_unregister(&port_dev->dev);
> +
> +	return retval;
>  }
>  
>  void usb_hub_remove_port_device(struct usb_hub *hub, int port1)
> @@ -765,5 +788,6 @@ void usb_hub_remove_port_device(struct usb_hub *hub, int port1)
>  	if (peer)
>  		unlink_peers(port_dev, peer);
>  	component_del(&port_dev->dev, &connector_ops);
> +	sysfs_put(port_dev->state_kn);
>  	device_unregister(&port_dev->dev);
>  }
> 
> base-commit: 933174ae28ba72ab8de5b35cb7c98fc211235096
> -- 
> 2.41.0.rc0.172.g3f132b7071-goog
> 

Re: [PATCH v3] usb: core: add sysfs entry for usb device state

[Greg Kroah-Hartman]

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@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202306042228.e532af6e-oliver.sang@intel.com
> Signed-off-by: Roy Luo <royluo@google.com>
> ---
> This patch comes directly from RFC v2 after being reviewed by Alan Stern
> Link: https://lore.kernel.org/all/20230531010134.1092942-1-royluo@google.com/
> More discussion about implementation options is in RFC v1
> Link: https://lore.kernel.org/all/20230525173818.219633-1-royluo@google.com/
> 
> 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@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'.

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@linux.intel.com>
> 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

Re: [PATCH v3] usb: core: add sysfs entry for usb device state

[Roy Luo]

On Wed, Jun 7, 2023 at 12:06 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> 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@intel.com>
> > Closes: https://lore.kernel.org/oe-lkp/202306042228.e532af6e-oliver.sang@intel.com
> > Signed-off-by: Roy Luo <royluo@google.com>
> > ---
> > This patch comes directly from RFC v2 after being reviewed by Alan Stern
> > Link: https://lore.kernel.org/all/20230531010134.1092942-1-royluo@google.com/
> > More discussion about implementation options is in RFC v1
> > Link: https://lore.kernel.org/all/20230525173818.219633-1-royluo@google.com/
> >
> > 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 :)

Oh I literally lost track of time.. will correct it in the next version.

>
> > +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'.
>
> 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?
>

Yes, I intentionally make the file pollable because it would be more
useful that way. I can give you a concrete example: when there's a
signal integrity issue during enumeration, the usb device state would
likely be repeatedly switching between POWERED and DEFAULT
state as the usb core goes through different levels of retry loops.
User space programs can poll the sysfs attribute to detect the state
changes and eventually recognize the pattern, which isn't possible
without the attribute being pollable. Right now I don't have anything
available for public review as the userspace part is still in development
and is dependent on this kernel patch.
Using uevent should also work, but I would argue that having the interested
parties to poll the sysfs attribute might be more efficient.
I will explicitly state that the file is pollable in the description
in the next patch.

Thanks,
Roy




> > +
> >  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..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

Re: [PATCH v3] usb: core: add sysfs entry for usb device state

[Roy Luo]

> > @@ -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

I was using sprintf() instead of sysfs_emit() because I randomly referred
to one of the nearby attributes. Looks like there are still many attributes in
port.c that uses sprintf(), any reason why we didn't replace them?
If not, I'm happy to do a clean-up so that others don't make the same mistake
as I did :D

Regards,
Roy

Re: [PATCH v3] usb: core: add sysfs entry for usb device state

[Greg Kroah-Hartman]

On Wed, Jun 07, 2023 at 06:58:08PM -0700, Roy Luo wrote:
> > > @@ -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
> 
> I was using sprintf() instead of sysfs_emit() because I randomly referred
> to one of the nearby attributes. Looks like there are still many attributes in
> port.c that uses sprintf(), any reason why we didn't replace them?
> If not, I'm happy to do a clean-up so that others don't make the same mistake
> as I did :D

Doing whole-scale replacements across the kernel for stuff like this
isn't needed, just for new stuff, using the new apis is a good idea.

thanks,

greg k-h