[PATCH V4 0/2] pci: Provide a flag to access VPD through function 0

[PATCH V4 0/2] pci: Provide a flag to access VPD through function 0

[Mark D Rustad]

Many multi-function devices provide shared registers in extended
config space for accessing VPD. The behavior of these registers
means that the state must be tracked and access locked correctly
for accesses not to hang or worse. One way to meet these needs is
to always perform the accesses through function 0, thereby using
the state tracking and mutex that already exists.

To provide this behavior, add a dev_flags bit to indicate that this
should be done. This bit can then be set for any non-zero function
that needs to redirect such VPD access to function 0. Do not set
this bit on the zero function or there will be an infinite recursion.

The second patch uses this new flag to invoke this behavior on all
multi-function Intel Ethernet devices.

Any hardware that shares VPD registers with multiple functions has
been suffering these problems forever. The hangs result in the log
message:

vpd r/w failed.  This is likely a firmware bug on this device.

Both read and write data corruption are also possible during
overlapping accesses in addition to hangs.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>

---
Changes in V2:
- Corrected a spelling error in a log message
- Added checks to see that the referenced function 0 is reasonable
Changes in V3:
- Don't leak a device reference
- Check that function 0 has VPD
- Make a helper for the function 0 checks
- Moved a multifunction check to the quirk patch
Changes in V4:
- Provide a more extensive commit log for patch 1

---

Mark Rustad (2):
      pci: Add dev_flags bit to access VPD through function 0
      pci: Add VPD quirk for Intel Ethernet devices


 drivers/pci/access.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/pci/quirks.c |    9 +++++++
 include/linux/pci.h  |    2 ++
 3 files changed, 71 insertions(+), 1 deletion(-)

-- 
Mark Rustad, Network Division, Intel Corporation

[Intel-wired-lan] [PATCH V4 0/2] pci: Provide a flag to access VPD through function 0

[Mark D Rustad]

Many multi-function devices provide shared registers in extended
config space for accessing VPD. The behavior of these registers
means that the state must be tracked and access locked correctly
for accesses not to hang or worse. One way to meet these needs is
to always perform the accesses through function 0, thereby using
the state tracking and mutex that already exists.

To provide this behavior, add a dev_flags bit to indicate that this
should be done. This bit can then be set for any non-zero function
that needs to redirect such VPD access to function 0. Do not set
this bit on the zero function or there will be an infinite recursion.

The second patch uses this new flag to invoke this behavior on all
multi-function Intel Ethernet devices.

Any hardware that shares VPD registers with multiple functions has
been suffering these problems forever. The hangs result in the log
message:

vpd r/w failed.  This is likely a firmware bug on this device.

Both read and write data corruption are also possible during
overlapping accesses in addition to hangs.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>

---
Changes in V2:
- Corrected a spelling error in a log message
- Added checks to see that the referenced function 0 is reasonable
Changes in V3:
- Don't leak a device reference
- Check that function 0 has VPD
- Make a helper for the function 0 checks
- Moved a multifunction check to the quirk patch
Changes in V4:
- Provide a more extensive commit log for patch 1

---

Mark Rustad (2):
      pci: Add dev_flags bit to access VPD through function 0
      pci: Add VPD quirk for Intel Ethernet devices


 drivers/pci/access.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/pci/quirks.c |    9 +++++++
 include/linux/pci.h  |    2 ++
 3 files changed, 71 insertions(+), 1 deletion(-)

-- 
Mark Rustad, Network Division, Intel Corporation

[PATCH V4 1/2] pci: Add dev_flags bit to access VPD through function 0

[Mark D Rustad]

From: Mark Rustad <mark.d.rustad@intel.com>

Add a dev_flags bit, PCI_DEV_FLAGS_VPD_REF_F0, to access VPD through
function 0 to provide VPD access on other functions. This is for
hardware devices that provide copies of the same VPD capability
registers in multiple functions. Because the kernel expects that
each function has its own registers, both the locking and the state
tracking are affected by VPD accesses to different functions.

On such devices for example, if a VPD write is performed on function
0, *any* later attempt to read VPD from any other function of that
device will hang. This has to do with how the kernel tracks the
expected value of the F bit per function.

Concurrent accesses to different functions of the same device can
not only hang but also corrupt both read and write VPD data.

When hangs occur, typically the error message:

vpd r/w failed.  This is likely a firmware bug on this device.

will be seen.

Never set this bit on function 0 or there will be an infinite recursion.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
---
Changes in V2:
- Corrected spelling in log message
- Added checks to see that the referenced function 0 is reasonable
Changes in V3:
- Don't leak a device reference
- Check that function 0 has VPD
- Make a helper for the function 0 checks
- Do multifunction check in the quirk
Changes in V4:
- Provide a much more detailed explanation in the commit log
---
 drivers/pci/access.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/pci.h  |    2 ++
 2 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index d9b64a175990..b965c12168b7 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -439,6 +439,56 @@ static const struct pci_vpd_ops pci_vpd_pci22_ops = {
 	.release = pci_vpd_pci22_release,
 };
 
+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_slot(dev->bus, PCI_SLOT(dev->devfn));
+	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_slot(dev->bus, PCI_SLOT(dev->devfn));
+	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,
+	.release = pci_vpd_pci22_release,
+};
+
+static int pci_vpd_f0_dev_check(struct pci_dev *dev)
+{
+	struct pci_dev *tdev = pci_get_slot(dev->bus, PCI_SLOT(dev->devfn));
+	int ret = 0;
+
+	if (!tdev)
+		return -ENODEV;
+	if (!tdev->vpd || !tdev->multifunction ||
+	    dev->class != tdev->class || dev->vendor != tdev->vendor ||
+	    dev->device != tdev->device)
+		ret = -ENODEV;
+
+	pci_dev_put(tdev);
+	return ret;
+}
+
 int pci_vpd_pci22_init(struct pci_dev *dev)
 {
 	struct pci_vpd_pci22 *vpd;
@@ -447,12 +497,21 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
 	cap = pci_find_capability(dev, PCI_CAP_ID_VPD);
 	if (!cap)
 		return -ENODEV;
+	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0) {
+		int ret = pci_vpd_f0_dev_check(dev);
+
+		if (ret)
+			return ret;
+	}
 	vpd = kzalloc(sizeof(*vpd), GFP_ATOMIC);
 	if (!vpd)
 		return -ENOMEM;
 
 	vpd->base.len = PCI_VPD_PCI22_SIZE;
