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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Missing cleanup of PCI pwrctrl devices on error paths in apple_pcie_init().
- [Medium] Using the `%pe` format specifier with an integer error code instead of an error pointer.

Pre-existing issues:
- [Critical] Use-After-Free and resource leak of port IRQ handlers and domains on probe failure.
- [High] MSI IRQ domain is permanently leaked on probe deferral, heavily exacerbated by the new pwrctrl deferred probing.
--

--- Patch [1]: [PATCH 1/2] PCI: apple: Integrate pwrctrl API ---
commit 937db91081c31bded4a8e5eaa8859cec8bb3fc57
Author: Yureka Lilian <yureka@cyberchaos.dev>
Subject: PCI: apple: Integrate pwrctrl API

This patch integrates the PCI pwrctrl framework into the Apple PCIe host
driver to provide standardized power management. It allows powering on
devices like WiFi and SD card readers using the pci-pwrctrl-generic
driver before probing the ports.

Link: https://lore.kernel.org/lkml/20220502093832.32778-4-marcan@marcan.st/

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

[ ... ]

> @@ -825,6 +826,21 @@ static int apple_pcie_init(struct pci_config_window *cfg)
>  	if (WARN_ON(!pcie))
>  		return -ENOENT;
>  
> +	ret = pci_pwrctrl_create_devices(pcie->dev);
> +	if (ret) {
> +		dev_err(pcie->dev, "Failed to create pwrctrl devices: %pe\n", ret);

[Severity: Medium]
Are we using the %pe format specifier with an integer error code instead
of a pointer here?

Passing the 32-bit integer ret directly might cause undefined behavior on
64-bit architectures, or print a hashed pointer address instead of the
actual error string since IS_ERR() evaluates to false.

Should this be cast with ERR_PTR(ret)?

> +		return ret;
> +	}
> +
> +	ret = pci_pwrctrl_power_on_devices(pcie->dev);
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER) {
> +			dev_err(pcie->dev, "Failed to power on devices: %pe\n", ret);

[Severity: Medium]
Is this %pe format specifier also receiving the integer ret directly
instead of a pointer?

> +			pci_pwrctrl_destroy_devices(pcie->dev);
> +		}
> +		return ret;

[Severity: High]
This is a pre-existing issue, but does returning -EPROBE_DEFER here
exacerbate a memory leak in apple_msi_init()?

Because pci_pwrctrl_power_on_devices() relies on of_platform_device_create()
spawning child devices that probe asynchronously, it seems this might
frequently return -EPROBE_DEFER.

Earlier in the probe path, apple_msi_init() creates an MSI domain via
msi_create_parent_irq_domain(), but lacks a devm_add_action() to clean
it up.

If we defer probe here, do we permanently leak the previous MSI domain and
pcie->bitmap allocations on each retry?

> +	}
> +
>  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
>  		ret = apple_pcie_setup_port(pcie, of_port);
>  		if (ret) {
>  			dev_err(dev, "Port %pOF setup fail: %d\n", of_port, ret);
>  			return ret;

[Severity: High]
If apple_pcie_setup_port() fails and we return ret here, do we bypass
tearing down the successfully created pwrctrl devices?

If pci_pwrctrl_create_devices() and pci_pwrctrl_power_on_devices()
succeeded earlier, it seems this error path leaves the hardware powered on
indefinitely and leaks the child platform devices since they are not
devres-managed.

Should we call pci_pwrctrl_power_off_devices() and
pci_pwrctrl_destroy_devices() before returning?

[Severity: Critical]
This isn't a bug introduced by this patch, but could this error path lead
to a use-after-free and resource leak of port IRQ handlers?

If the loop successfully sets up the first port but fails on a subsequent
port, the probe fails and devres frees the struct apple_pcie_port for
the successful port.

However, looking at apple_pcie_port_register_irqs(), it registers the
handler using request_irq() without devm:

drivers/pci/controller/pcie-apple.c:apple_pcie_port_register_irqs() {
    ...
    ret = request_irq(irq, apple_pcie_port_irq, 0, port_irqs[i].name, port);
    ...
}

Similarly, apple_pcie_port_setup_irq() uses
irq_set_chained_handler_and_data() without a devm teardown.

Since the IRQs remain active, could an incoming interrupt trigger the
handlers and dereference the freed port structure?

>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-apple-pcie-pwren-v1-0-5a281b182fe2@cyberchaos.dev?part=1