Re: [PATCH v8 10/16] usb: hub: Power on connected M.2 E-key connectors with power sequencing API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hub_is_port_power_switchable() evaluates a per-port property at the hub level, leading to brittle logic and unintended side-effects for ports without power sequencers.
--

commit 839f0d077bce3f935502df5060567449e3c570bf
Author: Chen-Yu Tsai <wenst@chromium.org>

    usb: hub: Power on connected M.2 E-key connectors with power sequencing API
    
    The new M.2 E-key connector can have a USB connection. For the USB device
    on this connector to work, its power must be enabled and the W_DISABLE2#
    signal deasserted. The connector driver handles this and provides a
    toggle over the power sequencing API.

    Link: https://lore.kernel.org/all/20260610084053.2059858-1-wenst@chromium.org/
    Link: https://lore.kernel.org/all/20260515090149.3169406-1-wenst@chromium.org/

> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
> index de524c6da9fc4..3f403a56e5f75 100644
> --- a/drivers/usb/core/hub.h
> +++ b/drivers/usb/core/hub.h
> @@ -103,7 +103,15 @@ static inline bool hub_is_port_power_switchable(struct usb_hub *hub)
>  	if (!hub)
>  		return false;
>  	hcs = hub->descriptor->wHubCharacteristics;
> -	return (le16_to_cpu(hcs) & HUB_CHAR_LPSM) < HUB_CHAR_NO_LPSM;
> +	if ((le16_to_cpu(hcs) & HUB_CHAR_LPSM) < HUB_CHAR_NO_LPSM)
> +		return true;
> +	/* check for controllable external power sequencers */
> +	for (unsigned int i = 0; i < hub->hdev->maxchild; i++) {
> +		if (hub->ports[i] && hub->ports[i]->pwrseq)
> +			return true;
> +	}
> +
> +	return false;
>  }

[Severity: Medium]
Does this loop incorrectly promote a per-port capability to a hub-wide
capability?

If any port on a hub has a pwrseq, hub_is_port_power_switchable() returns
true for the entire hub. Could this cause callers to erroneously try to apply
power switching logic to all ports on the hub, even those without a pwrseq,
just because a sibling port made the hub appear switchable?

For example, in drivers/usb/core/hub.c:hub_port_connect():

	if (hub_is_port_power_switchable(hub)
			&& !usb_port_is_power_on(port_dev, portstatus)
			&& !port_dev->port_owner)
		usb_hub_set_port_power(hdev, hub, port1, true);

Additionally, since hdev->maxchild is 0 during port creation, the loop
inside hub_is_port_power_switchable() does not execute at that time. Does
this make the hub-wide check redundant for its intended purpose in
usb_port_allow_power_off()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904084158.1341550-1-wenst@chromium.org?part=10