Re: [PATCH] PCI/pwrctrl: Do not try to power on/off devices that don't need pwrctrl

Bjorn Helgaas
2026-04-27T17:37:50Z

On Tue, Apr 21, 2026 at 04:11:01PM +0530, Manivannan Sadhasivam wrote:
> pci_pwrctrl_is_required() is used to detect whether a device really
> needs the PCI pwrctrl support or not. It is currently used in
> pci_pwrctrl_create_device(), but not in pci_pwrctrl_power_{on/off}_device()
> APIs. This leads to pwrctrl core trying to power on/off the incompatible
> devices like USB hub downstream ports defined in DT.
>
> Hence, add this check to prevent pwrctrl core from poking at wrong
> devices. For this purpose, move the pci_pwrctrl_is_required() helper
> definition to the top.
>
> Fixes: b35cf3b6aa1e ("PCI/pwrctrl: Add APIs to power on/off pwrctrl devices")
> Reported-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
> drivers/pci/pwrctrl/core.c | 90 ++++++++++++++++++++------------------
> 1 file changed, 48 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> index 97cff5b8ca88..b5a0a14d316e 100644
> --- a/drivers/pci/pwrctrl/core.c
> +++ b/drivers/pci/pwrctrl/core.c
> @@ -139,6 +139,48 @@ int devm_pci_pwrctrl_device_set_ready(struct device *dev,
> }
> EXPORT_SYMBOL_GPL(devm_pci_pwrctrl_device_set_ready);
>
> +/*
> + * Check whether the pwrctrl device really needs to be created or not. The
> + * pwrctrl device will only be created if the node satisfies below requirements:
> + *
> + * 1. Presence of compatible property with "pci" prefix to match against the
> + * pwrctrl driver (AND)
> + * 2. At least one of the power supplies defined in the devicetree node of the
> + * device (OR) in the remote endpoint parent node to indicate pwrctrl
> + * requirement.
> + */
> +static bool pci_pwrctrl_is_required(struct device_node *np)
> +{
> + struct device_node *endpoint;
> + const char *compat;
> + int ret;
> +
> + ret = of_property_read_string(np, "compatible", &compat);
> + if (ret < 0)
> + return false;
> +
> + if (!strstarts(compat, "pci"))
> + return false;
> +
> + if (of_pci_supply_present(np))
> + return true;
> +
> + if (of_graph_is_present(np)) {
> + for_each_endpoint_of_node(np, endpoint) {
> + struct device_node *remote __free(device_node) =
> + of_graph_get_remote_port_parent(endpoint);
> + if (remote) {
> + if (of_pci_supply_present(remote)) {
> + of_node_put(endpoint);
> + return true;
> + }
> + }
> + }
> + }
> +
> + return false;
> +}
> +
> static int __pci_pwrctrl_power_off_device(struct device *dev)
> {
> struct pci_pwrctrl *pwrctrl = dev_get_drvdata(dev);
> @@ -157,6 +199,9 @@ static void pci_pwrctrl_power_off_device(struct device_node *np)
> for_each_available_child_of_node_scoped(np, child)
> pci_pwrctrl_power_off_device(child);
>
> + if (!pci_pwrctrl_is_required(np))
> + return;

Sashiko asked:

Should we also add this same check to pci_pwrctrl_destroy_device()?
Since pci_pwrctrl_destroy_device() iterates over all children and
blindly calls of_device_unregister(), could it erroneously
unregister incompatible devices (like USB hubs) created by another
subsystem if this check is missing?

which seems like a reasonable question. pci_pwrctrl_create_device()
only calls of_platform_device_create() if pci_pwrctrl_is_required().

I'm also curious about the fact that of_platform_device_create() and
of_platform_device_destroy() look like they should be a symmetric
pair, but we don't use of_platform_device_destroy() in pwrctrl:

pci_pwrctrl_create_device
if (pci_pwrctrl_is_required(np))
pdev = of_platform_device_create(np)

pci_pwrctrl_destroy_device
pdev = of_find_device_by_node(np)
of_device_unregister(pdev)
platform_device_put(pdev)
of_node_clear_flag(np, OF_POPULATED)

Another Sashiko question (although I don't think it's directly related
to *this* patch):

Also, looking at the rest of this function, it calls device_is_bound()
before executing the power off callback:

drivers/pci/pwrctrl/core.c:pci_pwrctrl_power_off_device() {
...
if (device_is_bound(&pdev->dev)) {
ret = __pci_pwrctrl_power_off_device(&pdev->dev);
...
}

Could this result in a use-after-free?

If a concurrent driver unbind event occurs between the device_is_bound()
check and the callback execution inside __pci_pwrctrl_power_off_device(),
the pwrctrl structure might be freed since the device lock is not held.
This same pattern appears in pci_pwrctrl_power_on_device().

And another (again unrelated to this patch):

Is there a resource leak if powering on a child device fails in the loop
just above this check?

drivers/pci/pwrctrl/core.c:pci_pwrctrl_power_on_device() {
for_each_available_child_of_node_scoped(np, child) {
ret = pci_pwrctrl_power_on_device(child);
if (ret)
return ret;
}
...
}

If an error or -EPROBE_DEFER occurs, the function returns immediately.
Does it need to iterate backwards and power off the children it
successfully powered on during the current loop?

While the top-level caller pci_pwrctrl_power_on_devices() rolls back
siblings, the successfully powered-on sub-children here appear to be left
stranded. On -EPROBE_DEFER, the host controller might retry probing later,
repeatedly incrementing clock and regulator enable counts without
disabling them.

If you do anything about these, note that I split the code move of
pci_pwrctrl_is_required() to its own patch to make the addition of the
calls more obvious.

> pdev = of_find_device_by_node(np);
> if (!pdev)
> return;
> @@ -213,6 +258,9 @@ static int pci_pwrctrl_power_on_device(struct device_node *np)
> return ret;
> }
>
> + if (!pci_pwrctrl_is_required(np))
> + return 0;
> +
> pdev = of_find_device_by_node(np);
> if (!pdev)
> return 0;
> @@ -268,48 +316,6 @@ int pci_pwrctrl_power_on_devices(struct device *parent)
> }
> EXPORT_SYMBOL_GPL(pci_pwrctrl_power_on_devices);
>
> -/*
> - * Check whether the pwrctrl device really needs to be created or not. The
> - * pwrctrl device will only be created if the node satisfies below requirements:
> - *
> - * 1. Presence of compatible property with "pci" prefix to match against the
> - * pwrctrl driver (AND)
> - * 2. At least one of the power supplies defined in the devicetree node of the
> - * device (OR) in the remote endpoint parent node to indicate pwrctrl
> - * requirement.
> - */
> -static bool pci_pwrctrl_is_required(struct device_node *np)
> -{
> - struct device_node *endpoint;
> - const char *compat;
> - int ret;
> -
> - ret = of_property_read_string(np, "compatible", &compat);
> - if (ret < 0)
> - return false;
> -
> - if (!strstarts(compat, "pci"))
> - return false;
> -
> - if (of_pci_supply_present(np))
> - return true;
> -
> - if (of_graph_is_present(np)) {
> - for_each_endpoint_of_node(np, endpoint) {
> - struct device_node *remote __free(device_node) =
> - of_graph_get_remote_port_parent(endpoint);
> - if (remote) {
> - if (of_pci_supply_present(remote)) {
> - of_node_put(endpoint);
> - return true;
> - }
> - }
> - }
> - }
> -
> - return false;
> -}
> -
> static int pci_pwrctrl_create_device(struct device_node *np,
> struct device *parent)
> {
> --
> 2.51.0
>