Re: [PATCH v2 07/16] usb: hub: Power on connected M.2 E-key connectors

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Jun 10, 2026 at 04:40:41PM +0800, Chen-Yu Tsai wrote:
> 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.
> 
> This feature currently only supports a directly connected (no mux in
> between) M.2 E-key connector. Existing USB connector types are not
> covered. The USB A connector was recently added to the onboard devices
> driver. USB B connectors have historically been managed by the USB
> gadget or dual-role device controller drivers. USB C connectors are
> handled by TCPM drivers.
> 
> The power sequencing API does not know whether a power sequence provider
> is not needed or not available yet, so we only request it for connectors
> that we know need it, which at this time is just the E-key connector.
> 
> On the USB side, the port firmware node (if present) is tied to the
> usb_port device. This device is used to acquire the power sequencing
> descriptor. This allows the provider to tell the different ports on one
> hub apart.
> 
> This feature is not implemented in the onboard USB devices driver. The
> power sequencing API expects the consumer device to make the request,
> but there is no device node to instantiate a platform device to tie
> the driver to. The connector is not a child node of the USB host or
> hub, and the graph connection is from a USB port to the connector.
> And the connector itself already has a driver.
> 
> Power sequencing is not directly enabled in the connector driver as
> that would completely decouple the timing of it from the USB subsystem.
> It would not be possible for the USB subsystem to toggle the power
> for a power cycle or to disable the port.
> 
> This change depends on another change to make the power sequencing
> framework bool instead of tristate. The USB core and hub driver are
> bool, so if the power sequencing framework is built as a module, the
> kernel will fail to link.

>  int usb_hub_set_port_power(struct usb_device *hdev, struct usb_hub *hub,
>  			   int port1, bool set)
>  {
> -	int ret;
> +	struct usb_port *pwrseq_port = hub->ports[port1 - 1];
> +	int ret = 0;

Don't touch ret here. It's easier to maintain when assignment is closer to it's
first user (because it's getting validated there).

> +	/* non-SuperSpeed USB port holds pwrseq descriptor reference. */
> +	if (hub->ports[port1 - 1]->is_superspeed && hub->ports[port1 - 1]->peer)
> +		pwrseq_port = hub->ports[port1 - 1]->peer;

	ret = 0;

> +	if (set && !pwrseq_port->pwrseq_on)
> +		ret = pwrseq_power_on(pwrseq_port->pwrseq);
> +	else if (!set && pwrseq_port->pwrseq_on)
> +		ret = pwrseq_power_off(pwrseq_port->pwrseq);
> +	if (ret)
> +		return ret;
>  
>  	if (set)
>  		ret = set_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
>  	else
>  		ret = usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
>  
> -	if (ret)
> +	if (ret) {
> +		if (set && !pwrseq_port->pwrseq_on)
> +			pwrseq_power_off(pwrseq_port->pwrseq);
> +		else if (!set && pwrseq_port->pwrseq_on)
> +			pwrseq_power_on(pwrseq_port->pwrseq);
>  		return ret;

Can we rather have a couple of helpers? It might be hard to follow all this.
In such a case you won't even need the ret assignment here.


> +	}
>  
> -	if (set)
> +	if (set) {
>  		set_bit(port1, hub->power_bits);
> -	else
> +		pwrseq_port->pwrseq_on = 1;
> +	} else {
>  		clear_bit(port1, hub->power_bits);
> +		pwrseq_port->pwrseq_on = 0;
> +	}

Just

	pwrseq_port->pwrseq_on = set; // or explicit comparison
	assign_bit(port1, hub->power_bits, pwrseq_port->pwrseq_on);

>  	return 0;
>  }

...

> +static bool port_pwrseq_is_supported(struct usb_port *port_dev)
> +{
> +	struct device *dev = &port_dev->dev;
> +	struct fwnode_handle *port = dev->fwnode;

+ blank line here, because for RAII we assume the C99 definitions inside
the code, so one can insert the code in between. Doing it before ep validation
may lead to interesting errors in the future.

> +	struct fwnode_handle *ep __free(fwnode_handle) =
> +			fwnode_graph_get_next_port_endpoint(port, NULL);
> +	if (!ep)
> +		return false;
> +
> +	struct fwnode_handle *remote __free(fwnode_handle) =
> +			fwnode_graph_get_remote_port_parent(ep);
> +	if (!remote)
> +		return false;
> +
> +	if (!fwnode_device_is_compatible(remote, "pcie-m2-e-connector")) {
> +		dev_dbg(dev, "remote endpoint %pfw is not a supported connector", remote);
> +		return false;
> +	}
> +
> +	return true;
> +}

...

> +	if (IS_ERR(port_dev->pwrseq)) {
> +		retval = PTR_ERR(port_dev->pwrseq);
> +		dev_err_probe(&port_dev->dev, retval,
> +			      "failed to get power sequencing descriptor\n");

		retval = dev_err_probe(PTR_ERR(...));

> +		goto err_put_kn;
> +	}

...

>  	retval = component_add(&port_dev->dev, &connector_ops);
>  	if (retval) {
>  		dev_warn(&port_dev->dev, "failed to add component\n");

dev_warn_probe() // however it's not in your patch and was before...

> -		goto err_put_kn;
> +		goto err_pwrseq_off;
>  	}

...

> +err_pwrseq_off:
> +	if (port_dev->pwrseq_on)
> +		pwrseq_power_off(port_dev->pwrseq);

Hmm... I would rather see pwrseq framework to provide something like
_is_powered_on().

	if (pwrseq_is_powered_on())
		_power_off();

...

> +	if (port_dev->pwrseq_on)
> +		pwrseq_power_off(port_dev->pwrseq);

Ditto.

And perhaps even _power_off_if_on() that combines the check and the call.

However it seems that is reference counted and this _power_off() calls won't
guarantee actual power off.

-- 
With Best Regards,
Andy Shevchenko