[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZF1dsvJYYnl8Wv0v@bhelgaas>
Date: Thu, 11 May 2023 16:27:14 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: linux-pci@...r.kernel.org, Rob Herring <robh@...nel.org>,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Krzysztof Wilczyński <kw@...ux.com>,
Lukas Wunner <lukas@...ner.de>,
Bjorn Helgaas <bhelgaas@...gle.com>,
LKML <linux-kernel@...r.kernel.org>,
Emmanuel Grumbach <emmanuel.grumbach@...el.com>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: [PATCH 01/17] PCI: Add concurrency safe clear_and_set variants
for LNKCTL{,2}
[+cc Emmanuel, Rafael, Heiner, ancient ASPM history]
On Thu, May 11, 2023 at 10:58:40PM +0300, Ilpo Järvinen wrote:
> On Thu, 11 May 2023, Bjorn Helgaas wrote:
> > On Thu, May 11, 2023 at 08:35:48PM +0300, Ilpo Järvinen wrote:
> > > On Thu, 11 May 2023, Bjorn Helgaas wrote:
> > > > On Thu, May 11, 2023 at 04:14:25PM +0300, Ilpo Järvinen wrote:
> > > > > A few places write LNKCTL and LNKCTL2 registers without proper
> > > > > concurrency control and this could result in losing the changes
> > > > > one of the writers intended to make.
> > > > >
> > > > > Add pcie_capability_clear_and_set_word_locked() and helpers to use it
> > > > > with LNKCTL and LNKCTL2. The concurrency control is provided using a
> > > > > spinlock in the struct pci_dev.
> ...
[beginning of thread is
https://lore.kernel.org/r/20230511131441.45704-1-ilpo.jarvinen@linux.intel.com;
context here is that several drivers clear ASPM config directly,
probably because pci_disable_link_state() doesn't always do it]
> > 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():
>
> static void bcm4377_disable_aspm(struct bcm4377_data *bcm4377)
> {
> pci_disable_link_state(bcm4377->pdev,
> PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
>
> /*
> * pci_disable_link_state can fail if either CONFIG_PCIEASPM is disabled
> * or if the BIOS hasn't handed over control to us. We must *always*
> * disable ASPM for this device due to hardware errata though.
> */
> pcie_capability_clear_word(bcm4377->pdev, PCI_EXP_LNKCTL,
> PCI_EXP_LNKCTL_ASPMC);
> }
>
> 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.)
>
> It might even be that some drivers don't even bother to make the
> pci_disable_link_state() call because it isn't reliable enough.
Yeah, I noticed that this is problematic.
We went round and round about this ten years ago [1], which resulted
in https://git.kernel.org/linus/2add0ec14c25 ("PCI/ASPM: Warn when
driver asks to disable ASPM, but we can't do it").
I'm not 100% convinced by that anymore. It's true that if firmware
retains control of the PCIe capability, the OS is technically not
allowed to write to it, and it's conceivable that even a locked OS
update could collide with some SMI or something that also writes to
it.
I can certainly imagine that firmware might know that *enabling* ASPM
might break because of signal integrity issues or something. It seems
less likely that *disabling* ASPM would break something, but Rafael [2]
and Matthew [3] rightly pointed out that there is some risk.
But the current situation, where pci_disable_link_state() does nothing
if CONFIG_PCIEASPM is unset or if _OSC says firmware owns it, leads to
drivers doing it directly anyway. I'm not sure that's better than
making pci_disable_link_state() work 100% of the time, regardless of
CONFIG_PCIEASPM and _OSC. At least then the PCI core would know
what's going on.
Bjorn
[1] https://lore.kernel.org/all/CANUX_P3F5YhbZX3WGU-j1AGpbXb_T9Bis2ErhvKkFMtDvzatVQ@mail.gmail.com/
[2] https://lore.kernel.org/all/1725435.3DlCxYF2FV@vostro.rjw.lan/
[3] https://lore.kernel.org/all/1368303730.2425.47.camel@x230/
Powered by blists - more mailing lists