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

[PATCH 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.

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

---

Mark D 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 |   39 ++++++++++++++++++++++++++++++++++++++-
 drivers/pci/quirks.c |    9 +++++++++
 include/linux/pci.h  |    2 ++
 3 files changed, 49 insertions(+), 1 deletion(-)

-- 
Mark Rustad, Network Division, Intel Corporation

[Intel-wired-lan] [PATCH 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.

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

---

Mark D 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 |   39 ++++++++++++++++++++++++++++++++++++++-
 drivers/pci/quirks.c |    9 +++++++++
 include/linux/pci.h  |    2 ++
 3 files changed, 49 insertions(+), 1 deletion(-)

-- 
Mark Rustad, Network Division, Intel Corporation

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

[Mark D Rustad]

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 solves
concurrent access problems on many devices without changing the
attributes exposed in sysfs. Never set this bit on funciton 0 or
there will be an infinite recursion.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
---
 drivers/pci/access.c |   39 ++++++++++++++++++++++++++++++++++++++-
 include/linux/pci.h  |    2 ++
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index d9b64a175990..e435c087c354 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -439,6 +439,40 @@ 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,
+};
+
 int pci_vpd_pci22_init(struct pci_dev *dev)
 {
 	struct pci_vpd_pci22 *vpd;
@@ -452,7 +486,10 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
 		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 353db8dc4c6e..194df6d635e6 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 1/2] pci: Add dev_flags bit to access VPD through function 0

[Mark D Rustad]

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 solves
concurrent access problems on many devices without changing the
attributes exposed in sysfs. Never set this bit on funciton 0 or
there will be an infinite recursion.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
---
 drivers/pci/access.c |   39 ++++++++++++++++++++++++++++++++++++++-
 include/linux/pci.h  |    2 ++
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index d9b64a175990..e435c087c354 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -439,6 +439,40 @@ 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,
+};
+
 int pci_vpd_pci22_init(struct pci_dev *dev)
 {
 	struct pci_vpd_pci22 *vpd;
@@ -452,7 +486,10 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
 		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 353db8dc4c6e..194df6d635e6 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 2/2] pci: Add VPD quirk for Intel Ethernet devices

[Mark D Rustad]

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>
---
 drivers/pci/quirks.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index c6dc1dfd25d5..9ddf6a533f4f 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1903,6 +1903,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 (!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 2/2] pci: Add VPD quirk for Intel Ethernet devices

[Mark D Rustad]

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>
---
 drivers/pci/quirks.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index c6dc1dfd25d5..9ddf6a533f4f 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1903,6 +1903,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 (!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: [Intel-wired-lan] [PATCH 1/2] pci: Add dev_flags bit to access VPD through function 0

[Alexander Duyck]

On 06/02/2015 10:04 AM, Mark D Rustad wrote:
> 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 solves
> concurrent access problems on many devices without changing the
> attributes exposed in sysfs. Never set this bit on funciton 0 or
> there will be an infinite recursion.
>
> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
> ---
>   drivers/pci/access.c |   39 ++++++++++++++++++++++++++++++++++++++-
>   include/linux/pci.h  |    2 ++
>   2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index d9b64a175990..e435c087c354 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -439,6 +439,40 @@ 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;
> +}
> +

I'm pretty sure these could cause some serious errors if you direct 
assign the device into a VM since you then end up with multiple devices 
sharing a bus.  Also it would likely have side-effects on a LOM (Lan On 
Motherboard) as it also shares the bus with multiple non-Ethernet devices.

I believe you still need to add something like a check for 
!pci_is_root_bus(dev->bus) before you attempt to grab function 0.  It 
probably also wouldn't hurt to check the dev->multifunction bit before 
running this code since it wouldn't make sense to go chasing down the 
VPD on another function if the device doesn't have one.  You could 
probably do that either as a part of this code, or perhaps put it in the 
quirk.

- Alex

- Alex

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

[Alexander Duyck]

On 06/02/2015 10:04 AM, Mark D Rustad wrote:
> 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 solves
> concurrent access problems on many devices without changing the
> attributes exposed in sysfs. Never set this bit on funciton 0 or
> there will be an infinite recursion.
>
> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
> ---
>   drivers/pci/access.c |   39 ++++++++++++++++++++++++++++++++++++++-
>   include/linux/pci.h  |    2 ++
>   2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index d9b64a175990..e435c087c354 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -439,6 +439,40 @@ 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;
> +}
> +

I'm pretty sure these could cause some serious errors if you direct 
assign the device into a VM since you then end up with multiple devices 
sharing a bus.  Also it would likely have side-effects on a LOM (Lan On 
Motherboard) as it also shares the bus with multiple non-Ethernet devices.

I believe you still need to add something like a check for 
!pci_is_root_bus(dev->bus) before you attempt to grab function 0.  It 
probably also wouldn't hurt to check the dev->multifunction bit before 
running this code since it wouldn't make sense to go chasing down the 
VPD on another function if the device doesn't have one.  You could 
probably do that either as a part of this code, or perhaps put it in the 
quirk.

- Alex

- Alex

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

[Rustad, Mark D]

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

> On Jun 2, 2015, at 10:48 AM, Alexander Duyck <alexander.h.duyck@redhat.com> wrote:
> 
> I'm pretty sure these could cause some serious errors if you direct assign the device into a VM since you then end up with multiple devices sharing a bus.  Also it would likely have side-effects on a LOM (Lan On Motherboard) as it also shares the bus with multiple non-Ethernet devices.
> 
> I believe you still need to add something like a check for !pci_is_root_bus(dev->bus) before you attempt to grab function 0.  It probably also wouldn't hurt to check the dev->multifunction bit before running this code since it wouldn't make sense to go chasing down the VPD on another function if the device doesn't have one.  You could probably do that either as a part of this code, or perhaps put it in the quirk.

I'll look into those. I think you are right about more checks being needed. Thanks for the comments.

--
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 1/2] pci: Add dev_flags bit to access VPD through function 0

[Rustad, Mark D]

> On Jun 2, 2015, at 10:48 AM, Alexander Duyck <alexander.h.duyck@redhat.com> wrote:
> 
> I'm pretty sure these could cause some serious errors if you direct assign the device into a VM since you then end up with multiple devices sharing a bus.  Also it would likely have side-effects on a LOM (Lan On Motherboard) as it also shares the bus with multiple non-Ethernet devices.
> 
> I believe you still need to add something like a check for !pci_is_root_bus(dev->bus) before you attempt to grab function 0.  It probably also wouldn't hurt to check the dev->multifunction bit before running this code since it wouldn't make sense to go chasing down the VPD on another function if the device doesn't have one.  You could probably do that either as a part of this code, or perhaps put it in the quirk.

I'll look into those. I think you are right about more checks being needed. Thanks for the comments.

--
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/20150602/94886361/attachment-0001.asc>