Re: [PATCH v2 1/3] PCI/pwrctrl: Export per-device APIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] PCI/pwrctrl: leak of child devices on error in pci_pwrctrl_create_device()
- [Low] PCI/pwrctrl: missing !CONFIG_PCI_PWRCTRL stubs and device_node declaration
- [Low] PCI/pwrctrl: invalid kernel-doc annotations and duplicated comment in core.c
- [Low] PCI/pwrctrl: commit message omits exported create and destroy device APIs
--

commit 99f4514cfa7c5abc8881197d44fe3f03f5d6e806
Author: Yureka Lilian <yureka@cyberchaos.dev>

PCI/pwrctrl: Export per-device APIs

This commit exports pci_pwrctrl_power_on_device(),
pci_pwrctrl_power_off_device(), pci_pwrctrl_create_device(), and
pci_pwrctrl_destroy_device() for per-port power control management in PCIe
host controller drivers.

>     PCI/pwrctrl: Export per-device APIs
>     
>     Export the `pci_pwrctrl_power_on_device()` and
>     `pci_pwrctrl_power_off_device()` functions to allow PCIe host drivers
>     to call manage power control on a per-port basis.
>     
>     Signed-off-by: Yureka Lilian <yureka@cyberchaos.dev>

[Severity: Low]
This isn't a bug, but the patch also exports pci_pwrctrl_create_device()
and pci_pwrctrl_destroy_device(). Should the commit message mention those
two functions as well?

> diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> index 0e2387cf46577..b59dbedf251bc 100644
> --- a/drivers/pci/pwrctrl/core.c
> +++ b/drivers/pci/pwrctrl/core.c
> @@ -191,7 +191,16 @@ static int __pci_pwrctrl_power_off_device(struct device *dev)
>  	return pwrctrl->power_off(pwrctrl);
>  }
>  
> -static void pci_pwrctrl_power_off_device(struct device_node *np)
> +/**
> + * pci_pwrctrl_power_off_device - Power off pwrctrl devices
> + *
> + * @device_node: PCI port or device to operate on
> + *
> + * Recursively traverse all pwrctrl devices for the devicetree hierarchy
> + * below the specified PCI port or device and power them off in a depth
> + * first manner.
> + */
> +void pci_pwrctrl_power_off_device(struct device_node *np)

[ ... ]

> @@ -245,11 +255,21 @@ static int __pci_pwrctrl_power_on_device(struct device *dev)
>  	return pwrctrl->power_on(pwrctrl);
>  }
>  
> -/*
> +/**
> + * pci_pwrctrl_power_on_device - Power on pwrctrl devices
> + *
> + * @device_node: PCI port or device to operate on
> + *
>   * Power on the devices in a depth first manner. Before powering on the device,
>   * make sure its driver is bound.
> + * Recursively traverse all pwrctrl devices for the devicetree hierarchy
> + * below the specified PCI port or device and power them on in a depth
> + * first manner.  Before powering on the device, make sure its driver is bound.
> + *
> + * Return: 0 on success, -EPROBE_DEFER if any pwrctrl driver is not bound, an
> + * appropriate error code otherwise.
>   */
> -static int pci_pwrctrl_power_on_device(struct device_node *np)
> +int pci_pwrctrl_power_on_device(struct device_node *np)

[Severity: Low]
In the kernel-doc comments for pci_pwrctrl_power_off_device(),
pci_pwrctrl_power_on_device(), pci_pwrctrl_create_device(), and
pci_pwrctrl_destroy_device(), the parameter is documented as @device_node
rather than @np, and pci_pwrctrl_create_device() omits @parent.

Also, in pci_pwrctrl_power_on_device(), the previous two-sentence comment
was left in place right above the new description that repeats the same
text. Could these kernel-doc blocks be cleaned up to avoid kernel-doc
warnings?

[ ... ]

> @@ -335,7 +356,17 @@ int pci_pwrctrl_power_on_devices(struct device *parent)
>  }
>  EXPORT_SYMBOL_GPL(pci_pwrctrl_power_on_devices);
>  
> -static int pci_pwrctrl_create_device(struct device_node *np,
> +/**
> + * pci_pwrctrl_create_device - Create pwrctrl devices
> + *
> + * @device_node: PCI port or device to operate on
> + *
> + * Recursively create pwrctrl devices for the devicetree hierarchy below
> + * the specified PCI port or device in a depth first manner.
> + *
> + * Return: 0 on success, negative error number on error.
> + */
> +int pci_pwrctrl_create_device(struct device_node *np,
>  				     struct device *parent)
>  {
>  	struct platform_device *pdev;
> @@ -368,6 +399,7 @@ static int pci_pwrctrl_create_device(struct device_node *np,
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(pci_pwrctrl_create_device);

[Severity: Medium]
Does pci_pwrctrl_create_device() leak already-created child platform
devices on failure?

Inside pci_pwrctrl_create_device():

	for_each_available_child_of_node_scoped(np, child) {
		ret = pci_pwrctrl_create_device(child, parent);
		if (ret)
			return ret;
	}
	...
	pdev = of_platform_device_create(np, NULL, parent);
	if (!pdev) {
		dev_err(parent, "Failed to create pwrctrl device for node: %s\n", np->name);
		return -EINVAL;
	}

When pci_pwrctrl_create_device() was only called from
pci_pwrctrl_create_devices(), the caller cleaned up partially created
devices on error by calling pci_pwrctrl_destroy_devices(parent).

Now that pci_pwrctrl_create_device() is exported for direct per-port calls,
if device creation fails for a later sibling child or for np itself, any
child platform devices already created during the call remain registered
with OF_POPULATED set. Should pci_pwrctrl_create_device() unwind and
destroy the already-created children on error, similar to the err_power_off
rollback in pci_pwrctrl_power_on_device()?

[ ... ]

> diff --git a/include/linux/pci-pwrctrl.h b/include/linux/pci-pwrctrl.h
> index 1192a2527521d..f27be6489b50f 100644
> --- a/include/linux/pci-pwrctrl.h
> +++ b/include/linux/pci-pwrctrl.h
> @@ -59,6 +59,10 @@ int pci_pwrctrl_create_devices(struct device *parent);
>  void pci_pwrctrl_destroy_devices(struct device *parent);
>  int pci_pwrctrl_power_on_devices(struct device *parent);
>  void pci_pwrctrl_power_off_devices(struct device *parent);
> +int pci_pwrctrl_create_device(struct device_node *np, struct device *parent);
> +void pci_pwrctrl_destroy_device(struct device_node *np);
> +int pci_pwrctrl_power_on_device(struct device_node *np);
> +void pci_pwrctrl_power_off_device(struct device_node *np);
>  #else
>  static inline int pci_pwrctrl_create_devices(struct device *parent) { return 0; }
>  static void pci_pwrctrl_destroy_devices(struct device *parent) { }

[Severity: Low]
Will drivers calling these new functions fail to build when
CONFIG_PCI_PWRCTRL is disabled?

The declarations are added under #if IS_ENABLED(CONFIG_PCI_PWRCTRL), but
there are no corresponding static inline stub definitions in the #else
block. In addition, include/linux/pci-pwrctrl.h forward-declares struct
device and struct device_link, but does not forward-declare struct
device_node, which can trigger compiler warnings if this header is included
before linux/of.h.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-apple-pcie-pwren-v2-0-216a4066965f@cyberchaos.dev?part=1