-	vpd->base.ops = &pci_vpd_pci22_ops;
+	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0)
+		vpd->base.ops = &pci_vpd_f0_ops;
+	else
+		vpd->base.ops = &pci_vpd_pci22_ops;
 	mutex_init(&vpd->lock);
 	vpd->cap = cap;
 	vpd->busy = false;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8a0321a8fb59..8edb125db13a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -180,6 +180,8 @@ enum pci_dev_flags {
 	PCI_DEV_FLAGS_NO_BUS_RESET = (__force pci_dev_flags_t) (1 << 6),
 	/* Do not use PM reset even if device advertises NoSoftRst- */
 	PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
+	/* Get VPD from function 0 VPD */
+	PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
 };
 
 enum pci_irq_reroute_variant {

[Intel-wired-lan] [PATCH V4 1/2] pci: Add dev_flags bit to access VPD through function 0

[Mark D Rustad]

From: Mark Rustad <mark.d.rustad@intel.com>

Add a dev_flags bit, PCI_DEV_FLAGS_VPD_REF_F0, to access VPD through
function 0 to provide VPD access on other functions. This is for
hardware devices that provide copies of the same VPD capability
registers in multiple functions. Because the kernel expects that
each function has its own registers, both the locking and the state
tracking are affected by VPD accesses to different functions.

On such devices for example, if a VPD write is performed on function
0, *any* later attempt to read VPD from any other function of that
device will hang. This has to do with how the kernel tracks the
expected value of the F bit per function.

Concurrent accesses to different functions of the same device can
not only hang but also corrupt both read and write VPD data.

When hangs occur, typically the error message:

vpd r/w failed.  This is likely a firmware bug on this device.

will be seen.

Never set this bit on function 0 or there will be an infinite recursion.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
---
Changes in V2:
- Corrected spelling in log message
- Added checks to see that the referenced function 0 is reasonable
Changes in V3:
- Don't leak a device reference
- Check that function 0 has VPD
- Make a helper for the function 0 checks
- Do multifunction check in the quirk
Changes in V4:
- Provide a much more detailed explanation in the commit log
---
 drivers/pci/access.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/pci.h  |    2 ++
 2 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index d9b64a175990..b965c12168b7 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -439,6 +439,56 @@ static const struct pci_vpd_ops pci_vpd_pci22_ops = {
 	.release = pci_vpd_pci22_release,
 };
 
+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_slot(dev->bus, PCI_SLOT(dev->devfn));
+	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_slot(dev->bus, PCI_SLOT(dev->devfn));
+	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,
+	.release = pci_vpd_pci22_release,
+};
+
+static int pci_vpd_f0_dev_check(struct pci_dev *dev)
+{
+	struct pci_dev *tdev = pci_get_slot(dev->bus, PCI_SLOT(dev->devfn));
+	int ret = 0;
+
+	if (!tdev)
+		return -ENODEV;
+	if (!tdev->vpd || !tdev->multifunction ||
+	    dev->class != tdev->class || dev->vendor != tdev->vendor ||
+	    dev->device != tdev->device)
+		ret = -ENODEV;
+
+	pci_dev_put(tdev);
+	return ret;
+}
+
 int pci_vpd_pci22_init(struct pci_dev *dev)
 {
 	struct pci_vpd_pci22 *vpd;
@@ -447,12 +497,21 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
 	cap = pci_find_capability(dev, PCI_CAP_ID_VPD);
 	if (!cap)
 		return -ENODEV;
+	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0) {
+		int ret = pci_vpd_f0_dev_check(dev);
+
+		if (ret)
+			return ret;
+	}
 	vpd = kzalloc(sizeof(*vpd), GFP_ATOMIC);
 	if (!vpd)
 		return -ENOMEM;
 
 	vpd->base.len = PCI_VPD_PCI22_SIZE;
-	vpd->base.ops = &pci_vpd_pci22_ops;
+	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0)
+		vpd->base.ops = &pci_vpd_f0_ops;
+	else
+		vpd->base.ops = &pci_vpd_pci22_ops;
 	mutex_init(&vpd->lock);
 	vpd->cap = cap;
 	vpd->busy = false;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8a0321a8fb59..8edb125db13a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -180,6 +180,8 @@ enum pci_dev_flags {
 	PCI_DEV_FLAGS_NO_BUS_RESET = (__force pci_dev_flags_t) (1 << 6),
 	/* Do not use PM reset even if device advertises NoSoftRst- */
 	PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
+	/* Get VPD from function 0 VPD */
+	PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
 };
 
 enum pci_irq_reroute_variant {

[PATCH V4 2/2] pci: Add VPD quirk for Intel Ethernet devices

[Mark D Rustad]

From: Mark Rustad <mark.d.rustad@intel.com>

This quirk sets the PCI_DEV_FLAGS_VPD_REF_F0 flag on all Intel
Ethernet device functions other than function 0.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
---
Changes in V3:
- Added a multifunction device check
---
 drivers/pci/quirks.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index e9fd0e90fa3b..08c04e4f5ab2 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1894,6 +1894,15 @@ static void quirk_netmos(struct pci_dev *dev)
 DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_NETMOS, PCI_ANY_ID,
 			 PCI_CLASS_COMMUNICATION_SERIAL, 8, quirk_netmos);
 
+static void quirk_f0_vpd_link(struct pci_dev *dev)
+{
+	if (!dev->multifunction || !PCI_FUNC(dev->devfn))
+		return;
+	dev->dev_flags |= PCI_DEV_FLAGS_VPD_REF_F0;
+}
+DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
+			      PCI_CLASS_NETWORK_ETHERNET, 8, quirk_f0_vpd_link);
+
 static void quirk_e100_interrupt(struct pci_dev *dev)
 {
 	u16 command, pmcsr;

[Intel-wired-lan] [PATCH V4 2/2] pci: Add VPD quirk for Intel Ethernet devices

[Mark D Rustad]

From: Mark Rustad <mark.d.rustad@intel.com>

This quirk sets the PCI_DEV_FLAGS_VPD_REF_F0 flag on all Intel
Ethernet device functions other than function 0.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
---
Changes in V3:
- Added a multifunction device check
---
 drivers/pci/quirks.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index e9fd0e90fa3b..08c04e4f5ab2 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1894,6 +1894,15 @@ static void quirk_netmos(struct pci_dev *dev)
 DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_NETMOS, PCI_ANY_ID,
 			 PCI_CLASS_COMMUNICATION_SERIAL, 8, quirk_netmos);
 
