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?
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_LABS,
> > > 0x0031,
> > > + PCI_CLASS_BRIDGE_PCI, 8,
> > > quirk_al_vpd_release);
> > > +
> > > #endif
> > > --
> > > 2.17.1
> > >
