Re: [PATCH 01/17] PCI: Add concurrency safe clear_and_set variants for LNKCTL{,2}

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 1880 bytes --]

On Thu, 11 May 2023, Lukas Wunner wrote:

> On Thu, May 11, 2023 at 10:58:40PM +0300, Ilpo Järvinen wrote:
> > On Thu, 11 May 2023, Bjorn Helgaas wrote:
> > > Many of these are ASPM-related updates that IMHO should not be in
> > > drivers at all.  Drivers should use PCI core interfaces so the core
> > > doesn't get confused.
> > 
> > Ah, yes. I forgot to mention it in the cover letter but I noticed that 
> > some of those seem to be workarounds for the cases where core refuses to 
> > disable ASPM. Some sites even explicit have a comment about that after 
> > the call to pci_disable_link_state():
> [...]
> > That kinda feels something that would want a force disable quirk that is 
> > reliable. There are quirks for some devices which try to disable it but 
> > could fail for reasons mentioned in that comment. (But I'd prefer to make 
> > another series out of it rather than putting it into this one.)
> 
> I'm wondering if it's worth cleaning up ASPM handling in drivers first
> as the locking issue may then largely solve itself.  The locking could
> probably be kept internal to ASPM core code then.

For some part yes, but at least those copy-pasted gpu setup codes did some 
other things too.

In any case, it would go against some earlier policy decision:

/**
 * pci_disable_link_state - Disable device's link state, so the link will
 * never enter specific states.  Note that if the BIOS didn't grant ASPM
 * control to the OS, this does nothing because we can't touch the LNKCTL
 * register. Returns 0 or a negative errno.

Is it fine to make core capable of violating that policy?

One question before I trying to come up something is when PCIEASPM is =n, 
should I provide some simple function that just does the LNKCTL write to 
disable it? And another thing is the existing quirks, should they be 
kept depending on the existing behavior or not?


-- 
 i.