Re: [RFC PATCH v2 1/4] pcie/aspm: Add debug logging for aspm policy config

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Dead code when attempting to log Clock PM state in ASPM debug output.
- [Low] Unconditional and potentially misleading debug logging for ASPM link configuration.
--

commit 364db9458a74bb3fb257b69effddd2f95f805170
Author: Thomas Falcon <thomas.falcon@intel.com>

pcie/aspm: Add debug logging for aspm policy config

Include additional logging for ASPM and Clock PM state changes
keeping track of code flow and enabled power saving states.

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 925373b98dff0..cd23c1462502f 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c

[ ... ]

> @@ -961,11 +964,14 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
>  	state &= (link->aspm_capable & ~link->aspm_disable);
>  
>  	/* Can't enable any substates if L1 is not enabled */
> -	if (!(state & PCIE_LINK_STATE_L1))
> +	if (!(state & PCIE_LINK_STATE_L1)) {
> +		pci_dbg(parent, "ASPM: L1 not enabled, disabling L1 substates\n");
>  		state &= ~PCIE_LINK_STATE_L1SS;
> +	}
>  
>  	/* Spec says both ports must be in D0 before enabling PCI PM substates*/
>  	if (parent->current_state != PCI_D0 || child->current_state != PCI_D0) {
> +		pci_dbg(parent, "ASPM: Both ports are not in D0, disable PCI PM L1 substates unless explicitly enabled\n");
>  		state &= ~PCIE_LINK_STATE_L1_SS_PCIPM;
>  		state |= (link->aspm_enabled & PCIE_LINK_STATE_L1_SS_PCIPM);
>  	}

Are these debug statements being printed more often than intended?

Since these prints happen unconditionally before the early return check:

    if (link->aspm_enabled == state)
        return;

enabling dynamic debugging will output these messages even if the device 
states already match the requested state and no hardware configuration is 
actually applied.

[ ... ]

> @@ -997,16 +1005,34 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)

[ ... ]

> +	pci_dbg(parent, "ASPM: enabled states:%s%s%s%s%s%s%s%s\n",
> +		FLAG(state, L0S_UP, " L0s-Upstream"),
> +		FLAG(state, L0S_DW, " L0s-Downstream"),
> +		FLAG(state, L1, " L1"),
> +		FLAG(state, L1_1, " ASPM-L1.1"),
> +		FLAG(state, L1_2, " ASPM-L1.2"),
> +		FLAG(state, L1_1_PCIPM, " PCI-PM-L1.1"),
> +		FLAG(state, L1_2_PCIPM, " PCI-PM-L1.2"),
> +		FLAG(state, CLKPM, " ClockPM"));

Is it possible for the CLKPM flag to ever be true here?

It looks like the PCIE_LINK_STATE_CLKPM bit is masked out earlier in the
function when verifying capabilities:

    state &= (link->aspm_capable & ~link->aspm_disable);

Since the CLKPM bit is outside the 7-bit range of aspm_capable, this bit
is never present in the state mask at this point, so the macro will always
evaluate to an empty string. 

Because the Clock PM state change is already correctly logged in 
pcie_set_clkpm(), does this FLAG check need to be included here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511210936.562622-1-thomas.falcon@intel.com?part=1