Re: [PATCH V4 5/7] arm64: dts: imx8mq-evk: Move power supply properties to Root Port node

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] DT ABI breakage: Removing `vpcie-supply` and `vpcie3v3aux-supply` from the PCIe controller node breaks PCIe functionality on older kernels.

Pre-existing issues:
- [High] Resource and memory leak of regulators and `slot->supplies` array in `slot_pwrctrl_probe` error paths and driver teardown.
--

commit 331966289ddb574974fd80a80b3f1214ffc7055f
Author: Sherry Sun <sherry.sun@nxp.com>

arm64: dts: imx8mq-evk: Move power supply properties to Root Port node

This commit moves the vpcie-supply and vpcie3v3aux-supply properties from the
PCIe controller node to the Root Port child node. The change is intended to
support the new PCI pwrctrl framework.

> diff --git a/arch/arm64/boot/dts/freescale/imx8mq-evk.dts b/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
> index e7d87ea81b697..541f6d5e9cefb 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
> @@ -403,8 +403,6 @@ &pcie1 {
>  		 <&pcie0_refclk>,
>  		 <&clk IMX8MQ_CLK_PCIE2_PHY>,
>  		 <&clk IMX8MQ_CLK_PCIE2_AUX>;
> -	vpcie-supply = <&reg_pcie1>;
> -	vpcie3v3aux-supply = <&reg_pcie1>;

[Severity: Critical]
Does removing these properties break DT ABI compatibility for older kernels?

If an older kernel lacks the new PCI pwrctrl framework, its pci-imx6.c driver
will look for vpcie-supply and vpcie3v3aux-supply in the parent PCIe controller
node.

Without pci-pwrctrl to enable the child node's supplies, the regulators might
never be turned on, causing PCIe devices to fail probing.

>  	vph-supply = <&vgen5_reg>;
>  	supports-clkreq;
>  	status = "okay";
> @@ -422,6 +420,8 @@ &pcie1_ep {
>  
>  &pcie1_port0 {
>  	reset-gpios = <&gpio5 12 GPIO_ACTIVE_LOW>;
> +	vpcie3v3-supply = <&reg_pcie1>;
> +	vpcie3v3aux-supply = <&reg_pcie1>;
>  };

[ ... ]

[Severity: High]
This is a pre-existing issue, but while reviewing the PCI pwrctrl framework
that this DT change supports, I noticed a potential resource leak in the
probing path.

In drivers/pci/pwrctrl/generic.c:slot_pwrctrl_probe():

        slot->clk = devm_clk_get_optional(dev, NULL);
        if (IS_ERR(slot->clk))
                return dev_err_probe(dev, PTR_ERR(slot->clk),
                                     "Failed to enable slot clock\n");

If devm_clk_get_optional() fails (e.g., returns -EPROBE_DEFER), does this
return path leak the slot->supplies array allocated by
of_regulator_bulk_get_all(), as well as the regulator references?

Since devm_add_action_or_reset() has not yet been registered at this point, it
appears the cleanup is skipped entirely on this failure path.

Additionally, looking at the driver teardown path:

drivers/pci/pwrctrl/generic.c:devm_slot_pwrctrl_release() {
        struct slot_pwrctrl *slot = data;

        regulator_bulk_free(slot->num_supplies, slot->supplies);
}

Does regulator_bulk_free() actually free the dynamically allocated array
itself, or does it only drop the regulator references, leaving the
slot->supplies array memory permanently leaked on unbind?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630060710.3294811-1-sherry.sun@oss.nxp.com?part=5