usb: core: Improve device lifecycle debuggability

[PATCH 1/2] usb: core: Centralize device state update logic

Posted by Kuen-Han Tsai 2 months, 1 week ago

Introduce a new static inline function, update_usb_device_state(), to
centralize the process of changing a device's state, including the
management of active_duration during suspend/resume transitions.

This change prepares for adding tracepoints, allowing tracing logic to
be added in a single, central location within the new helper function.

Signed-off-by: Kuen-Han Tsai <khtsai@google.com>
---
 drivers/usb/core/hub.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 256fe8c86828d51c33442345acdb7f3fe80a98ce..ce3d94c960470e9be7979b1021551eab5fd03517 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2147,6 +2147,20 @@ static void update_port_device_state(struct usb_device *udev)
 	}
 }
 
+static inline void update_usb_device_state(struct usb_device *udev,
+					   enum usb_device_state new_state)
+{
+	if (udev->state == USB_STATE_SUSPENDED &&
+	    new_state != USB_STATE_SUSPENDED)
+		udev->active_duration -= jiffies;
+	else if (new_state == USB_STATE_SUSPENDED &&
+		 udev->state != USB_STATE_SUSPENDED)
+		udev->active_duration += jiffies;
+
+	udev->state = new_state;
+	update_port_device_state(udev);
+}
+
 static void recursively_mark_NOTATTACHED(struct usb_device *udev)
 {
 	struct usb_hub *hub = usb_hub_to_struct_hub(udev);
@@ -2156,10 +2170,7 @@ static void recursively_mark_NOTATTACHED(struct usb_device *udev)
 		if (hub->ports[i]->child)
 			recursively_mark_NOTATTACHED(hub->ports[i]->child);
 	}
-	if (udev->state == USB_STATE_SUSPENDED)
-		udev->active_duration -= jiffies;
-	udev->state = USB_STATE_NOTATTACHED;
-	update_port_device_state(udev);
+	update_usb_device_state(udev, USB_STATE_NOTATTACHED);
 }
 
 /**
@@ -2209,14 +2220,7 @@ void usb_set_device_state(struct usb_device *udev,
 			else
 				wakeup = 0;
 		}
-		if (udev->state == USB_STATE_SUSPENDED &&
-			new_state != USB_STATE_SUSPENDED)
-			udev->active_duration -= jiffies;
-		else if (new_state == USB_STATE_SUSPENDED &&
-				udev->state != USB_STATE_SUSPENDED)
-			udev->active_duration += jiffies;
-		udev->state = new_state;
-		update_port_device_state(udev);
+		update_usb_device_state(udev, new_state);
 	} else
 		recursively_mark_NOTATTACHED(udev);
 	spin_unlock_irqrestore(&device_state_lock, flags);

-- 
2.51.0.740.g6adb054d12-goog

Re: [PATCH 1/2] usb: core: Centralize device state update logic

Posted by Alan Stern 2 months, 1 week ago

On Mon, Oct 13, 2025 at 10:01:22AM +0800, Kuen-Han Tsai wrote:
> Introduce a new static inline function, update_usb_device_state(), to
> centralize the process of changing a device's state, including the
> management of active_duration during suspend/resume transitions.
> 
> This change prepares for adding tracepoints, allowing tracing logic to
> be added in a single, central location within the new helper function.
> 
> Signed-off-by: Kuen-Han Tsai <khtsai@google.com>
> ---
>  drivers/usb/core/hub.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 256fe8c86828d51c33442345acdb7f3fe80a98ce..ce3d94c960470e9be7979b1021551eab5fd03517 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2147,6 +2147,20 @@ static void update_port_device_state(struct usb_device *udev)
>  	}
>  }
>  
> +static inline void update_usb_device_state(struct usb_device *udev,
> +					   enum usb_device_state new_state)
> +{
> +	if (udev->state == USB_STATE_SUSPENDED &&
> +	    new_state != USB_STATE_SUSPENDED)
> +		udev->active_duration -= jiffies;
> +	else if (new_state == USB_STATE_SUSPENDED &&
> +		 udev->state != USB_STATE_SUSPENDED)
> +		udev->active_duration += jiffies;
> +
> +	udev->state = new_state;
> +	update_port_device_state(udev);
> +}

This seems complicated enough to be a standalone function, not inline.

Alan Stern

Re: [PATCH 1/2] usb: core: Centralize device state update logic

Posted by Kuen-Han Tsai 2 months ago

Hi Alan,

On Mon, Oct 13, 2025 at 9:16 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Mon, Oct 13, 2025 at 10:01:22AM +0800, Kuen-Han Tsai wrote:
> > Introduce a new static inline function, update_usb_device_state(), to
> > centralize the process of changing a device's state, including the
> > management of active_duration during suspend/resume transitions.
> >
> > This change prepares for adding tracepoints, allowing tracing logic to
> > be added in a single, central location within the new helper function.
> >
> > Signed-off-by: Kuen-Han Tsai <khtsai@google.com>
> > ---
> >  drivers/usb/core/hub.c | 28 ++++++++++++++++------------
> >  1 file changed, 16 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 256fe8c86828d51c33442345acdb7f3fe80a98ce..ce3d94c960470e9be7979b1021551eab5fd03517 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -2147,6 +2147,20 @@ static void update_port_device_state(struct usb_device *udev)
> >       }
> >  }
> >
> > +static inline void update_usb_device_state(struct usb_device *udev,
> > +                                        enum usb_device_state new_state)
> > +{
> > +     if (udev->state == USB_STATE_SUSPENDED &&
> > +         new_state != USB_STATE_SUSPENDED)
> > +             udev->active_duration -= jiffies;
> > +     else if (new_state == USB_STATE_SUSPENDED &&
> > +              udev->state != USB_STATE_SUSPENDED)
> > +             udev->active_duration += jiffies;
> > +
> > +     udev->state = new_state;
> > +     update_port_device_state(udev);
> > +}
>
> This seems complicated enough to be a standalone function, not inline.
>

Thanks for the suggestion. I will change it into a static void function.

Regards,
Kuen-Han