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: 4487 bytes --]

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.
> > > > 
> > > > Suggested-by: Lukas Wunner <lukas@wunner.de>
> > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > 
> > > Thanks for raising this issue!  Definitely looks like something that
> > > needs attention.
> > > 
> > > > ---
> > > >  drivers/pci/access.c | 14 ++++++++++++++
> > > >  drivers/pci/probe.c  |  1 +
> > > >  include/linux/pci.h  | 17 +++++++++++++++++
> > > >  3 files changed, 32 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > > > index 3c230ca3de58..d92a3daadd0c 100644
> > > > --- a/drivers/pci/access.c
> > > > +++ b/drivers/pci/access.c
> > > > @@ -531,6 +531,20 @@ int pcie_capability_clear_and_set_dword(struct pci_dev *dev, int pos,
> > > >  }
> > > >  EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
> > > >  
> > > > +int pcie_capability_clear_and_set_word_locked(struct pci_dev *dev, int pos,
> > > > +					      u16 clear, u16 set)
> > > > +{
> > > > +	unsigned long flags;
> > > > +	int ret;
> > > > +
> > > > +	spin_lock_irqsave(&dev->cap_lock, flags);
> > > > +	ret = pcie_capability_clear_and_set_word(dev, pos, clear, set);
> > > > +	spin_unlock_irqrestore(&dev->cap_lock, flags);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +EXPORT_SYMBOL(pcie_capability_clear_and_set_word_locked);
> > > 
> > > I didn't see the prior discussion with Lukas, so maybe this was
> > > answered there, but is there any reason not to add locking to
> > > pcie_capability_clear_and_set_word() and friends directly?  
> > >
> > > It would be nice to avoid having to decide whether to use the locked
> > > or unlocked versions.  It would also be nice to preserve the use of
> > > PCI_EXP_LNKCTL directly, for grep purposes.  And it would obviate the
> > > need for some of these patches, e.g., the use of
> > > pcie_capability_clear_word(), where it's not obvious at the call site
> > > why a change is needed.
> > 
> > There wasn't that big discussion about it (internally). I brought both
> > alternatives up and Lukas just said he didn't know what's the best 
> > approach (+ gave a weak nudge towards the separate accessor so I went 
> > with it to make forward progress). Based on that I don't think he had a 
> > strong opinion on it.
> > 
> > I'm certainly fine to just use it in the normal accessor functions that 
> > do RMW and add the locking there. It would certainly have to those good 
> > sides you mentioned.
> 
> Let's start with that, then.
> 
> 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.


-- 
 i.