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

[Val Packett]

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@xxxxxxxxxxxxxxxx>
> > > > >
> > > > > 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@xxxxxxxxxxxxxxxx/

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