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

[Val Packett <val@packett.cool>]

On 10/13/26 2:42 PM, Ilpo Järvinen wrote:
> On Tue, 13 Oct 2026, Bjorn Helgaas wrote:
>> On Tue, Oct 13, 2026 at 02:41:32PM +0300, Ilpo Järvinen wrote:
>>> On Mon, 12 Oct 2026, Bjorn Helgaas wrote:
>>>> On Wed, Jul 08, 2026 at 04:30:16PM +0200, Manivannan Sadhasivam wrote:
>>>>> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>>>>>
>>>>> pci_enable_link_state() and pci_enable_link_state_locked() APIs only enable
>>>>> the ASPM states that were not previously disabled via
>>>>> pci_disable_link_state*() API or blacklisted during init (e.g. the pre-1.1
>>>>> device blacklist, which is only meant to be overridden with
>>>>> 'pcie_aspm=force'). This is an intentional behavior as these APIs must not
>>>>> silently re-enable states that were deliberately disabled, since doing so
>>>>> could enable ASPM on a link where it is known to be unsafe.
>>>> I think this is saying that many uses of pci_disable_link_state() are
>>>> to avoid hardware errata, i.e., these are basically quirks, and we
>>>> should *never* enable those ASPM states.
>>>>
>>>>> However, some drivers (e.g. the atheros WLAN drivers) save the currently
>>>>> enabled ASPM states, disable all ASPM states before firmware download, and
>>>>> then try to restore exactly the states that were enabled before. Restoring
>>>>> those states requires re-enabling states that were just disabled via
>>>>> pci_disable_link_state() API. But, this cannot be achieved using the
>>>>> existing APIs.
>>>> This sounds like it's also for hardware errata: if ASPM works
>>>> correctly, there's no need to disable it during firmware download.  I
>>>> think this is *only* the atheros drivers, isn't it?  I didn't see any
>>>> others.
>>> There are more that should use disable/enable API. In addition ath*, I had
>>> also these in my unfinished series:
>>>
>>>    brcmfmac
>>>    ib-hfi1
>>>    rtlwifi
>>>    rtsx
>>>    r8169
> [..]
>>>> Instead of adding pci_force_enable_link_state(), I think we should add
>>>> a new interface for the *quirk* behavior that permanently disables
>>>> states.  Then pci_enable_link_state() and pci_disable_link_state()
>>>> could be made symmetric.
>>> My suggestion is to make the disables/enables counted per state which
>>> would allow parallel sources of disables to function without clashing.
>> By parallel sources, are you thinking of sysfs and driver calls?  If
>> locking would be sufficient I think that would end up being simpler.
>>
>> But maybe the issue is these devices where we think ASPM breakage only
>> happens in certain circumstances, e.g., firmware download, so we want
>> to keep states disabled for that period.  Then I guess locking inside
>> the enable/disable doesn't prevent sysfs from enabling those states
>> during that period.
> There are (at least) these sources:
>
> - quirks (disable only)
> - hw has ASPM related problem at certain phase of operation
> [..]

FWIW, one not-yet-merged user is this patch that disables ASPM around 
link retraining for bwctrl on host controllers that do require that:

https://lore.kernel.org/all/20260819-bwscale-v5-1-6dea79786b37@oss.qualcomm.com/

That particular version of the patch was already written assuming 
symmetric enable/disable and I hit that when testing it, so here's an 
example of the potential trouble :)


Naming suggestion: if it ends up being that counter thing, it should use 
the verb "inhibit", like idle inhibitors in userspace. Those are 
commonly understood to be counter-based ("as long as there's at least 
one inhibitor, the state is disabled") and temporary.

~val