+static void quirk_f0_vpd_link(struct pci_dev *dev)
+{
+	if (!dev->multifunction || !PCI_FUNC(dev->devfn))
+		return;
+	dev->dev_flags |= PCI_DEV_FLAGS_VPD_REF_F0;
+}
+DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
+			      PCI_CLASS_NETWORK_ETHERNET, 8, quirk_f0_vpd_link);
+
 static void quirk_e100_interrupt(struct pci_dev *dev)
 {
 	u16 command, pmcsr;

Re: [PATCH V4 0/2] pci: Provide a flag to access VPD through function 0

[Bjorn Helgaas]

[+cc Alex]

On Mon, Jul 13, 2015 at 11:39:54AM -0700, Mark D Rustad wrote:
> Many multi-function devices provide shared registers in extended
> config space for accessing VPD. The behavior of these registers
> means that the state must be tracked and access locked correctly
> for accesses not to hang or worse. One way to meet these needs is
> to always perform the accesses through function 0, thereby using
> the state tracking and mutex that already exists.
> 
> To provide this behavior, add a dev_flags bit to indicate that this
> should be done. This bit can then be set for any non-zero function
> that needs to redirect such VPD access to function 0. Do not set
> this bit on the zero function or there will be an infinite recursion.
> 
> The second patch uses this new flag to invoke this behavior on all
> multi-function Intel Ethernet devices.
> 
> Any hardware that shares VPD registers with multiple functions has
> been suffering these problems forever. The hangs result in the log
> message:
> 
> vpd r/w failed.  This is likely a firmware bug on this device.
> 
> Both read and write data corruption are also possible during
> overlapping accesses in addition to hangs.
> 
> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
> 
> ---
> Changes in V2:
> - Corrected a spelling error in a log message
> - Added checks to see that the referenced function 0 is reasonable
> Changes in V3:
> - Don't leak a device reference
> - Check that function 0 has VPD
> - Make a helper for the function 0 checks
> - Moved a multifunction check to the quirk patch
> Changes in V4:
> - Provide a more extensive commit log for patch 1

I applied these to pci/misc for v4.3 with changelogs as follows.  I added
Alex's ack, since he acked v3 and the only difference here is the
changelog.  I also added a stable tag.  Thanks!

Bjorn


commit 932c435caba8a2ce473a91753bad0173269ef334
Author: Mark Rustad <mark.d.rustad@intel.com>
Date:   Mon Jul 13 11:40:02 2015 -0700

    PCI: Add dev_flags bit to access VPD through function 0
    
    Add a dev_flags bit, PCI_DEV_FLAGS_VPD_REF_F0, to access VPD through
    function 0 to provide VPD access on other functions.  This is for hardware
    devices that provide copies of the same VPD capability registers in
    multiple functions.  Because the kernel expects that each function has its
    own registers, both the locking and the state tracking are affected by VPD
    accesses to different functions.
    
    On such devices for example, if a VPD write is performed on function 0,
    *any* later attempt to read VPD from any other function of that device will
    hang.  This has to do with how the kernel tracks the expected value of the
    F bit per function.
    
    Concurrent accesses to different functions of the same device can not only
    hang but also corrupt both read and write VPD data.
    
    When hangs occur, typically the error message:
    
      vpd r/w failed.  This is likely a firmware bug on this device.
    
    will be seen.
    
    Never set this bit on function 0 or there will be an infinite recursion.
    
    Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Acked-by: Alexander Duyck <alexander.h.duyck@redhat.com>
    CC: stable@vger.kernel.org

commit 7aa6ca4d39edf01f997b9e02cf6d2fdeb224f351
Author: Mark Rustad <mark.d.rustad@intel.com>
Date:   Mon Jul 13 11:40:07 2015 -0700

    PCI: Add VPD function 0 quirk for Intel Ethernet devices
    
    Set the PCI_DEV_FLAGS_VPD_REF_F0 flag on all Intel Ethernet device
    functions other than function 0, so that on multi-function devices, we will
    always read VPD from function 0 instead of from the other functions.
    
    [bhelgaas: changelog]
    Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Acked-by: Alexander Duyck <alexander.h.duyck@redhat.com>
    CC: stable@vger.kernel.org

[Intel-wired-lan] [PATCH V4 0/2] pci: Provide a flag to access VPD through function 0

[Bjorn Helgaas]

[+cc Alex]

On Mon, Jul 13, 2015 at 11:39:54AM -0700, Mark D Rustad wrote:
> Many multi-function devices provide shared registers in extended
> config space for accessing VPD. The behavior of these registers
> means that the state must be tracked and access locked correctly
> for accesses not to hang or worse. One way to meet these needs is
> to always perform the accesses through function 0, thereby using
> the state tracking and mutex that already exists.
> 
> To provide this behavior, add a dev_flags bit to indicate that this
> should be done. This bit can then be set for any non-zero function
> that needs to redirect such VPD access to function 0. Do not set
> this bit on the zero function or there will be an infinite recursion.
> 
> The second patch uses this new flag to invoke this behavior on all
> multi-function Intel Ethernet devices.
> 
> Any hardware that shares VPD registers with multiple functions has
> been suffering these problems forever. The hangs result in the log
> message:
> 
> vpd r/w failed.  This is likely a firmware bug on this device.
> 
> Both read and write data corruption are also possible during
> overlapping accesses in addition to hangs.
> 
> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
> 
> ---
> Changes in V2:
> - Corrected a spelling error in a log message
> - Added checks to see that the referenced function 0 is reasonable
> Changes in V3:
> - Don't leak a device reference
> - Check that function 0 has VPD
> - Make a helper for the function 0 checks
> - Moved a multifunction check to the quirk patch
> Changes in V4:
> - Provide a more extensive commit log for patch 1

I applied these to pci/misc for v4.3 with changelogs as follows.  I added
Alex's ack, since he acked v3 and the only difference here is the
changelog.  I also added a stable tag.  Thanks!

Bjorn


commit 932c435caba8a2ce473a91753bad0173269ef334
Author: Mark Rustad <mark.d.rustad@intel.com>
Date:   Mon Jul 13 11:40:02 2015 -0700

    PCI: Add dev_flags bit to access VPD through function 0
    
    Add a dev_flags bit, PCI_DEV_FLAGS_VPD_REF_F0, to access VPD through
    function 0 to provide VPD access on other functions.  This is for hardware
    devices that provide copies of the same VPD capability registers in
    multiple functions.  Because the kernel expects that each function has its
    own registers, both the locking and the state tracking are affected by VPD
    accesses to different functions.
    
    On such devices for example, if a VPD write is performed on function 0,
    *any* later attempt to read VPD from any other function of that device will
    hang.  This has to do with how the kernel tracks the expected value of the
    F bit per function.
    
    Concurrent accesses to different functions of the same device can not only
    hang but also corrupt both read and write VPD data.
    
    When hangs occur, typically the error message:
    
      vpd r/w failed.  This is likely a firmware bug on this device.
    
    will be seen.
    
    Never set this bit on function 0 or there will be an infinite recursion.
    
    Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Acked-by: Alexander Duyck <alexander.h.duyck@redhat.com>
    CC: stable at vger.kernel.org

commit 7aa6ca4d39edf01f997b9e02cf6d2fdeb224f351
Author: Mark Rustad <mark.d.rustad@intel.com>
Date:   Mon Jul 13 11:40:07 2015 -0700

    PCI: Add VPD function 0 quirk for Intel Ethernet devices
    
    Set the PCI_DEV_FLAGS_VPD_REF_F0 flag on all Intel Ethernet device
    functions other than function 0, so that on multi-function devices, we will
    always read VPD from function 0 instead of from the other functions.
    
    [bhelgaas: changelog]
    Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Acked-by: Alexander Duyck <alexander.h.duyck@redhat.com>
    CC: stable at vger.kernel.org

Re: [PATCH V4 1/2] pci: Add dev_flags bit to access VPD through function 0

[Alex Williamson]

On Mon, 2015-07-13 at 11:40 -0700, Mark D Rustad wrote:
> From: Mark Rustad <mark.d.rustad@intel.com>
> 
> Add a dev_flags bit, PCI_DEV_FLAGS_VPD_REF_F0, to access VPD through
> function 0 to provide VPD access on other functions. This is for
> hardware devices that provide copies of the same VPD capability
> registers in multiple functions. Because the kernel expects that
> each function has its own registers, both the locking and the state
> tracking are affected by VPD accesses to different functions.
> 
> On such devices for example, if a VPD write is performed on function
> 0, *any* later attempt to read VPD from any other function of that
> device will hang. This has to do with how the kernel tracks the
> expected value of the F bit per function.
> 
> Concurrent accesses to different functions of the same device can
> not only hang but also corrupt both read and write VPD data.
> 
> When hangs occur, typically the error message:
> 
> vpd r/w failed.  This is likely a firmware bug on this device.
> 
> will be seen.
> 
> Never set this bit on function 0 or there will be an infinite recursion.
> 
> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
> ---
> Changes in V2:
> - Corrected spelling in log message
> - Added checks to see that the referenced function 0 is reasonable
> Changes in V3:
> - Don't leak a device reference
> - Check that function 0 has VPD
> - Make a helper for the function 0 checks
> - Do multifunction check in the quirk
> Changes in V4:
> - Provide a much more detailed explanation in the commit log
> ---
>  drivers/pci/access.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/pci.h  |    2 ++
>  2 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index d9b64a175990..b965c12168b7 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -439,6 +439,56 @@ static const struct pci_vpd_ops pci_vpd_pci22_ops = {
>  	.release = pci_vpd_pci22_release,
>  };
>  
> +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_slot(dev->bus, PCI_SLOT(dev->devfn));
> +	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_slot(dev->bus, PCI_SLOT(dev->devfn));
> +	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,
> +	.release = pci_vpd_pci22_release,
> +};
> +
> +static int pci_vpd_f0_dev_check(struct pci_dev *dev)
> +{
> +	struct pci_dev *tdev = pci_get_slot(dev->bus, PCI_SLOT(dev->devfn));
> +	int ret = 0;
> +
> +	if (!tdev)
> +		return -ENODEV;
> +	if (!tdev->vpd || !tdev->multifunction ||
> +	    dev->class != tdev->class || dev->vendor != tdev->vendor ||
> +	    dev->device != tdev->device)
> +		ret = -ENODEV;
> +
> +	pci_dev_put(tdev);
> +	return ret;
> +}
> +
>  int pci_vpd_pci22_init(struct pci_dev *dev)
>  {
>  	struct pci_vpd_pci22 *vpd;
> @@ -447,12 +497,21 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
>  	cap = pci_find_capability(dev, PCI_CAP_ID_VPD);
>  	if (!cap)
>  		return -ENODEV;
> +	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0) {
> +		int ret = pci_vpd_f0_dev_check(dev);
> +
> +		if (ret)
> +			return ret;
> +	}


