Re: [PATCH v2 1/2] PCI: dwc: Force L2 link entry on shutdown/reboot without D3cold check

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch breaks Wake-on-LAN and PME from S4/S5 (hibernation/poweroff) by unconditionally powering off PCIe endpoints and disabling auxiliary power.
--

--- Patch [1]: [PATCH v2 1/2] PCI: dwc: Force L2 link entry on shutdown/reboot without D3cold check ---
commit abaf02ec8187ffca064828adacb9cade46a5b293
Author: Krishna Chaitanya Chundru <[email protected]>

PCI: dwc: Force L2 link entry on shutdown/reboot without D3cold check

This commit modifies dw_pcie_suspend_noirq() to force the PCIe link into L2
during system shutdown or reboot. It bypasses the pci_host_common_d3cold_possible()
check to prevent the link from staying up when endpoints fail to suspend.

> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
[ ... ]
> @@ -1222,16 +1223,29 @@ static int dw_pcie_pme_turn_off(struct dw_pcie *pci)
>  
>  int dw_pcie_suspend_noirq(struct dw_pcie *pci)
>  {
> -	bool pme_capable = false;
> +	bool shutdown = system_state == SYSTEM_HALT ||
> +			system_state == SYSTEM_POWER_OFF ||
> +			system_state == SYSTEM_RESTART;
> +	bool d3cold, pme_capable = false;
>  	int ret = 0;
>  	u32 val;
>  
>  	if (!dw_pcie_link_up(pci))
>  		goto stop_link;
>  
> -	if (!pci_host_common_d3cold_possible(pci->pp.bridge, &pme_capable))
> +	/*
> +	 * During reboot/halt/poweroff the link is going away regardless, so
> +	 * force L2 entry without checking whether endpoints have transitioned
> +	 * to D3hot -- there's no point walking the bus to find out.
> +	 */
> +	if (shutdown)
> +		goto d3cold;

[Severity: High]
When shutdown is true, the code jumps straight to the d3cold label, entirely
bypassing the pci_host_common_d3cold_possible() check. Because pme_capable
is initialized to false, doesn't this mean it will unconditionally remain
false during shutdown or poweroff?

If pme_capable remains false, later in dw_pcie_suspend_noirq() under the
stop_link label, we set:

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

Will this cause the host driver to explicitly cut auxiliary power to the
endpoints, breaking Wake-on-LAN and PME generation from S4 (hibernation)
or S5 (poweroff) for devices that were correctly configured for wakeup?

> +
> +	d3cold = pci_host_common_d3cold_possible(pci->pp.bridge, &pme_capable);
> +	if (!d3cold)
>  		return 0;
>  
> +d3cold:
>  	if (pci->pp.ops->pme_turn_off) {
>  		pci->pp.ops->pme_turn_off(&pci->pp);
>  	} else {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1