[PATCH 0/6] PCI/VPD: Further improvements

[PATCH 0/6] PCI/VPD: Further improvements

[Heiner Kallweit]

This series includes a number of further improvements to VPD handling.

Heiner Kallweit (6):
  PCI/VPD: Move pci_read/write_vpd in the code
  PCI/VPD: Remove struct pci_vpd_ops
  PCI/VPD: Remove member valid from struct pci_vpd
  PCI/VPD: Embed struct pci_vpd member in struct pci_dev
  PCI/VPD: Determine VPD size in pci_vpd_init already
  PCI/VPD: Treat invalid VPD like no VPD capability

 drivers/pci/probe.c |   1 -
 drivers/pci/vpd.c   | 253 ++++++++++++++++----------------------------
 include/linux/pci.h |   9 +-
 3 files changed, 97 insertions(+), 166 deletions(-)

-- 
2.32.0

[PATCH 1/6] PCI/VPD: Move pci_read/write_vpd in the code

[Heiner Kallweit]

In preparation of subsequent patches move these two functions in the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/vpd.c | 60 +++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 5e2e63809..e87f299ee 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -31,36 +31,6 @@ static struct pci_dev *pci_get_func0_dev(struct pci_dev *dev)
 	return pci_get_slot(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
 }
 
-/**
- * pci_read_vpd - Read one entry from Vital Product Data
- * @dev:	pci device struct
- * @pos:	offset in vpd space
- * @count:	number of bytes to read
- * @buf:	pointer to where to store result
- */
-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);
-}
-EXPORT_SYMBOL(pci_read_vpd);
-
-/**
- * pci_write_vpd - Write entry to Vital Product Data
- * @dev:	pci device struct
- * @pos:	offset in vpd space
- * @count:	number of bytes to write
- * @buf:	buffer containing write data
- */
-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);
-}
-EXPORT_SYMBOL(pci_write_vpd);
-
 #define PCI_VPD_MAX_SIZE (PCI_VPD_ADDR_MASK + 1)
 
 /**
@@ -409,6 +379,36 @@ int pci_vpd_find_info_keyword(const u8 *buf, unsigned int off,
 }
 EXPORT_SYMBOL_GPL(pci_vpd_find_info_keyword);
 
+/**
+ * pci_read_vpd - Read one entry from Vital Product Data
+ * @dev:	pci device struct
+ * @pos:	offset in vpd space
+ * @count:	number of bytes to read
+ * @buf:	pointer to where to store result
+ */
+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);
+}
+EXPORT_SYMBOL(pci_read_vpd);
+
+/**
+ * pci_write_vpd - Write entry to Vital Product Data
+ * @dev:	pci device struct
+ * @pos:	offset in vpd space
+ * @count:	number of bytes to write
+ * @buf:	buffer containing write data
+ */
+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);
+}
+EXPORT_SYMBOL(pci_write_vpd);
+
 #ifdef CONFIG_PCI_QUIRKS
 /*
  * Quirk non-zero PCI functions to route VPD access through function 0 for
-- 
2.32.0

[PATCH 2/6] PCI/VPD: Remove struct pci_vpd_ops

[Heiner Kallweit]

Code can be significantly simplified by removing struct pci_vpd_ops and
avoiding the indirect calls.

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

[PATCH 3/6] PCI/VPD: Remove member valid from struct pci_vpd

[Heiner Kallweit]

Instead of having a separate flag let's use vp->len != 0 as indicator that
VPD validity has been checked. Now vpd->len == PCI_VPD_SZ_INVALID is used
to indicate that VPD is invalid.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/vpd.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 6a0d617b2..d6c216caf 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -17,7 +17,6 @@ struct pci_vpd {
 	struct mutex	lock;
 	unsigned int	len;
 	u8		cap;
-	unsigned int	valid:1;
 };
 
 static struct pci_dev *pci_get_func0_dev(struct pci_dev *dev)
@@ -25,7 +24,8 @@ static struct pci_dev *pci_get_func0_dev(struct pci_dev *dev)
 	return pci_get_slot(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
 }
 
-#define PCI_VPD_MAX_SIZE (PCI_VPD_ADDR_MASK + 1)
+#define PCI_VPD_MAX_SIZE	(PCI_VPD_ADDR_MASK + 1)
+#define PCI_VPD_SZ_INVALID	UINT_MAX
 
 /**
  * pci_vpd_size - determine actual size of Vital Product Data
@@ -36,6 +36,9 @@ static size_t pci_vpd_size(struct pci_dev *dev)
 	size_t off = 0;
 	unsigned char header[1+2];	/* 1 byte tag, 2 bytes length */
 
