Re: [PATCH V5 1/7] arm: dts: imx6qdl-sabresd: Add power supply property to Root Port node

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing error check for pwrseq_power_on() in slot_pwrctrl_power_on() leads to potential kernel panic.
--

commit 4b51e513820a111cd03554e7f701a9ce808be27b
Author: Sherry Sun <sherry.sun@nxp.com>

arm: dts: imx6qdl-sabresd: Add power supply property to Root Port node

This commit adds the power supply property to the Root Port child node to
support the new PCI pwrctrl framework. The legacy power supply property is
retained for backward compatibility.

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/nxp/imx/imx6qdl-sabresd.dtsi
> index fe9046c03ddd6..60fe0f973668b 100644
> --- a/arch/arm/boot/dts/nxp/imx/imx6qdl-sabresd.dtsi
> +++ b/arch/arm/boot/dts/nxp/imx/imx6qdl-sabresd.dtsi
> @@ -756,12 +756,14 @@ &pcie {
>  	pinctrl-0 = <&pinctrl_pcie>;
>  	/* This property is deprecated, use reset-gpios from the Root Port node. */
>  	reset-gpio = <&gpio7 12 GPIO_ACTIVE_LOW>;
> +	/* This property is deprecated, use power supply from the Root Port node. */
>  	vpcie-supply = <&reg_pcie>;
>  	status = "okay";
>  };
>  
>  &pcie_port0 {
>  	reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
> +	vpcie3v3-supply = <&reg_pcie>;

[Severity: High]
This is a pre-existing issue, but does enabling the pwrctrl framework here
expose a regression where error checks are missing in the core code?

Looking at drivers/pci/pwrctrl/generic.c:slot_pwrctrl_power_on(), it drops
the error code from pwrseq_power_on() and unconditionally returns 0:

drivers/pci/pwrctrl/generic.c:slot_pwrctrl_power_on() {
        ...
        if (slot->pwrseq) {
                pwrseq_power_on(slot->pwrseq);
                return 0;
        }
        ...
}

If the power sequencer fails to power on the slot, the PCI subsystem is
falsely notified that the slot is ready. The PCI core will then enumerate
the bus and attempt to access the configuration space of the unpowered
device. 

On ARM/ARM64 platforms, couldn't accessing unpowered PCIe registers result
in a synchronous external abort (SError) and an immediate kernel panic?

>  };
[ ... ]

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