Re: [PATCH v4 3/7] PCI/VPD: Add VPD release quirk for Amazon's Annapurna Labs Root Port

[Chocron, Jonathan]

On Thu, 2019-08-22 at 16:07 +0100, Andrew Murray wrote:
> On Thu, Aug 22, 2019 at 02:36:24PM +0000, Chocron, Jonathan wrote:
> > On Thu, 2019-08-22 at 12:41 +0100, Andrew Murray wrote:
> > > On Wed, Aug 21, 2019 at 06:35:43PM +0300, Jonathan Chocron wrote:
> > > > The Amazon Annapurna Labs PCIe Root Port exposes the VPD
> > > > capability,
> > > > but there is no actual support for it.
> > > > 
> > > > The reason for not using the already existing
> > > > quirk_blacklist_vpd()
> > > > is that, although this fails pci_vpd_read/write, the 'vpd'
> > > > sysfs
> > > > entry still exists. When running lspci -vv, for example, this
> > > > results in the following error:
> > > > 
> > > > pcilib: sysfs_read_vpd: read failed: Input/output error
> > > 
> > > Oh that's not nice. It's probably triggered by the -EIO in
> > > pci_vpd_read.
> > > A quick search online seems to show that other people have
> > > experienced
> > > this too - though from as far as I can tell this just gives you a
> > > warning and pcilib will continnue to give other output?
> > > 
> > 
> > Correct.
> > 
> > > I guess every vpd blacklist'd driver will have the same issue.
> > > And
> > > for
> > > this reason I don't think that this patch is the right solution -
> > > as
> > > otherwise all the other blacklisted drivers could follow your
> > > lead.
> > > 
> > 
> > I think that going forward, they should follow my lead, I just
> > didn't
> > want to possibly break any assumptions other vendors' tools might
> > have
> > regarding the existence/non-existence of the vpd sysfs entry.
> > 
> > > I don't think you need to fix this specifically for the AL driver
> > > and
> > > so
> > > I'd suggest that you can probably drop this patch. (Ideally
> > > pciutils
> > > could be updated to not warn for this specific use-case).
> > > 
> > 
> > I don't think that solution should be implemented in pcituils. It
> > rightfully warns when it fails to read from the vpd sysfs file - it
> > first 'open's the file which succeeds, and then fails when trying
> > to
> > 'read' from it.
> 
> Indeed - this is correct.
> 
> > I don't think that it should specifically "mask" out
> > -EIO, since it shouldn't have to "know" that the underlying reason
> > is a
> 
> You're probably right - I guess the kernel should document somewhere
> (ABI/testing/sysfs-bus-pci?) what the kernel does when such a quirk
> exists,
> then userspace can conform. For example if -EIO cannot be returned
> any
> other way then it would be OK for pciutils to mask it out - but its
> ambigious at the moment.
> 
> > VPD quirk (or more precisely vpd->len == 0). Furthermore, it is
> > possible that this error code would be returned for some other
> > reason
> > (not sure if currently this occurs).
> > 
> > I think that if the device doesn't properly support vpd, the kernel
> > shouldn't expose the "empty" sysfs file in the first place.
> > 
> > In the long run, quirk_blacklist_vpd() should probably be modified
> > to
> > do what our quirk does or something similar (and then the al quirk
> > can
> > be removed). What do you think?
> 
> When I first saw your quirk, I did wonder why quirk_blacklist_vpd
> doesn't
> do what your quirk does. Perhaps there isn't a reason. It was first
> introduced in 2016:
> 
> 7c20078a8197 ("PCI: Prevent VPD access for buggy devices")
> 
> Some may argue that actually because your hardware has a VPD
> capability
> it should have the sysfs file - but the capability doesn't work and
> so
> the sysfs file should return an error.
> 
> I'd be keen to change quirk_blacklist_vpd - Babu, Bjorn any
> objections?
> 
Since the merge window is closing and I don't want to affect any other
PCIe controllers without having their maintainers testing this change,
I'll remove this function and register our device_id with the existing
quirk_blacklist_vpd. This will be part of v5.

I'll then submit a separate patch (for the next kernel version) which
changes the quirk_blacklist_vpd to do what I originally intended.

> Thanks,
> 
> Andrew Murray
> 
> > 
> > > Thanks,
> > > 
> > > Andrew Murray
> > > 
> > > > 
> > > > This quirk removes the sysfs entry, which avoids the error
> > > > print.
> > > > 
> > > > Signed-off-by: Jonathan Chocron <jonnyc@xxxxxxxxxx>
> > > > Reviewed-by: Gustavo Pimentel <gustavo.pimentel@xxxxxxxxxxxx>
> > > > ---
> > > >  drivers/pci/vpd.c | 16 ++++++++++++++++
> > > >  1 file changed, 16 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> > > > index 4963c2e2bd4c..c23a8ec08db9 100644
> > > > --- a/drivers/pci/vpd.c
> > > > +++ b/drivers/pci/vpd.c
> > > > @@ -644,4 +644,20 @@ static void
> > > > quirk_chelsio_extend_vpd(struct
> > > > pci_dev *dev)
> > > >  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
> > > >  			quirk_chelsio_extend_vpd);
> > > >  
> > > > +static void quirk_al_vpd_release(struct pci_dev *dev)
> > > > +{
> > > > +	if (dev->vpd) {
> > > > +		pci_vpd_release(dev);
> > > > +		dev->vpd = NULL;
> > > > +		pci_warn(dev, FW_BUG "Releasing VPD capability
> > > > (No
> > > > support for VPD read/write transactions)\n");
> > > > +	}
> > > > +}
> > > > +
> > > > +/*
> > > > + * The 0031 device id is reused for other non Root Port device
> > > > types,
> > > > + * therefore the quirk is registered for the
> > > > PCI_CLASS_BRIDGE_PCI
> > > > class.
> > > > + */
> > > > +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_L
> > > > ABS,
> > > > 0x0031,
> > > > +			      PCI_CLASS_BRIDGE_PCI, 8,
> > > > quirk_al_vpd_release);
> > > > +
> > > >  #endif
> > > > -- 
> > > > 2.17.1
> > > >