* Re: [PATCH] PCI/pwrctrl: Do not try to power on/off devices that don't need pwrctrl
2026-04-21 10:41 [PATCH] PCI/pwrctrl: Do not try to power on/off devices that don't need pwrctrl Manivannan Sadhasivam
@ 2026-04-23 8:21 ` Bartosz Golaszewski
2026-04-24 16:15 ` Bjorn Helgaas
2026-04-27 17:37 ` Bjorn Helgaas
2 siblings, 0 replies; 4+ messages in thread
From: Bartosz Golaszewski @ 2026-04-23 8:21 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: linux-pci, linux-kernel, Krishna Chaitanya Chundru, brgl,
bhelgaas
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>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] PCI/pwrctrl: Do not try to power on/off devices that don't need pwrctrl
2026-04-21 10:41 [PATCH] PCI/pwrctrl: Do not try to power on/off devices that don't need pwrctrl Manivannan Sadhasivam
2026-04-23 8:21 ` Bartosz Golaszewski
2026-04-24 16:15 ` Bjorn Helgaas
@ 2026-04-27 17:37 ` Bjorn Helgaas
2 siblings, 0 replies; 4+ messages in thread
From: Bjorn Helgaas @ 2026-04-27 17:37 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: brgl, bhelgaas, linux-pci, linux-kernel,
Krishna Chaitanya Chundru
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread