Re: [PATCH v2 2/3] PCI: apple: Integrate pwrctrl API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] PCI: apple: unbalanced pwrctrl power and device cleanup in apple_pcie_init()
--

commit 3ba5f942a54bed93131c2e3157428ab3600ebf03
Author: Yureka Lilian <yureka@cyberchaos.dev>

PCI: apple: Integrate pwrctrl API

This commit integrates the PCI pwrctrl API into the Apple PCIe host
controller driver so child port devices are created and powered on before
setting up the ports in apple_pcie_init().

> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index c2cffc0659f44..2d32092e2122b 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c

[ ... ]

> @@ -825,6 +826,28 @@ static int apple_pcie_init(struct pci_config_window *cfg)
>  	if (WARN_ON(!pcie))
>  		return -ENOENT;
>  
> +	// Ensure all ports can be powered on before attempting initialization
> +	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> +		ret = pci_pwrctrl_create_device(of_port, pcie->dev);
> +		if (ret) {
> +			dev_err(pcie->dev, "Failed to create pwrctrl device: %pe\n", ERR_PTR(ret));
> +			return ret;
> +		}
> +
> +		ret = pci_pwrctrl_power_on_device(of_port);
> +		if (ret) {
> +			if (ret != -EPROBE_DEFER) {
> +				dev_err(pcie->dev,
> +					"Failed to power on device: %pe\n",
> +					ERR_PTR(ret));
> +				for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> +					pci_pwrctrl_destroy_device(of_port);
> +				}
> +			}
> +			return ret;
> +		}
> +	}
> +
>  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
>  		ret = apple_pcie_setup_port(pcie, of_port);
>  		if (ret) {

[Severity: Medium]
Can this loop in apple_pcie_init() leave pwrctrl devices powered on or leaked
when an error or probe deferral occurs on a multi-port controller?

If an earlier port succeeds in both pci_pwrctrl_create_device() and
pci_pwrctrl_power_on_device(), and a subsequent port fails in:

1. pci_pwrctrl_create_device(): apple_pcie_init() returns immediately without
   powering off or destroying the pwrctrl devices created and powered on in
   previous iterations.

2. pci_pwrctrl_power_on_device() with -EPROBE_DEFER:
   pci_pwrctrl_power_on_device() only unwinds children of the failing port
   node, leaving earlier sibling ports powered on. When deferred probing
   retries apple_pcie_init(), pci_pwrctrl_power_on_device() is called again
   on the earlier ports, enabling their regulators and clocks a second time
   and leaking enable reference counts on every retry.

3. pci_pwrctrl_power_on_device() with a non-deferral error: the error path
   calls pci_pwrctrl_destroy_device() without first calling
   pci_pwrctrl_power_off_device() on the ports that were already powered on,
   which unregisters active pwrctrl platform devices and frees regulators
   while they are still enabled. The inner
   for_each_available_child_of_node_scoped(dev->of_node, of_port) loop also
   redeclares of_port and shadows the outer loop variable (-Wshadow).

4. apple_pcie_setup_port() in the second loop: if port setup fails or returns
   -EPROBE_DEFER (for example from devm_fwnode_gpiod_get()), apple_pcie_init()
   returns without powering off the pwrctrl devices enabled in the first loop
   or destroying them on non-deferral errors.

Would it be cleaner to use the controller-level helpers
pci_pwrctrl_create_devices(), pci_pwrctrl_power_on_devices(),
pci_pwrctrl_power_off_devices(), and pci_pwrctrl_destroy_devices() with goto
error unwinding so all ports are balanced properly?

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