In addition to the (PCI_SLOT() != devfn) issue, I'm concerned about
topologies like we see on Skylake.  IIRC, the integrated NIC appears at
something like 00:1f.6.  I don't know if that specific NIC has VPD, nor
am I sure it really matter because another example or some future
version might.  So we'll set the PCI_DEV_FLAGS_VPD_REF_F0 because we do
so for all (PCI_FUNC() != 0) Intel NICs, we'll call
pci_vpd_f0_dev_check(), which will error because function 0 has a
different class code and device ID, so we return error and if VPD exists
on the device, it's now inaccessible.

I thought there was talk about whitelisting anything on the root bus to
avoid strange root complex integrated devices (and perhaps avoid the
general case for assigned devices within a VM), but I don't see anything
like that here.

Perhaps instead of failing and hiding VPD we should fail, clear the
flag, and allow normal access.  Thanks,

Alex

>  	vpd = kzalloc(sizeof(*vpd), GFP_ATOMIC);
>  	if (!vpd)
>  		return -ENOMEM;
>  
>  	vpd->base.len = PCI_VPD_PCI22_SIZE;
> -	vpd->base.ops = &pci_vpd_pci22_ops;
> +	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0)
> +		vpd->base.ops = &pci_vpd_f0_ops;
> +	else
> +		vpd->base.ops = &pci_vpd_pci22_ops;
>  	mutex_init(&vpd->lock);
>  	vpd->cap = cap;
>  	vpd->busy = false;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8a0321a8fb59..8edb125db13a 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -180,6 +180,8 @@ enum pci_dev_flags {
>  	PCI_DEV_FLAGS_NO_BUS_RESET = (__force pci_dev_flags_t) (1 << 6),
>  	/* Do not use PM reset even if device advertises NoSoftRst- */
>  	PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
> +	/* Get VPD from function 0 VPD */
> +	PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
>  };
>  
>  enum pci_irq_reroute_variant {
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Intel-wired-lan] [PATCH V4 1/2] pci: Add dev_flags bit to access VPD through function 0

[Alex Williamson]

On Mon, 2015-07-13 at 11:40 -0700, Mark D Rustad wrote:
> From: Mark Rustad <mark.d.rustad@intel.com>
> 
> Add a dev_flags bit, PCI_DEV_FLAGS_VPD_REF_F0, to access VPD through
> function 0 to provide VPD access on other functions. This is for
> hardware devices that provide copies of the same VPD capability
> registers in multiple functions. Because the kernel expects that
> each function has its own registers, both the locking and the state
> tracking are affected by VPD accesses to different functions.
> 
> On such devices for example, if a VPD write is performed on function
> 0, *any* later attempt to read VPD from any other function of that
> device will hang. This has to do with how the kernel tracks the
> expected value of the F bit per function.
> 
> Concurrent accesses to different functions of the same device can
> not only hang but also corrupt both read and write VPD data.
> 
> When hangs occur, typically the error message:
> 
> vpd r/w failed.  This is likely a firmware bug on this device.
> 
> will be seen.
> 
> Never set this bit on function 0 or there will be an infinite recursion.
> 
> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
> ---
> Changes in V2:
> - Corrected spelling in log message
> - Added checks to see that the referenced function 0 is reasonable
> Changes in V3:
> - Don't leak a device reference
> - Check that function 0 has VPD
> - Make a helper for the function 0 checks
> - Do multifunction check in the quirk
> Changes in V4:
> - Provide a much more detailed explanation in the commit log
> ---
>  drivers/pci/access.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/pci.h  |    2 ++
>  2 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index d9b64a175990..b965c12168b7 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -439,6 +439,56 @@ static const struct pci_vpd_ops pci_vpd_pci22_ops = {
>  	.release = pci_vpd_pci22_release,
>  };
>  
> +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_slot(dev->bus, PCI_SLOT(dev->devfn));
> +	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_slot(dev->bus, PCI_SLOT(dev->devfn));
> +	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,
> +	.release = pci_vpd_pci22_release,
> +};
> +
> +static int pci_vpd_f0_dev_check(struct pci_dev *dev)
> +{
> +	struct pci_dev *tdev = pci_get_slot(dev->bus, PCI_SLOT(dev->devfn));
> +	int ret = 0;
> +
> +	if (!tdev)
> +		return -ENODEV;
> +	if (!tdev->vpd || !tdev->multifunction ||
> +	    dev->class != tdev->class || dev->vendor != tdev->vendor ||
> +	    dev->device != tdev->device)
> +		ret = -ENODEV;
> +
> +	pci_dev_put(tdev);
> +	return ret;
> +}
> +
>  int pci_vpd_pci22_init(struct pci_dev *dev)
>  {
>  	struct pci_vpd_pci22 *vpd;
> @@ -447,12 +497,21 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
>  	cap = pci_find_capability(dev, PCI_CAP_ID_VPD);
>  	if (!cap)
>  		return -ENODEV;
> +	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0) {
> +		int ret = pci_vpd_f0_dev_check(dev);
> +
> +		if (ret)
> +			return ret;
> +	}


