Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Dave Jones <davej@codemonkey.org.uk>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org
References: <20210914112628.GA1412445@bjorn-Precision-5520>
From:   Heiner Kallweit <hkallweit1@gmail.com>
Subject: Re: Linux 5.15-rc1
Message-ID: <54bd54b9-3774-92a5-4193-5ccccd235572@gmail.com>
Date:   Tue, 14 Sep 2021 19:07:40 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <20210914112628.GA1412445@bjorn-Precision-5520>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 14.09.2021 13:26, Bjorn Helgaas wrote:
> On Tue, Sep 14, 2021 at 08:21:46AM +0200, Heiner Kallweit wrote:
>> On 14.09.2021 01:46, Bjorn Helgaas wrote:
> 
>>> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, PCI_ANY_ID, quirk_blacklist_vpd);
>>>  /*
>>
>> Leaving the quirks in FIXUP_HEADER stage would have the advantage that for
>> blacklisted devices the vpd sysfs attribute isn't visibale. The needed
>> changes to the patch are minimal.
> 
> What do you have in mind?  The only thing I can think of would be to
> add a "pci_dev.no_vpd" bit.  "vpd.cap == 0" means the device has no
> VPD, and "vpd.len == 0" means we haven't determined the size yet.  All
> devices start off with vpd.cap == 0 and vpd.len == 0, so a
> FIXUP_HEADER quirk would have to set a sentinel value or some other
> bit.
> 
> Bjorn
> 

Why not leave vpd.len == PCI_VPD_SZ_INVALID as sentinel?

And one more question: Why do you move the "if (!vpd->cap)" check from
pci_vpd_read() to pci_read_vpd()? At a first glance I see no benefit.

Here comes my version. Your changes to pci_vpd_size() I left as-is.
I tested the positive case and it works as expected.


diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 25557b272..04b14c488 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -52,7 +52,7 @@ static struct pci_dev *pci_get_func0_dev(struct pci_dev *dev)
  * pci_vpd_size - determine actual size of Vital Product Data
  * @dev:	pci device struct
  */
-static size_t pci_vpd_size(struct pci_dev *dev)
+static void pci_vpd_size(struct pci_dev *dev)
 {
 	size_t off = 0, size;
 	unsigned char tag, header[1+2];	/* 1 byte tag, 2 bytes length */
@@ -71,7 +71,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 ?: PCI_VPD_SZ_INVALID;
+				goto finish;
 			}
 			size = pci_vpd_lrdt_size(header);
 			if (off + size > PCI_VPD_MAX_SIZE)
@@ -87,16 +87,19 @@ static size_t pci_vpd_size(struct pci_dev *dev)
 
 			off += PCI_VPD_SRDT_TAG_SIZE + size;
 			if (tag == PCI_VPD_STIN_END)	/* End tag descriptor */
-				return off;
+				goto finish;
 		}
 	}
-	return off;
+	goto finish;
 
 error:
 	pci_info(dev, "invalid VPD tag %#04x (size %zu) at offset %zu%s\n",
 		 header[0], size, off, off == 0 ?
 		 "; assume missing optional EEPROM" : "");
-	return off ?: PCI_VPD_SZ_INVALID;
+finish:
+	dev->vpd.len = off;
+	if (off == 0)
+		dev->vpd.cap = 0;		/* No VPD at all */
 }
 
 /*
@@ -145,6 +148,8 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
 	loff_t end = pos + count;
 	u8 *buf = arg;
 
+	if (vpd->len == 0 && vpd->cap)
+		pci_vpd_size(dev);
 	if (!vpd->cap)
 		return -ENODEV;
 
@@ -206,6 +211,8 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count,
 	loff_t end = pos + count;
 	int ret = 0;
 
+	if (vpd->len == 0 && vpd->cap)
+		pci_vpd_size(dev);
 	if (!vpd->cap)
 		return -ENODEV;
 
@@ -245,9 +252,6 @@ 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);
-
 	if (dev->vpd.len == PCI_VPD_SZ_INVALID)
 		dev->vpd.cap = 0;
 }
@@ -294,25 +298,27 @@ const struct attribute_group pci_dev_vpd_attr_group = {
 
 void *pci_vpd_alloc(struct pci_dev *dev, unsigned int *size)
 {
-	unsigned int len = dev->vpd.len;
+	struct pci_vpd *vpd = &dev->vpd;
 	void *buf;
 	int cnt;
 
-	if (!dev->vpd.cap)
+	if (vpd->len == 0 && vpd->cap)
+		pci_vpd_size(dev);
+	if (!vpd->cap)
 		return ERR_PTR(-ENODEV);
 
-	buf = kmalloc(len, GFP_KERNEL);
+	buf = kmalloc(vpd->len, GFP_KERNEL);
 	if (!buf)
 		return ERR_PTR(-ENOMEM);
 
-	cnt = pci_read_vpd(dev, 0, len, buf);
-	if (cnt != len) {
+	cnt = pci_read_vpd(dev, 0, vpd->len, buf);
+	if (cnt != vpd->len) {
 		kfree(buf);
 		return ERR_PTR(-EIO);
 	}
 
 	if (size)
-		*size = len;
+		*size = vpd->len;
 
 	return buf;
 }
-- 
2.33.0