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