In addition to the (PCI_SLOT() != devfn) issue, I'm concerned about
topologies like we see on Skylake.  IIRC, the integrated NIC appears at
something like 00:1f.6.  I don't know if that specific NIC has VPD, nor
am I sure it really matter because another example or some future
version might.  So we'll set the PCI_DEV_FLAGS_VPD_REF_F0 because we do
so for all (PCI_FUNC() != 0) Intel NICs, we'll call
pci_vpd_f0_dev_check(), which will error because function 0 has a
different class code and device ID, so we return error and if VPD exists
on the device, it's now inaccessible.

I thought there was talk about whitelisting anything on the root bus to
avoid strange root complex integrated devices (and perhaps avoid the
general case for assigned devices within a VM), but I don't see anything
like that here.

Perhaps instead of failing and hiding VPD we should fail, clear the
flag, and allow normal access.  Thanks,

Alex

>  	vpd = kzalloc(sizeof(*vpd), GFP_ATOMIC);
>  	if (!vpd)
>  		return -ENOMEM;
>  
>  	vpd->base.len = PCI_VPD_PCI22_SIZE;
> -	vpd->base.ops = &pci_vpd_pci22_ops;
> +	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0)
> +		vpd->base.ops = &pci_vpd_f0_ops;
> +	else
> +		vpd->base.ops = &pci_vpd_pci22_ops;
>  	mutex_init(&vpd->lock);
>  	vpd->cap = cap;
>  	vpd->busy = false;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8a0321a8fb59..8edb125db13a 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -180,6 +180,8 @@ enum pci_dev_flags {
>  	PCI_DEV_FLAGS_NO_BUS_RESET = (__force pci_dev_flags_t) (1 << 6),
>  	/* Do not use PM reset even if device advertises NoSoftRst- */
>  	PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
> +	/* Get VPD from function 0 VPD */
> +	PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
>  };
>  
>  enum pci_irq_reroute_variant {
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4 1/2] pci: Add dev_flags bit to access VPD through function 0

[Rustad, Mark D]

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

> On Sep 15, 2015, at 11:19 AM, Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> In addition to the (PCI_SLOT() != devfn) issue, I'm concerned about
> topologies like we see on Skylake.  IIRC, the integrated NIC appears at
> something like 00:1f.6.  I don't know if that specific NIC has VPD, nor
> am I sure it really matter because another example or some future
> version might.  So we'll set the PCI_DEV_FLAGS_VPD_REF_F0 because we do
> so for all (PCI_FUNC() != 0) Intel NICs, we'll call
> pci_vpd_f0_dev_check(), which will error because function 0 has a
> different class code and device ID, so we return error and if VPD exists
> on the device, it's now inaccessible.

Yes, that is exactly what would happen.

> I thought there was talk about whitelisting anything on the root bus to
> avoid strange root complex integrated devices (and perhaps avoid the
> general case for assigned devices within a VM), but I don't see anything
> like that here.

I hadn't heard that talk, but I'm not on the PCI list and I guess I wasn't copied.

> Perhaps instead of failing and hiding VPD we should fail, clear the
> flag, and allow normal access.  Thanks,

Because the purpose of VPD is to hold information about the device, I would suggest that VPD should never be provided for an embedded network device, but rather for the device as a whole. So while there may well be VPD for an SOC, that VPD should not be associated with one of its embedded devices, but rather something more appropriate for the device as a whole. And attaching VPD to a whole bunch of internal devices would just be madness.

So I understand the concern, but I don't think that it should really happen in real systems. I did think about this case when I was working on the patches. A networking device should really only have VPD when it is its own physical device, such as a NIC.

--
Mark Rustad, Networking Division, Intel Corporation


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

[Intel-wired-lan] [PATCH V4 1/2] pci: Add dev_flags bit to access VPD through function 0

[Rustad, Mark D]

> On Sep 15, 2015, at 11:19 AM, Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> In addition to the (PCI_SLOT() != devfn) issue, I'm concerned about
> topologies like we see on Skylake.  IIRC, the integrated NIC appears at
> something like 00:1f.6.  I don't know if that specific NIC has VPD, nor
> am I sure it really matter because another example or some future
> version might.  So we'll set the PCI_DEV_FLAGS_VPD_REF_F0 because we do
> so for all (PCI_FUNC() != 0) Intel NICs, we'll call
> pci_vpd_f0_dev_check(), which will error because function 0 has a
> different class code and device ID, so we return error and if VPD exists
> on the device, it's now inaccessible.

Yes, that is exactly what would happen.

> I thought there was talk about whitelisting anything on the root bus to
> avoid strange root complex integrated devices (and perhaps avoid the
> general case for assigned devices within a VM), but I don't see anything
> like that here.

I hadn't heard that talk, but I'm not on the PCI list and I guess I wasn't copied.

> Perhaps instead of failing and hiding VPD we should fail, clear the
> flag, and allow normal access.  Thanks,

Because the purpose of VPD is to hold information about the device, I would suggest that VPD should never be provided for an embedded network device, but rather for the device as a whole. So while there may well be VPD for an SOC, that VPD should not be associated with one of its embedded devices, but rather something more appropriate for the device as a whole. And attaching VPD to a whole bunch of internal devices would just be madness.

So I understand the concern, but I don't think that it should really happen in real systems. I did think about this case when I was working on the patches. A networking device should really only have VPD when it is its own physical device, such as a NIC.

--
Mark Rustad, Networking Division, Intel Corporation

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 841 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20150915/fea2b91e/attachment.asc>

Re: [PATCH V4 1/2] pci: Add dev_flags bit to access VPD through function 0

[Alex Williamson]

