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

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

[Manivannan Sadhasivam]

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

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

[Bartosz Golaszewski]

On Tue, 21 Apr 2026 12:41:01 +0200, Manivannan Sadhasivam
<manivannan.sadhasivam@oss.qualcomm.com> said:
> 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>
> ---

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

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

[Bjorn Helgaas]

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.

Is there a user-visible symptom when the pwrctrl core tries to power
on/off a device when it shouldn't?

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

[Bjorn Helgaas]

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
>