[+cc Mark, Alex D, Jesse, Alex W] On Sun, Aug 08, 2021 at 07:20:05PM +0200, Heiner Kallweit wrote: > Code can be significantly simplified by removing struct pci_vpd_ops and > avoiding the indirect calls. I really like this patch. Nothing to do with *this* patch, but I have a little heartburn about the "access somebody else's VPD" approach. I think the beginning of this was Mark's post [1]. IIUC, there are Intel multifunction NICs that share some VPD hardware between functions, and concurrent accesses to VPD of different functions doesn't work correctly. I'm pretty sure this is a defect per spec, because PCIe r5.0, sec 7.9.19 doesn't say anything about having to treat VPD on multi-function devices specially. The current solution is for all Intel multi-function NICs to redirect VPD access to function 0. That single-threads VPD access across all the functions because we hold function 0's vpd->lock mutex while reading or writing. But I think we still have the problem that this implicit sharing of function 0's VPD opens a channel between functions: if functions are assigned to different VMs, the VMs can communicate by reading and writing VPD. So I wonder if we should just disallow VPD access for these NICs except on function 0. There was a little bit of discussion in that direction at [2]. [1] https://lore.kernel.org/linux-pci/20150519000037.56109.68356.stgit@xxxxxxxxxxxxxxxxxxxxxxxxx/ [2] https://lore.kernel.org/linux-pci/20150519160158.00002cd6@unknown/ > Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx> > --- > drivers/pci/vpd.c | 90 ++++++++++++++++++----------------------------- > 1 file changed, 34 insertions(+), 56 deletions(-) > > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c > index e87f299ee..6a0d617b2 100644 > --- a/drivers/pci/vpd.c > +++ b/drivers/pci/vpd.c > @@ -13,13 +13,7 @@ > > /* VPD access through PCI 2.2+ VPD capability */ > > -struct pci_vpd_ops { > - ssize_t (*read)(struct pci_dev *dev, loff_t pos, size_t count, void *buf); > - ssize_t (*write)(struct pci_dev *dev, loff_t pos, size_t count, const void *buf); > -}; > - > struct pci_vpd { > - const struct pci_vpd_ops *ops; > struct mutex lock; > unsigned int len; > u8 cap; > @@ -123,11 +117,16 @@ static int pci_vpd_wait(struct pci_dev *dev, bool set) > static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, > void *arg) > { > - struct pci_vpd *vpd = dev->vpd; > + struct pci_vpd *vpd; > int ret = 0; > loff_t end = pos + count; > u8 *buf = arg; > > + if (!dev || !dev->vpd) > + return -ENODEV; > + > + vpd = dev->vpd; > + > if (pos < 0) > return -EINVAL; > > @@ -189,11 +188,16 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, > static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, > const void *arg) > { > - struct pci_vpd *vpd = dev->vpd; > + struct pci_vpd *vpd; > const u8 *buf = arg; > loff_t end = pos + count; > int ret = 0; > > + if (!dev || !dev->vpd) > + return -ENODEV; > + > + vpd = dev->vpd; > + > if (pos < 0 || (pos & 3) || (count & 3)) > return -EINVAL; > > @@ -238,44 +242,6 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, > return ret ? ret : count; > } > > -static const struct pci_vpd_ops pci_vpd_ops = { > - .read = pci_vpd_read, > - .write = pci_vpd_write, > -}; > - > -static ssize_t pci_vpd_f0_read(struct pci_dev *dev, loff_t pos, size_t count, > - void *arg) > -{ > - struct pci_dev *tdev = pci_get_func0_dev(dev); > - ssize_t ret; > - > - if (!tdev) > - return -ENODEV; > - > - ret = pci_read_vpd(tdev, pos, count, arg); > - pci_dev_put(tdev); > - return ret; > -} > - > -static ssize_t pci_vpd_f0_write(struct pci_dev *dev, loff_t pos, size_t count, > - const void *arg) > -{ > - struct pci_dev *tdev = pci_get_func0_dev(dev); > - ssize_t ret; > - > - if (!tdev) > - return -ENODEV; > - > - ret = pci_write_vpd(tdev, pos, count, arg); > - pci_dev_put(tdev); > - return ret; > -} > - > -static const struct pci_vpd_ops pci_vpd_f0_ops = { > - .read = pci_vpd_f0_read, > - .write = pci_vpd_f0_write, > -}; > - > void pci_vpd_init(struct pci_dev *dev) > { > struct pci_vpd *vpd; > @@ -290,10 +256,6 @@ void pci_vpd_init(struct pci_dev *dev) > return; > > vpd->len = PCI_VPD_MAX_SIZE; > - if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0) > - vpd->ops = &pci_vpd_f0_ops; > - else > - vpd->ops = &pci_vpd_ops; > mutex_init(&vpd->lock); > vpd->cap = cap; > vpd->valid = 0; > @@ -388,9 +350,17 @@ EXPORT_SYMBOL_GPL(pci_vpd_find_info_keyword); > */ > ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf) > { > - if (!dev->vpd || !dev->vpd->ops) > - return -ENODEV; > - return dev->vpd->ops->read(dev, pos, count, buf); > + ssize_t ret; > + > + if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0) { > + dev = pci_get_func0_dev(dev); > + ret = pci_vpd_read(dev, pos, count, buf); > + pci_dev_put(dev); > + } else { > + ret = pci_vpd_read(dev, pos, count, buf); > + } > + > + return ret; > } > EXPORT_SYMBOL(pci_read_vpd); > > @@ -403,9 +373,17 @@ EXPORT_SYMBOL(pci_read_vpd); > */ > ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf) > { > - if (!dev->vpd || !dev->vpd->ops) > - return -ENODEV; > - return dev->vpd->ops->write(dev, pos, count, buf); > + ssize_t ret; > + > + if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0) { > + dev = pci_get_func0_dev(dev); > + ret = pci_vpd_write(dev, pos, count, buf); > + pci_dev_put(dev); > + } else { > + ret = pci_vpd_write(dev, pos, count, buf); > + } > + > + return ret; > } > EXPORT_SYMBOL(pci_write_vpd); > > -- > 2.32.0 > >