On Tue, 2015-09-15 at 18:39 +0000, Rustad, Mark D wrote:
> > On Sep 15, 2015, at 11:19 AM, Alex Williamson <alex.williamson@redhat.com> wrote:
> > 
> > In addition to the (PCI_SLOT() != devfn) issue, I'm concerned about
> > topologies like we see on Skylake.  IIRC, the integrated NIC appears at
> > something like 00:1f.6.  I don't know if that specific NIC has VPD, nor
> > am I sure it really matter because another example or some future
> > version might.  So we'll set the PCI_DEV_FLAGS_VPD_REF_F0 because we do
> > so for all (PCI_FUNC() != 0) Intel NICs, we'll call
> > pci_vpd_f0_dev_check(), which will error because function 0 has a
> > different class code and device ID, so we return error and if VPD exists
> > on the device, it's now inaccessible.
> 
> Yes, that is exactly what would happen.
> 
> > I thought there was talk about whitelisting anything on the root bus to
> > avoid strange root complex integrated devices (and perhaps avoid the
> > general case for assigned devices within a VM), but I don't see anything
> > like that here.
> 
> I hadn't heard that talk, but I'm not on the PCI list and I guess I
> wasn't copied.
> 
> > Perhaps instead of failing and hiding VPD we should fail, clear the
> > flag, and allow normal access.  Thanks,
> 
> Because the purpose of VPD is to hold information about the device, I
> would suggest that VPD should never be provided for an embedded
> network device, but rather for the device as a whole. So while there
> may well be VPD for an SOC, that VPD should not be associated with one
> of its embedded devices, but rather something more appropriate for the
> device as a whole. And attaching VPD to a whole bunch of internal
> devices would just be madness.

FRU-type information is only one of the use cases of VPD, the spec also
defines (PCI rev 3.0, 6.4):

        ... a mechanism for storing information such as performance and
        failure data on the device being monitored.

That information could very much be function specific.

> So I understand the concern, but I don't think that it should really
> happen in real systems. I did think about this case when I was working
> on the patches. A networking device should really only have VPD when
> it is its own physical device, such as a NIC.
> 

When I was looking at whether we should provide VPD access of an
assigned device at all, I ran across this interesting statement in the
PCI spec (rev 3.0, I.3.1.1):

        CP Extended Capability

        This field allows a new capability to be identified in the VPD
        area. Since dynamic control/status cannot be placed in VPD, the
        data for this field identifies where, in the device’s memory or
        I/O address space, the control/status registers for the
        capability can be found. Location of the control/status
        registers is identified by providing the index (a value between
        0 and 5) of the Base Address register that defines the address
        range that contains the registers, and the offset within that
        Base Address register range where the control/status registers
        reside. The data area for this field is four bytes long. The
        first byte contains the ID of the extended capability. The
        second byte contains the index (zero based) of the Base Address
        register used. The next two bytes contain the offset (in
        little-endian order) within that address range where the
        control/status registers defined for that capability reside.

Again, this sounds like function specific data, and both here and above,
blocking access to VPD could affect the functionality of drivers.  It
may be the case that Intel would find this use to be madness, but
there's no PCI spec requirement that separate functions are in any way
similar and we're looking at an interface that may be used by non-Intel
devices as well.  Thanks,

Alex

[Intel-wired-lan] [PATCH V4 1/2] pci: Add dev_flags bit to access VPD through function 0

[Alex Williamson]

On Tue, 2015-09-15 at 18:39 +0000, Rustad, Mark D wrote:
> > On Sep 15, 2015, at 11:19 AM, Alex Williamson <alex.williamson@redhat.com> wrote:
> > 
> > In addition to the (PCI_SLOT() != devfn) issue, I'm concerned about
> > topologies like we see on Skylake.  IIRC, the integrated NIC appears at
> > something like 00:1f.6.  I don't know if that specific NIC has VPD, nor
> > am I sure it really matter because another example or some future
> > version might.  So we'll set the PCI_DEV_FLAGS_VPD_REF_F0 because we do
> > so for all (PCI_FUNC() != 0) Intel NICs, we'll call
> > pci_vpd_f0_dev_check(), which will error because function 0 has a
> > different class code and device ID, so we return error and if VPD exists
> > on the device, it's now inaccessible.
> 
> Yes, that is exactly what would happen.
> 
> > I thought there was talk about whitelisting anything on the root bus to
> > avoid strange root complex integrated devices (and perhaps avoid the
> > general case for assigned devices within a VM), but I don't see anything
> > like that here.
> 
> I hadn't heard that talk, but I'm not on the PCI list and I guess I
> wasn't copied.
> 
> > Perhaps instead of failing and hiding VPD we should fail, clear the
> > flag, and allow normal access.  Thanks,
> 
> Because the purpose of VPD is to hold information about the device, I
> would suggest that VPD should never be provided for an embedded
> network device, but rather for the device as a whole. So while there
> may well be VPD for an SOC, that VPD should not be associated with one
> of its embedded devices, but rather something more appropriate for the
> device as a whole. And attaching VPD to a whole bunch of internal
> devices would just be madness.

FRU-type information is only one of the use cases of VPD, the spec also
defines (PCI rev 3.0, 6.4):

        ... a mechanism for storing information such as performance and
        failure data on the device being monitored.

That information could very much be function specific.

> So I understand the concern, but I don't think that it should really
> happen in real systems. I did think about this case when I was working
> on the patches. A networking device should really only have VPD when
> it is its own physical device, such as a NIC.
> 

When I was looking at whether we should provide VPD access of an
assigned device at all, I ran across this interesting statement in the
PCI spec (rev 3.0, I.3.1.1):

        CP Extended Capability

        This field allows a new capability to be identified in the VPD
        area. Since dynamic control/status cannot be placed in VPD, the
        data for this field identifies where, in the device?s memory or
        I/O address space, the control/status registers for the
        capability can be found. Location of the control/status
        registers is identified by providing the index (a value between
        0 and 5) of the Base Address register that defines the address
        range that contains the registers, and the offset within that
        Base Address register range where the control/status registers
        reside. The data area for this field is four bytes long. The
        first byte contains the ID of the extended capability. The
        second byte contains the index (zero based) of the Base Address
        register used. The next two bytes contain the offset (in
        little-endian order) within that address range where the
        control/status registers defined for that capability reside.

Again, this sounds like function specific data, and both here and above,
blocking access to VPD could affect the functionality of drivers.  It
may be the case that Intel would find this use to be madness, but
there's no PCI spec requirement that separate functions are in any way
similar and we're looking at an interface that may be used by non-Intel
devices as well.  Thanks,

Alex

Re: [PATCH V4 1/2] pci: Add dev_flags bit to access VPD through function 0

[Rustad, Mark D]

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

> On Sep 15, 2015, at 12:04 PM, Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> 
> FRU-type information is only one of the use cases of VPD, the spec also
> defines (PCI rev 3.0, 6.4):
> 
>        ... a mechanism for storing information such as performance and
>        failure data on the device being monitored.
> 
> That information could very much be function specific.

It is open to interpretation. I guess still I see it as the physical device as a whole.