+	/* Otherwise the following reads would fail. */
+	dev->vpd->len = PCI_VPD_MAX_SIZE;
+
 	while (pci_read_vpd(dev, off, 1, header) == 1) {
 		unsigned char tag;
 		size_t size;
@@ -48,7 +51,7 @@ static size_t pci_vpd_size(struct pci_dev *dev)
 			if (pci_read_vpd(dev, off + 1, 2, &header[1]) != 2) {
 				pci_warn(dev, "failed VPD read at offset %zu\n",
 					 off + 1);
-				return off;
+				return off ?: PCI_VPD_SZ_INVALID;
 			}
 			size = pci_vpd_lrdt_size(header);
 			if (off + size > PCI_VPD_MAX_SIZE)
@@ -73,7 +76,7 @@ static size_t pci_vpd_size(struct pci_dev *dev)
 	pci_info(dev, "invalid VPD tag %#04x at offset %zu%s\n",
 		 header[0], off, off == 0 ?
 		 "; assume missing optional EEPROM" : "");
-	return off;
+	return off ?: PCI_VPD_SZ_INVALID;
 }
 
 /*
@@ -130,12 +133,10 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
 	if (pos < 0)
 		return -EINVAL;
 
-	if (!vpd->valid) {
-		vpd->valid = 1;
+	if (!vpd->len)
 		vpd->len = pci_vpd_size(dev);
-	}
 
-	if (vpd->len == 0)
+	if (vpd->len == PCI_VPD_SZ_INVALID)
 		return -EIO;
 
 	if (pos > vpd->len)
@@ -201,12 +202,10 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count,
 	if (pos < 0 || (pos & 3) || (count & 3))
 		return -EINVAL;
 
-	if (!vpd->valid) {
-		vpd->valid = 1;
+	if (!vpd->len)
 		vpd->len = pci_vpd_size(dev);
-	}
 
-	if (vpd->len == 0)
+	if (vpd->len == PCI_VPD_SZ_INVALID)
 		return -EIO;
 
 	if (end > vpd->len)
@@ -255,10 +254,8 @@ void pci_vpd_init(struct pci_dev *dev)
 	if (!vpd)
 		return;
 
-	vpd->len = PCI_VPD_MAX_SIZE;
 	mutex_init(&vpd->lock);
 	vpd->cap = cap;
-	vpd->valid = 0;
 	dev->vpd = vpd;
 }
 
@@ -423,8 +420,7 @@ DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
 static void quirk_blacklist_vpd(struct pci_dev *dev)
 {
 	if (dev->vpd) {
-		dev->vpd->len = 0;
-		dev->vpd->valid = 1;
+		dev->vpd->len = PCI_VPD_SZ_INVALID;
 		pci_warn(dev, FW_BUG "disabling VPD access (can't determine size of non-standard VPD format)\n");
 	}
 }
@@ -455,7 +451,6 @@ static void pci_vpd_set_size(struct pci_dev *dev, size_t len)
 	if (!vpd || len == 0 || len > PCI_VPD_MAX_SIZE)
 		return;
 
-	vpd->valid = 1;
 	vpd->len = len;
 }
 
-- 
2.32.0

[PATCH 4/6] PCI/VPD: Embed struct pci_vpd member in struct pci_dev

[Heiner Kallweit]

Now that struct pci_vpd became really small, we can simplify the code
by embedding a struct pci_vpd member in struct pci_dev instead of
dynamically allocating it.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/probe.c |  1 -
 drivers/pci/vpd.c   | 67 ++++++++++-----------------------------------
 include/linux/pci.h |  9 ++++--
 3 files changed, 21 insertions(+), 56 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 79177ac37..0ec5c792c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2225,7 +2225,6 @@ static void pci_release_capabilities(struct pci_dev *dev)
 {
 	pci_aer_exit(dev);
 	pci_rcec_exit(dev);
-	pci_vpd_release(dev);
 	pci_iov_release(dev);
 	pci_free_cap_save_buffers(dev);
 }
diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index d6c216caf..86f9440e0 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -13,12 +13,6 @@
 
 /* VPD access through PCI 2.2+ VPD capability */
 
