Re: [PATCH 2/6] PCI/VPD: Remove struct pci_vpd_ops
From: Bjorn Helgaas <helgaas@kernel.org>
Date: 2021-08-11 22:00:50
[+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@mdrustad-wks.jf.intel.com/ (local) [2] https://lore.kernel.org/linux-pci/20150519160158.00002cd6@unknown/ (local)
quoted hunk ↗ jump to hunk
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- 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