> When I was looking at whether we should provide VPD access of an
> assigned device at all, I ran across this interesting statement in the
> PCI spec (rev 3.0, I.3.1.1):
> 
>        CP Extended Capability
> 
>        This field allows a new capability to be identified in the VPD
>        area. Since dynamic control/status cannot be placed in VPD, the
>        data for this field identifies where, in the device’s memory or
>        I/O address space, the control/status registers for the
>        capability can be found. Location of the control/status
>        registers is identified by providing the index (a value between
>        0 and 5) of the Base Address register that defines the address
>        range that contains the registers, and the offset within that
>        Base Address register range where the control/status registers
>        reside. The data area for this field is four bytes long. The
>        first byte contains the ID of the extended capability. The
>        second byte contains the index (zero based) of the Base Address
>        register used. The next two bytes contain the offset (in
>        little-endian order) within that address range where the
>        control/status registers defined for that capability reside.
> 
> Again, this sounds like function specific data, and both here and above,
> blocking access to VPD could affect the functionality of drivers.  It
> may be the case that Intel would find this use to be madness, but
> there's no PCI spec requirement that separate functions are in any way
> similar and we're looking at an interface that may be used by non-Intel
> devices as well.  Thanks,

It isn't an interface as such, it is a quirk to address some widespread design problems with multi function devices with VPD. And you are right that functions can be different. In fact this quirk is needed only because now they often (usually in fact) are not different! I do hope to see some non-Intel devices use the quirk, because I'm pretty sure there are other devices that have the same issue.

I realize that I covered a pretty wide swath by making the quirk apply to all Intel Ethernet devices, but that still seems correct. The Skylake is not an issue because it does not have VPD so the pci_find_capability will fail before any handling of the quirk is possible. The code that applies the quirk could check specific devices, but it would make the code a lot bigger, and I see this kind of code as dead weight for so many systems that I tried to make it as small as possible. Since all Intel Ethernet seems to be correct now and as far as I can see into the future, that is what I did.

Going back to something you mentioned before, I think you are right that the failure case for the pci_vpd_f0_dev_check could be made to simply clear the quirk and continue, since pci_vpd_f0_dev_check really should not fail in cases where the quirk is applicable. That does seem like a reasonable change to me the more I think about it.

I think a whitelist would be unnecessary dead weight.

--
Mark Rustad, Networking Division, Intel Corporation


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

[Intel-wired-lan] [PATCH V4 1/2] pci: Add dev_flags bit to access VPD through function 0

[Rustad, Mark D]

> On Sep 15, 2015, at 12:04 PM, Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> 
> FRU-type information is only one of the use cases of VPD, the spec also
> defines (PCI rev 3.0, 6.4):
> 
>        ... a mechanism for storing information such as performance and
>        failure data on the device being monitored.
> 
> That information could very much be function specific.

It is open to interpretation. I guess still I see it as the physical device as a whole.

> When I was looking at whether we should provide VPD access of an
> assigned device at all, I ran across this interesting statement in the
> PCI spec (rev 3.0, I.3.1.1):
> 
>        CP Extended Capability
> 
>        This field allows a new capability to be identified in the VPD
>        area. Since dynamic control/status cannot be placed in VPD, the
>        data for this field identifies where, in the device?s memory or
>        I/O address space, the control/status registers for the
>        capability can be found. Location of the control/status
>        registers is identified by providing the index (a value between
>        0 and 5) of the Base Address register that defines the address
>        range that contains the registers, and the offset within that
>        Base Address register range where the control/status registers
>        reside. The data area for this field is four bytes long. The
>        first byte contains the ID of the extended capability. The
>        second byte contains the index (zero based) of the Base Address
>        register used. The next two bytes contain the offset (in
>        little-endian order) within that address range where the
>        control/status registers defined for that capability reside.
> 
> Again, this sounds like function specific data, and both here and above,
> blocking access to VPD could affect the functionality of drivers.  It
> may be the case that Intel would find this use to be madness, but
> there's no PCI spec requirement that separate functions are in any way
> similar and we're looking at an interface that may be used by non-Intel
> devices as well.  Thanks,

It isn't an interface as such, it is a quirk to address some widespread design problems with multi function devices with VPD. And you are right that functions can be different. In fact this quirk is needed only because now they often (usually in fact) are not different! I do hope to see some non-Intel devices use the quirk, because I'm pretty sure there are other devices that have the same issue.

I realize that I covered a pretty wide swath by making the quirk apply to all Intel Ethernet devices, but that still seems correct. The Skylake is not an issue because it does not have VPD so the pci_find_capability will fail before any handling of the quirk is possible. The code that applies the quirk could check specific devices, but it would make the code a lot bigger, and I see this kind of code as dead weight for so many systems that I tried to make it as small as possible. Since all Intel Ethernet seems to be correct now and as far as I can see into the future, that is what I did.

Going back to something you mentioned before, I think you are right that the failure case for the pci_vpd_f0_dev_check could be made to simply clear the quirk and continue, since pci_vpd_f0_dev_check really should not fail in cases where the quirk is applicable. That does seem like a reasonable change to me the more I think about it.

I think a whitelist would be unnecessary dead weight.

--
Mark Rustad, Networking Division, Intel Corporation

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 841 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20150915/7f8c9669/attachment.asc>

Re: [PATCH V4 1/2] pci: Add dev_flags bit to access VPD through function 0

[Alex Williamson]

On Tue, 2015-09-15 at 20:47 +0000, Rustad, Mark D wrote:
> > On Sep 15, 2015, at 12:04 PM, Alex Williamson <alex.williamson@redhat.com> wrote:
> > 
> > 
> > FRU-type information is only one of the use cases of VPD, the spec also
> > defines (PCI rev 3.0, 6.4):
> > 
> >        ... a mechanism for storing information such as performance and
> >        failure data on the device being monitored.
> > 
> > That information could very much be function specific.
> 
> It is open to interpretation. I guess still I see it as the physical device as a whole.
> 
> > When I was looking at whether we should provide VPD access of an
> > assigned device at all, I ran across this interesting statement in the
> > PCI spec (rev 3.0, I.3.1.1):
> > 
> >        CP Extended Capability
> > 
> >        This field allows a new capability to be identified in the VPD
> >        area. Since dynamic control/status cannot be placed in VPD, the
> >        data for this field identifies where, in the device’s memory or
> >        I/O address space, the control/status registers for the
> >        capability can be found. Location of the control/status
> >        registers is identified by providing the index (a value between
> >        0 and 5) of the Base Address register that defines the address
> >        range that contains the registers, and the offset within that
> >        Base Address register range where the control/status registers
> >        reside. The data area for this field is four bytes long. The
> >        first byte contains the ID of the extended capability. The
> >        second byte contains the index (zero based) of the Base Address
> >        register used. The next two bytes contain the offset (in
> >        little-endian order) within that address range where the
> >        control/status registers defined for that capability reside.
> > 
> > Again, this sounds like function specific data, and both here and above,
> > blocking access to VPD could affect the functionality of drivers.  It
> > may be the case that Intel would find this use to be madness, but
> > there's no PCI spec requirement that separate functions are in any way
> > similar and we're looking at an interface that may be used by non-Intel
> > devices as well.  Thanks,
> 
> It isn't an interface as such, it is a quirk to address some
> widespread design problems with multi function devices with VPD. And
> you are right that functions can be different. In fact this quirk is
> needed only because now they often (usually in fact) are not
> different! I do hope to see some non-Intel devices use the quirk,
> because I'm pretty sure there are other devices that have the same
> issue.
> 
> I realize that I covered a pretty wide swath by making the quirk apply
> to all Intel Ethernet devices, but that still seems correct. The
> Skylake is not an issue because it does not have VPD so the
> pci_find_capability will fail before any handling of the quirk is
> possible. The code that applies the quirk could check specific
> devices, but it would make the code a lot bigger, and I see this kind
> of code as dead weight for so many systems that I tried to make it as
> small as possible. Since all Intel Ethernet seems to be correct now
> and as far as I can see into the future, that is what I did.
> 
> Going back to something you mentioned before, I think you are right
> that the failure case for the pci_vpd_f0_dev_check could be made to
> simply clear the quirk and continue, since pci_vpd_f0_dev_check really
> should not fail in cases where the quirk is applicable. That does seem
> like a reasonable change to me the more I think about it.
> 
> I think a whitelist would be unnecessary dead weight.