-struct pci_vpd {
-	struct mutex	lock;
-	unsigned int	len;
-	u8		cap;
-};
-
 static struct pci_dev *pci_get_func0_dev(struct pci_dev *dev)
 {
 	return pci_get_slot(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
@@ -37,7 +31,7 @@ static size_t pci_vpd_size(struct pci_dev *dev)
 	unsigned char header[1+2];	/* 1 byte tag, 2 bytes length */
 
 	/* Otherwise the following reads would fail. */
-	dev->vpd->len = PCI_VPD_MAX_SIZE;
+	dev->vpd.len = PCI_VPD_MAX_SIZE;
 
 	while (pci_read_vpd(dev, off, 1, header) == 1) {
 		unsigned char tag;
@@ -90,7 +84,7 @@ static size_t pci_vpd_size(struct pci_dev *dev)
  */
 static int pci_vpd_wait(struct pci_dev *dev, bool set)
 {
-	struct pci_vpd *vpd = dev->vpd;
+	struct pci_vpd *vpd = &dev->vpd;
 	unsigned long timeout = jiffies + msecs_to_jiffies(125);
 	unsigned long max_sleep = 16;
 	u16 status;
@@ -120,16 +114,14 @@ 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;
+	struct pci_vpd *vpd = &dev->vpd;
 	int ret = 0;
 	loff_t end = pos + count;
 	u8 *buf = arg;
 
-	if (!dev || !dev->vpd)
+	if (!vpd->cap)
 		return -ENODEV;
 
-	vpd = dev->vpd;
-
 	if (pos < 0)
 		return -EINVAL;
 
@@ -189,16 +181,14 @@ 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;
+	struct pci_vpd *vpd = &dev->vpd;
 	const u8 *buf = arg;
 	loff_t end = pos + count;
 	int ret = 0;
 
-	if (!dev || !dev->vpd)
+	if (!vpd->cap)
 		return -ENODEV;
 
-	vpd = dev->vpd;
-
 	if (pos < 0 || (pos & 3) || (count & 3))
 		return -EINVAL;
 
@@ -243,25 +233,8 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count,
 
 void pci_vpd_init(struct pci_dev *dev)
 {
-	struct pci_vpd *vpd;
-	u8 cap;
-
-	cap = pci_find_capability(dev, PCI_CAP_ID_VPD);
-	if (!cap)
-		return;
-
-	vpd = kzalloc(sizeof(*vpd), GFP_ATOMIC);
-	if (!vpd)
-		return;
-
-	mutex_init(&vpd->lock);
-	vpd->cap = cap;
-	dev->vpd = vpd;
-}
-
-void pci_vpd_release(struct pci_dev *dev)
-{
-	kfree(dev->vpd);
+	dev->vpd.cap = pci_find_capability(dev, PCI_CAP_ID_VPD);
+	mutex_init(&dev->vpd.lock);
 }
 
 static ssize_t vpd_read(struct file *filp, struct kobject *kobj,
@@ -293,7 +266,7 @@ static umode_t vpd_attr_is_visible(struct kobject *kobj,
 {
 	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
 
-	if (!pdev->vpd)
+	if (!pdev->vpd.cap)
 		return 0;
 
 	return a->attr.mode;
@@ -401,7 +374,7 @@ static void quirk_f0_vpd_link(struct pci_dev *dev)
 	if (!f0)
 		return;
 
-	if (f0->vpd && dev->class == f0->class &&
+	if (f0->vpd.cap && dev->class == f0->class &&
 	    dev->vendor == f0->vendor && dev->device == f0->device)
 		dev->dev_flags |= PCI_DEV_FLAGS_VPD_REF_F0;
 
@@ -419,10 +392,8 @@ DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
  */
 static void quirk_blacklist_vpd(struct pci_dev *dev)
 {
-	if (dev->vpd) {
-		dev->vpd->len = PCI_VPD_SZ_INVALID;
-		pci_warn(dev, FW_BUG "disabling VPD access (can't determine size of non-standard VPD format)\n");
-	}
+	dev->vpd.len = PCI_VPD_SZ_INVALID;
+	pci_warn(dev, FW_BUG "disabling VPD access (can't determine size of non-standard VPD format)\n");
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0060, quirk_blacklist_vpd);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x007c, quirk_blacklist_vpd);
@@ -444,16 +415,6 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, PCI_ANY_ID,
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS, 0x0031,
 			      PCI_CLASS_BRIDGE_PCI, 8, quirk_blacklist_vpd);
 
-static void pci_vpd_set_size(struct pci_dev *dev, size_t len)
-{
-	struct pci_vpd *vpd = dev->vpd;
-
-	if (!vpd || len == 0 || len > PCI_VPD_MAX_SIZE)
-		return;
-
-	vpd->len = len;
-}
-
 static void quirk_chelsio_extend_vpd(struct pci_dev *dev)
 {
 	int chip = (dev->device & 0xf000) >> 12;
@@ -472,9 +433,9 @@ static void quirk_chelsio_extend_vpd(struct pci_dev *dev)
 	 * limits.
 	 */
 	if (chip == 0x0 && prod >= 0x20)
-		pci_vpd_set_size(dev, 8192);
+		dev->vpd.len = 8192;
 	else if (chip >= 0x4 && func < 0x8)
-		pci_vpd_set_size(dev, 2048);
+		dev->vpd.len = 2048;
 }
 
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 540b377ca..e752cc39a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -300,9 +300,14 @@ struct pci_cap_saved_state {
 	struct pci_cap_saved_data	cap;
 };
 
+struct pci_vpd {
+	struct mutex	lock;
+	unsigned int	len;
+	u8		cap;
+};
+
 struct irq_affinity;
 struct pcie_link_state;
-struct pci_vpd;
 struct pci_sriov;
 struct pci_p2pdma;
 struct rcec_ea;
@@ -473,7 +478,7 @@ struct pci_dev {
 #ifdef CONFIG_PCI_MSI
 	const struct attribute_group **msi_irq_groups;
 #endif
-	struct pci_vpd *vpd;
+	struct pci_vpd	vpd;
 #ifdef CONFIG_PCIE_DPC
 	u16		dpc_cap;
 	unsigned int	dpc_rp_extensions:1;
-- 
2.32.0

[PATCH 5/6] PCI/VPD: Determine VPD size in pci_vpd_init already

[Heiner Kallweit]

After preceding patches in this series we now can move determining
VPD size to pci_vpd_init(). Any quirk sets dev->vpd.len to a
non-zero value and therefore causes skipping the dynamic size
calculation. Prerequisite is that we move the quirks from FINAL
to HEADER stage.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/vpd.c | 42 +++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 86f9440e0..9187ba496 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -125,9 +125,6 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
 	if (pos < 0)
 		return -EINVAL;
 
-	if (!vpd->len)
-		vpd->len = pci_vpd_size(dev);
-
 	if (vpd->len == PCI_VPD_SZ_INVALID)
 		return -EIO;
 
@@ -192,9 +189,6 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count,
 	if (pos < 0 || (pos & 3) || (count & 3))
 		return -EINVAL;
 
-	if (!vpd->len)
-		vpd->len = pci_vpd_size(dev);
-
 	if (vpd->len == PCI_VPD_SZ_INVALID)
 		return -EIO;
 
@@ -235,6 +229,9 @@ void pci_vpd_init(struct pci_dev *dev)
 {
 	dev->vpd.cap = pci_find_capability(dev, PCI_CAP_ID_VPD);
 	mutex_init(&dev->vpd.lock);
+
+	if (!dev->vpd.len)
+		dev->vpd.len = pci_vpd_size(dev);
 }
 
 static ssize_t vpd_read(struct file *filp, struct kobject *kobj,
@@ -395,25 +392,24 @@ static void quirk_blacklist_vpd(struct pci_dev *dev)
 	dev->vpd.len = PCI_VPD_SZ_INVALID;
 	pci_warn(dev, FW_BUG "disabling VPD access (can't determine size of non-standard VPD format)\n");
 }
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0060, quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x007c, quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0413, quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0078, quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0079, quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0073, quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0071, quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005b, quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x002f, quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005d, quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005f, quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, PCI_ANY_ID,
-		quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_LSI_LOGIC, 0x0060, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_LSI_LOGIC, 0x007c, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_LSI_LOGIC, 0x0413, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_LSI_LOGIC, 0x0078, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_LSI_LOGIC, 0x0079, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_LSI_LOGIC, 0x0073, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_LSI_LOGIC, 0x0071, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_LSI_LOGIC, 0x005b, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_LSI_LOGIC, 0x002f, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_LSI_LOGIC, 0x005d, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_LSI_LOGIC, 0x005f, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATTANSIC, PCI_ANY_ID, quirk_blacklist_vpd);
 /*
  * The Amazon Annapurna Labs 0x0031 device id is reused for other non Root Port
  * device types, so 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_blacklist_vpd);
+DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS, 0x0031,
+			       PCI_CLASS_BRIDGE_PCI, 8, quirk_blacklist_vpd);
 
 static void quirk_chelsio_extend_vpd(struct pci_dev *dev)
 {
@@ -438,7 +434,7 @@ static void quirk_chelsio_extend_vpd(struct pci_dev *dev)
 		dev->vpd.len = 2048;
 }
 
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
-			quirk_chelsio_extend_vpd);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
+			 quirk_chelsio_extend_vpd);
 
 #endif
-- 
2.32.0

[PATCH 6/6] PCI/VPD: Treat invalid VPD like no VPD capability

[Heiner Kallweit]

Exporting sysfs files that can't be accessed doesn't make much sense.
Therefore, if either a quirk or the dynamic size calculation result
in VPD being marked as invalid, treat this like device has no VPD
capability. One consequence is that vpd sysfs file is not visible.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/vpd.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 9187ba496..58a3aa6a4 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -125,9 +125,6 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
 	if (pos < 0)
 		return -EINVAL;
 
-	if (vpd->len == PCI_VPD_SZ_INVALID)
-		return -EIO;
-
 	if (pos > vpd->len)
 		return 0;
 
@@ -189,9 +186,6 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count,
 	if (pos < 0 || (pos & 3) || (count & 3))
 		return -EINVAL;
 
-	if (vpd->len == PCI_VPD_SZ_INVALID)
-		return -EIO;
-
 	if (end > vpd->len)
 		return -EINVAL;
 
@@ -232,6 +226,9 @@ void pci_vpd_init(struct pci_dev *dev)
 
 	if (!dev->vpd.len)
 		dev->vpd.len = pci_vpd_size(dev);
+
+	if (dev->vpd.len == PCI_VPD_SZ_INVALID)
+		dev->vpd.cap = 0;
 }
 
 static ssize_t vpd_read(struct file *filp, struct kobject *kobj,
-- 
2.32.0

Re: [PATCH 2/6] PCI/VPD: Remove struct pci_vpd_ops

[Bjorn Helgaas]

[+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/
[2] https://lore.kernel.org/linux-pci/20150519160158.00002cd6@unknown/

> 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
> 
> 

Re: [PATCH 2/6] PCI/VPD: Remove struct pci_vpd_ops

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 626 bytes --]

at 3:00 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:

> 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].

If this is done, you'll have to be sure that any non-0 functions assigned  
to guests (which will appear as function 0 to the guest!) does not appear  
to have VPD and block access to those resources. That seems kind of messy,  
but should work. One hopes that no guest drivers are hard-wired to assume  
VPD presence based on the device type.

--
Mark Rustad (he/him), Ethernet Products Group, Intel Corporation

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH 2/6] PCI/VPD: Remove struct pci_vpd_ops

[Alex Williamson]

On Wed, 11 Aug 2021 23:43:43 +0000
"Rustad, Mark D" <mark.d.rustad@intel.com> wrote:

> at 3:00 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> > 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].  
> 
> If this is done, you'll have to be sure that any non-0 functions assigned  
> to guests (which will appear as function 0 to the guest!) does not appear  
> to have VPD and block access to those resources. That seems kind of messy,  
> but should work. One hopes that no guest drivers are hard-wired to assume  
> VPD presence based on the device type.

A bit messy, yes.  But if we're saying VPD is not essential for
operation in the VM, I'd rather we mask it on all functions rather than
create scenarios where different identical functions produce different
userspace results.  Thanks,

Alex

Re: [PATCH 0/6] PCI/VPD: Further improvements

[Bjorn Helgaas]

On Sun, Aug 08, 2021 at 07:18:08PM +0200, Heiner Kallweit wrote:
> This series includes a number of further improvements to VPD handling.
> 
> Heiner Kallweit (6):
>   PCI/VPD: Move pci_read/write_vpd in the code
>   PCI/VPD: Remove struct pci_vpd_ops
>   PCI/VPD: Remove member valid from struct pci_vpd
>   PCI/VPD: Embed struct pci_vpd member in struct pci_dev
>   PCI/VPD: Determine VPD size in pci_vpd_init already
>   PCI/VPD: Treat invalid VPD like no VPD capability
> 
>  drivers/pci/probe.c |   1 -
>  drivers/pci/vpd.c   | 253 ++++++++++++++++----------------------------
>  include/linux/pci.h |   9 +-
>  3 files changed, 97 insertions(+), 166 deletions(-)

This looks great!

Applied to pci/vpd for v5.15, thanks!

I tweaked 2/6 to test for func0_dev being NULL in pci_read_vpd()
instead of pci_vpd_read().  Could go either way, but it's really only
relevant for the PCI_DEV_FLAGS_VPD_REF_F0 case, which is now all in
pci_read_vpd().  For the non-PCI_DEV_FLAGS_VPD_REF_F0 case, we've
already dereference dev->dev_flags, so "dev" can never be NULL in
pci_vpd_read().