Re: [PATCH] PCI/ASPM: Enable L0s/L1 for removable devices when BIOS didn't configure ASPM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The patch bypasses ACPI BIOS mechanisms that disable ASPM, forcibly enabling ASPM on hardware even when the BIOS explicitly denied OS control.
- [High] The logic forcefully enables ASPM on boot-connected removable devices where the BIOS may have intentionally disabled it for stability.
- [Medium] The `!link->aspm_enabled` condition is brittle and will silently fail to enable L1 for hotplugged devices if the parent port defaults to L0s.
- [Medium] The check creates a paradoxical state handling where partially enabled ASPM escapes the override, but fully disabled ASPM is overridden to full ASPM.
--

commit 6d92b2eb999d5bb255b9529c7fd1ff1a3798ce5b
Author: Mario Limonciello <mario.limonciello@amd.com>

PCI/ASPM: Enable L0s/L1 for removable devices when BIOS didn't configure ASPM

This commit enables ASPM L0s and L1 by default for removable devices like
Thunderbolt/USB4 eGPUs when the BIOS hasn't configured any ASPM states.
This aligns Linux behavior with Windows for hotplugged devices.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=221319

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 925373b98dff..77497d90be0b 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -804,8 +804,15 @@ static void pcie_aspm_override_default_link_state(struct pcie_link_state *link)
>  	struct pci_dev *pdev = link->downstream;
>  	u32 override;
>  
> -	/* For devicetree platforms, enable L0s and L1 by default */
> -	if (of_have_populated_dt()) {
> +	/*
> +	 * For devicetree platforms, enable L0s and L1 by default.
> +	 *
> +	 * For removable devices (e.g., Thunderbolt/USB4), enable L0s and L1
> +	 * by default if BIOS didn't configure any ASPM states. This handles
> +	 * hotplugged devices where firmware may not have configured ASPM.
> +	 */
> +	if (of_have_populated_dt() ||
> +	    (dev_is_removable(&pdev->dev) && !link->aspm_enabled)) {

Does this bypass the ACPI BIOS mechanisms that disable ASPM? When the BIOS
denies ASPM control via ACPI FADT NO_ASPM or _OSC, the kernel sets
aspm_disabled = true and aspm_policy = POLICY_DEFAULT. In
pcie_aspm_sanity_check(), devices are skipped if aspm_disabled is true.

But since this modifies link->aspm_default to L0s|L1, and
pcie_aspm_init_link_state() calls pcie_config_aspm_path() for
POLICY_DEFAULT, pcie_config_aspm_link() will compute the target state as
L0s|L1. Because this differs from link->aspm_enabled == 0, the kernel will
actively write to the PCI registers to enable ASPM, violating the
OS-firmware ACPI contract.

Will this condition silently fail to enable L1 for hotplugged devices if the
parent port defaults to L0s? link->aspm_enabled is an aggregate bitmask
populated in pcie_aspm_cap_init() by reading both the upstream and
downstream link control registers.

If the parent port has PCI_EXP_LNKCTL_ASPM_L0S set, the bitmask will include
PCIE_LINK_STATE_L0S_DW and be non-zero. The strict boolean negation
!link->aspm_enabled would then evaluate to false, skipping the override
entirely.

Does this forcefully enable ASPM on boot-connected removable devices where
the BIOS intentionally disabled it for stability? dev_is_removable()
evaluates to true for Thunderbolt/USB4 devices whether they were hotplugged
or present at boot.

If the BIOS intentionally disabled ASPM for a boot-connected device due to
hardware errata, link->aspm_enabled would be 0. This check evaluates to
true, overriding the BIOS's intentional disablement.

Does this create a paradoxical policy where hardware deemed partially stable
escapes the override, but hardware deemed completely unstable receives the
most aggressive ASPM configuration?

If the BIOS evaluated a device and enabled only L0s, link->aspm_enabled is
non-zero, the check is false, and L1 remains disabled. However, if the BIOS
disabled both L0s and L1, the bitmask is 0, the check evaluates to true,
and both L0s and L1 are forcefully enabled.

>  		if (link->aspm_support & PCIE_LINK_STATE_L0S)
>  			link->aspm_default |= PCIE_LINK_STATE_L0S;
>  		if (link->aspm_support & PCIE_LINK_STATE_L1)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260504225246.480921-1-mario.limonciello@amd.com?part=1