Re: [RFC PATCH] PCI: dwc: Power the controller down when hibernating

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pci: hibernation S4 WOL and thaw broken by short-circuited D3cold check
--

commit fefc3f1865b9c97f0642e5cbcd08bb6b0f27167f
Author: Birk Skyum <birk.skyum@pm.me>

PCI: dwc: Power the controller down when hibernating

This commit modifies dw_pcie_suspend_noirq() to power down the controller
during hibernation by checking pm_suspend_in_progress(). This ensures that a
hibernation image resumed after a power cycle does not crash when the kernel
reads uninitialized DBI registers.

> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 77aa79ef0ccf8..458e4c6e798b0 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
[ ... ]
> @@ -1231,7 +1232,14 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
>  	if (!dw_pcie_link_up(pci))
>  		goto stop_link;
>  
> -	if (!pci_host_common_d3cold_possible(pci->pp.bridge, &pme_capable))
> +	/*
> +	 * Leaving the controller powered is only safe if it keeps its state
> +	 * until resume. A hibernation image is resumed after a power cycle, by
> +	 * a kernel that may not have brought this controller up, so only a
> +	 * suspend may leave it powered.
> +	 */
> +	if (pm_suspend_in_progress() &&
> +	    !pci_host_common_d3cold_possible(pci->pp.bridge, &pme_capable))
>  		return 0;

[Severity: High]
Does this short-circuiting && operator cause issues during hibernation?

During hibernation phases (poweroff_noirq and freeze_noirq),
pm_suspend_in_progress() returns false. This causes the && operator to
short-circuit, skipping the call to pci_host_common_d3cold_possible().

Because of this, pme_capable remains at its default uninitialized value of
false. Later in dw_pcie_suspend_noirq(), pci->pp.skip_pwrctrl_off is assigned
the value of pme_capable:

stop_link:
	...
	pci->pp.skip_pwrctrl_off = pme_capable;
	dw_pcie_stop_link(pci);

Since pme_capable is unconditionally false, pci_pwrctrl_power_off_devices()
will force cutting auxiliary power. 

This completely breaks S4 Wake-on-LAN and wipes endpoint hardware state during
the freeze phase, violating the core PM contract that thaw operates on preserved
hardware state. Endpoints lacking D3cold support will crash or fail to resume
during the thaw phase.

>  
>  	if (pci->pp.ops->pme_turn_off) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179157746908.73485.2343672865542690797@pm.me?part=1