Re: [PATCH v3 2/8] PCI/ASPM: Add pci_force_enable_link_state() API

[sashiko-bot@kernel.org]

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

New issues:
- [High] pci_force_enable_link_state() fails to override init-disabled ASPM states because it does not handle uninitialized aspm_capable masks.
- [High] pci_force_enable_link_state() fails to re-enable Clock PM (CLKPM) because it does not clear link->clkpm_disable.
- [High] pci_force_enable_link_state() fails with -EPERM when aspm_disabled is true, preventing driver state restoration.
- [Medium] The kernel doc for `pci_force_enable_link_state()` incorrectly advises callers to use `pcie_aspm_enabled()`, which returns a `bool` instead of a bitmask.

Pre-existing issues:
- [Critical] TOCTOU race causing Use-After-Free of `pcie_link_state` when device removal interleaves with `__pci_enable_link_state()`.
--

--- Patch [2]: [PATCH v3 2/8] PCI/ASPM: Add pci_force_enable_link_state() API ---
commit 2bf493d2bfb473abb29a075bfd6fe57330878665
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

PCI/ASPM: Add pci_force_enable_link_state() API

This patch introduces the pci_force_enable_link_state() API to allow
drivers to re-enable ASPM states that were previously disabled via
pci_disable_link_state() or disabled during initialization. It adds a force
parameter to the shared helper __pci_enable_link_state() to clear the
disabled states before enabling.

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
[ ... ]
> @@ -1506,7 +1506,8 @@ int pci_disable_link_state(struct pci_dev *pdev, int state)
>  }
>  EXPORT_SYMBOL(pci_disable_link_state);
>  
> -static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
> +static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked,
> +				   bool force)
>  {
>  	struct pcie_link_state *link = pcie_aspm_get_link(pdev);

[Severity: Critical]
This is a pre-existing issue, but does fetching the link pointer locklessly
here lead to a Use-After-Free if device removal interleaves?

If a driver fetches the pointer, then sleeps in pci_set_power_state():

__pci_enable_link_state() {
    struct pcie_link_state *link = pcie_aspm_get_link(pdev);
    ...
    if (state & PCIE_LINK_STATE_L1_SS_PCIPM) {
        pci_set_power_state(pdev, PCI_D0);
    ...
    mutex_lock(&aspm_lock);
    link->aspm_default = ...;
}

Could a concurrent hot-remove call pcie_aspm_exit_link_state(), freeing
the link structure before we acquire the aspm_lock and dereference it?

[Severity: High]
Will this function return -EPERM on systems where BIOS ACPI _OSC hasn't
granted OS ASPM control, breaking driver state restoration?

While __pci_disable_link_state() intentionally bypasses the aspm_disabled
check, __pci_enable_link_state() strictly enforces it:

__pci_enable_link_state() {
    ...
    if (aspm_disabled) {
        pci_warn(pdev, "can't override BIOS ASPM; OS doesn't have ASPM control\n");
        return -EPERM;
    }
    ...
}

If a driver attempts a supported save/disable/restore sequence on such
a system, the restore step unconditionally fails, permanently disabling ASPM.

[ ... ]
> @@ -1527,6 +1528,10 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
>  		down_read(&pci_bus_sem);
>  	mutex_lock(&aspm_lock);
>  	link->aspm_default = pci_calc_aspm_enable_mask(state);
> +
> +	/* Force enable states that were previously disabled */
> +	if (force)
> +		link->aspm_disable &= ~link->aspm_default;

[Severity: High]
Does this logic neglect to clear the Clock PM disable mask?

If a driver calls pci_disable_link_state() with PCIE_LINK_STATE_CLKPM,
link->clkpm_disable is set to 1. When calling the new force-enable API
to restore the states, link->aspm_disable is cleared here, but
link->clkpm_disable remains 1.

Consequently, pcie_set_clkpm() will silently skip re-enabling Clock PM,
leading to permanent loss of clock power management.

>  	pcie_config_aspm_link(link, policy_to_aspm_state(link));

[Severity: High]
Can pci_force_enable_link_state() fail to override init-disabled ASPM
states here because link->aspm_capable remains uninitialized?

If a device was blacklisted during pcie_aspm_cap_init(), the initialization
returns early and link->aspm_capable remains 0. When pcie_config_aspm_link()
is called, it masks the requested state against link->aspm_capable:

pcie_config_aspm_link() {
    ...
    state &= link->aspm_capable;
    ...
}

Since aspm_capable is 0, the requested state becomes 0, silently failing to
enable ASPM and breaking the documented purpose of overriding the pre-1.1
device blacklist.

[ ... ]
> @@ -1576,10 +1581,36 @@ int pci_enable_link_state_locked(struct pci_dev *pdev, int state)
>  {
>  	lockdep_assert_held_read(&pci_bus_sem);
>  
> -	return __pci_enable_link_state(pdev, state, true);
> +	return __pci_enable_link_state(pdev, state, true, false);
>  }
>  EXPORT_SYMBOL(pci_enable_link_state_locked);
>  
> +/**
> + * pci_force_enable_link_state - Force enable device link state
> + * @pdev: PCI device
> + * @state: Mask of ASPM link states to enable
> + *
> + * Enable device link state, so the link will enter the specified states.
> + * Unlike pci_enable_link_state(), this also re-enables states previously
> + * disabled by pci_disable_link_state() and overrides the ASPM states disabled
> + * during init (e.g., the pre-1.1 device blacklist). The caller is therefore
> + * responsible for only enabling states the device supports, typically the ones
> + * previously reported by pcie_aspm_enabled().

[Severity: Medium]
Is it safe to advise callers to use pcie_aspm_enabled() for the state mask?

The pcie_aspm_enabled() function returns a bool, not a bitmask. If a driver
uses the return value as the state argument, the 7-bit field is truncated
to a boolean 1 or 0.

Since 1 translates to PCIE_LINK_STATE_L0S_UP (bit 0), it inadvertently
drops all other link states (like L1) the driver intended to restore.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-pci-aspm-fix-v3-0-6bd72451746e@kernel.org?part=2