Yep, a whitelist is probably not the way to go.  AFAICT, you're looking
for plugin-cards where all the functions meet the criteria of having the
same class, vendor, and device ID.  If we don't meet that criteria, then
it's not a device we're expecting and we should leave it alone.

Also, rather than clearing the flag, can we move the tests done by
pci_vpd_f0_dev_check() into the
quirk setup function?  It seems like function 0 should be sufficiently
configured by the time we're probing non-zero functions that we can be
more selective in setting the flag rather than unsetting it later.
Thanks,

Alex

[Intel-wired-lan] [PATCH V4 1/2] pci: Add dev_flags bit to access VPD through function 0

[Alex Williamson]

On Tue, 2015-09-15 at 20:47 +0000, Rustad, Mark D wrote:
> > On Sep 15, 2015, at 12:04 PM, Alex Williamson <alex.williamson@redhat.com> wrote:
> > 
> > 
> > FRU-type information is only one of the use cases of VPD, the spec also
> > defines (PCI rev 3.0, 6.4):
> > 
> >        ... a mechanism for storing information such as performance and
> >        failure data on the device being monitored.
> > 
> > That information could very much be function specific.
> 
> It is open to interpretation. I guess still I see it as the physical device as a whole.
> 
> > When I was looking at whether we should provide VPD access of an
> > assigned device at all, I ran across this interesting statement in the
> > PCI spec (rev 3.0, I.3.1.1):
> > 
> >        CP Extended Capability
> > 
> >        This field allows a new capability to be identified in the VPD
> >        area. Since dynamic control/status cannot be placed in VPD, the
> >        data for this field identifies where, in the device?s memory or
> >        I/O address space, the control/status registers for the
> >        capability can be found. Location of the control/status
> >        registers is identified by providing the index (a value between
> >        0 and 5) of the Base Address register that defines the address
> >        range that contains the registers, and the offset within that
> >        Base Address register range where the control/status registers
> >        reside. The data area for this field is four bytes long. The
> >        first byte contains the ID of the extended capability. The
> >        second byte contains the index (zero based) of the Base Address
> >        register used. The next two bytes contain the offset (in
> >        little-endian order) within that address range where the
> >        control/status registers defined for that capability reside.
> > 
> > Again, this sounds like function specific data, and both here and above,
> > blocking access to VPD could affect the functionality of drivers.  It
> > may be the case that Intel would find this use to be madness, but
> > there's no PCI spec requirement that separate functions are in any way
> > similar and we're looking at an interface that may be used by non-Intel
> > devices as well.  Thanks,
> 
> It isn't an interface as such, it is a quirk to address some
> widespread design problems with multi function devices with VPD. And
> you are right that functions can be different. In fact this quirk is
> needed only because now they often (usually in fact) are not
> different! I do hope to see some non-Intel devices use the quirk,
> because I'm pretty sure there are other devices that have the same
> issue.
> 
> I realize that I covered a pretty wide swath by making the quirk apply
> to all Intel Ethernet devices, but that still seems correct. The
> Skylake is not an issue because it does not have VPD so the
> pci_find_capability will fail before any handling of the quirk is
> possible. The code that applies the quirk could check specific
> devices, but it would make the code a lot bigger, and I see this kind
> of code as dead weight for so many systems that I tried to make it as
> small as possible. Since all Intel Ethernet seems to be correct now
> and as far as I can see into the future, that is what I did.
> 
> Going back to something you mentioned before, I think you are right
> that the failure case for the pci_vpd_f0_dev_check could be made to
> simply clear the quirk and continue, since pci_vpd_f0_dev_check really
> should not fail in cases where the quirk is applicable. That does seem
> like a reasonable change to me the more I think about it.
> 
> I think a whitelist would be unnecessary dead weight.

Yep, a whitelist is probably not the way to go.  AFAICT, you're looking
for plugin-cards where all the functions meet the criteria of having the
same class, vendor, and device ID.  If we don't meet that criteria, then
it's not a device we're expecting and we should leave it alone.

Also, rather than clearing the flag, can we move the tests done by
pci_vpd_f0_dev_check() into the
quirk setup function?  It seems like function 0 should be sufficiently
configured by the time we're probing non-zero functions that we can be
more selective in setting the flag rather than unsetting it later.
Thanks,

Alex

Re: [PATCH V4 1/2] pci: Add dev_flags bit to access VPD through function 0

[Rustad, Mark D]

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

> On Sep 15, 2015, at 2:17 PM, Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> Also, rather than clearing the flag, can we move the tests done by
> pci_vpd_f0_dev_check() into the
> quirk setup function?  It seems like function 0 should be sufficiently
> configured by the time we're probing non-zero functions that we can be
> more selective in setting the flag rather than unsetting it later.

I guess I was being very conservative in not assuming anything about the state of other devices at that point. Things seem to be increasingly parallel all the time and I am not deeply involved in the evolution of the PCI subsystem. If you want to make that assumption, I would suggest that pci_vpd_f0_dev_check remain a separate function called by the quirk setup so that it can be used by other quirk setup functions as well.

--
Mark Rustad, Networking Division, Intel Corporation


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

[Intel-wired-lan] [PATCH V4 1/2] pci: Add dev_flags bit to access VPD through function 0

[Rustad, Mark D]

> On Sep 15, 2015, at 2:17 PM, Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> Also, rather than clearing the flag, can we move the tests done by
> pci_vpd_f0_dev_check() into the
> quirk setup function?  It seems like function 0 should be sufficiently
> configured by the time we're probing non-zero functions that we can be
> more selective in setting the flag rather than unsetting it later.

I guess I was being very conservative in not assuming anything about the state of other devices at that point. Things seem to be increasingly parallel all the time and I am not deeply involved in the evolution of the PCI subsystem. If you want to make that assumption, I would suggest that pci_vpd_f0_dev_check remain a separate function called by the quirk setup so that it can be used by other quirk setup functions as well.

--
Mark Rustad, Networking Division, Intel Corporation

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 841 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20150915/48f8ee00/attachment.asc>