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

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

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

---

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 |   61 +++++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/pci/quirks.c |    9 +++++++
 2 files changed, 69 insertions(+), 1 deletion(-)

-- 
Mark Rustad, Network Division, Intel Corporation

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

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

---

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 |   61 +++++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/pci/quirks.c |    9 +++++++
 2 files changed, 69 insertions(+), 1 deletion(-)

-- 
Mark Rustad, Network Division, Intel Corporation

[PATCH V3 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 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
---
 drivers/pci/access.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 60 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 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 V3 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 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
---
 drivers/pci/access.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 60 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 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 V3 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>
---
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 c6dc1dfd25d5..1d6d848b66f9 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 (!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 V3 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>
---
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 c6dc1dfd25d5..1d6d848b66f9 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 (!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 V3 0/2] pci: Provide a flag to access VPD through function 0

[Rustad, Mark D]

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

> On Jun 3, 2015, at 11:46 AM, Mark D Rustad <mark.d.rustad@intel.com> 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.
> 
> 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

So does this series look acceptable now? I think I addressed the issues that Alex raised. Can these also be considered for -stable?

--
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 V3 0/2] pci: Provide a flag to access VPD through function 0

[Rustad, Mark D]

> On Jun 3, 2015, at 11:46 AM, Mark D Rustad <mark.d.rustad@intel.com> 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.
> 
> 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

So does this series look acceptable now? I think I addressed the issues that Alex raised. Can these also be considered for -stable?

--
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/20150605/fb249164/attachment.asc>

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

[Rustad, Mark D]

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

+ Alex

> On Jun 5, 2015, at 2:59 PM, Rustad, Mark D <mark.d.rustad@intel.com> wrote:
> 
>> On Jun 3, 2015, at 11:46 AM, Mark D Rustad <mark.d.rustad@intel.com> 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.
>> 
>> 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
> 
> So does this series look acceptable now? I think I addressed the issues that Alex raised. Can these also be considered for -stable?

More than a week has passed without any comment. Is this going to be accepted or is there still an issue?

--
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 V3 0/2] pci: Provide a flag to access VPD through function 0

[Rustad, Mark D]

+ Alex

> On Jun 5, 2015, at 2:59 PM, Rustad, Mark D <mark.d.rustad@intel.com> wrote:
> 
>> On Jun 3, 2015, at 11:46 AM, Mark D Rustad <mark.d.rustad@intel.com> 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.
>> 
>> 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
> 
> So does this series look acceptable now? I think I addressed the issues that Alex raised. Can these also be considered for -stable?

More than a week has passed without any comment. Is this going to be accepted or is there still an issue?

--
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/20150617/4805fe5a/attachment.asc>

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

[Bjorn Helgaas]

On Wed, Jun 17, 2015 at 11:29 AM, Rustad, Mark D
<mark.d.rustad@intel.com> wrote:
> + Alex
>
>> On Jun 5, 2015, at 2:59 PM, Rustad, Mark D <mark.d.rustad@intel.com> wrote:
>>
>>> On Jun 3, 2015, at 11:46 AM, Mark D Rustad <mark.d.rustad@intel.com> 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.
>>>
>>> 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
>>
>> So does this series look acceptable now? I think I addressed the issues that Alex raised. Can these also be considered for -stable?
>
> More than a week has passed without any comment. Is this going to be accepted or is there still an issue?

Sorry, Mark, I've just been busy with other issues and haven't had a
chance to look at this yet.

Bjorn

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

[Bjorn Helgaas]

On Wed, Jun 17, 2015 at 11:29 AM, Rustad, Mark D
<mark.d.rustad@intel.com> wrote:
> + Alex
>
>> On Jun 5, 2015, at 2:59 PM, Rustad, Mark D <mark.d.rustad@intel.com> wrote:
>>
>>> On Jun 3, 2015, at 11:46 AM, Mark D Rustad <mark.d.rustad@intel.com> 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.
>>>
>>> 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
>>
>> So does this series look acceptable now? I think I addressed the issues that Alex raised. Can these also be considered for -stable?
>
> More than a week has passed without any comment. Is this going to be accepted or is there still an issue?

Sorry, Mark, I've just been busy with other issues and haven't had a
chance to look at this yet.

Bjorn

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

[Rustad, Mark D]

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

> On Jun 17, 2015, at 9:44 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> 
> On Wed, Jun 17, 2015 at 11:29 AM, Rustad, Mark D
> <mark.d.rustad@intel.com> wrote:
>> + Alex
>> 
>>> On Jun 5, 2015, at 2:59 PM, Rustad, Mark D <mark.d.rustad@intel.com> wrote:
>>> 
>>>> On Jun 3, 2015, at 11:46 AM, Mark D Rustad <mark.d.rustad@intel.com> 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.
>>>> 
>>>> 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
>>> 
>>> So does this series look acceptable now? I think I addressed the issues that Alex raised. Can these also be considered for -stable?
>> 
>> More than a week has passed without any comment. Is this going to be accepted or is there still an issue?
> 
> Sorry, Mark, I've just been busy with other issues and haven't had a
> chance to look at this yet.

Is there any chance of this getting into this merge window?

--
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 V3 0/2] pci: Provide a flag to access VPD through function 0

[Rustad, Mark D]

> On Jun 17, 2015, at 9:44 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> 
> On Wed, Jun 17, 2015 at 11:29 AM, Rustad, Mark D
> <mark.d.rustad@intel.com> wrote:
>> + Alex
>> 
>>> On Jun 5, 2015, at 2:59 PM, Rustad, Mark D <mark.d.rustad@intel.com> wrote:
>>> 
>>>> On Jun 3, 2015, at 11:46 AM, Mark D Rustad <mark.d.rustad@intel.com> 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.
>>>> 
>>>> 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
>>> 
>>> So does this series look acceptable now? I think I addressed the issues that Alex raised. Can these also be considered for -stable?
>> 
>> More than a week has passed without any comment. Is this going to be accepted or is there still an issue?
> 
> Sorry, Mark, I've just been busy with other issues and haven't had a
> chance to look at this yet.

Is there any chance of this getting into this merge window?

--
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/20150626/a9cce4ff/attachment-0001.asc>

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

[Rustad, Mark D]

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

> On Jun 26, 2015, at 11:04 AM, Rustad, Mark D <mark.d.rustad@intel.com> wrote:
> 
>> Sorry, Mark, I've just been busy with other issues and haven't had a
>> chance to look at this yet.
> 
> Is there any chance of this getting into this merge window?

Well, it has missed the merge window, but this really is a bug fix. These patches address problems that, under race conditions, can corrupt VPD data and under other conditions can cause hangs. In fact I would submit that the reason that the VPD operations have been made interruptible is directly related to hangs caused by the sharing of VPD capability registers between functions. You see, if one function ever performs a VPD write, any subsequent read on any other function that shares those registers will definitely hang.

I imagine that there are many devices beyond Intel's Ethernet devices that would benefit from using the quirk that these patches introduce.

Please apply it and consider it for -stable.

--
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 V3 0/2] pci: Provide a flag to access VPD through function 0

[Rustad, Mark D]

> On Jun 26, 2015, at 11:04 AM, Rustad, Mark D <mark.d.rustad@intel.com> wrote:
> 
>> Sorry, Mark, I've just been busy with other issues and haven't had a
>> chance to look at this yet.
> 
> Is there any chance of this getting into this merge window?

Well, it has missed the merge window, but this really is a bug fix. These patches address problems that, under race conditions, can corrupt VPD data and under other conditions can cause hangs. In fact I would submit that the reason that the VPD operations have been made interruptible is directly related to hangs caused by the sharing of VPD capability registers between functions. You see, if one function ever performs a VPD write, any subsequent read on any other function that shares those registers will definitely hang.

I imagine that there are many devices beyond Intel's Ethernet devices that would benefit from using the quirk that these patches introduce.

Please apply it and consider it for -stable.

--
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/20150706/719cc42f/attachment.asc>

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

[Bjorn Helgaas]

On Mon, Jul 6, 2015 at 12:31 PM, Rustad, Mark D <mark.d.rustad@intel.com> wrote:
>> On Jun 26, 2015, at 11:04 AM, Rustad, Mark D <mark.d.rustad@intel.com> wrote:
>>
>>> Sorry, Mark, I've just been busy with other issues and haven't had a
>>> chance to look at this yet.
>>
>> Is there any chance of this getting into this merge window?
>
> Well, it has missed the merge window, but this really is a bug fix. These patches address problems that, under race conditions, can corrupt VPD data and under other conditions can cause hangs. In fact I would submit that the reason that the VPD operations have been made interruptible is directly related to hangs caused by the sharing of VPD capability registers between functions. You see, if one function ever performs a VPD write, any subsequent read on any other function that shares those registers will definitely hang.

As a rule, after the merge window closes, I only merge fixes for
things we broke during the merge window or drivers for brand-new
things, where there's no chance of breaking something that used to
work.

It would be nice to have a description of what a user might see when
tripping over this problem and maybe some pointers to problem reports.
I see it fixes concurrent access problems, and I infer that it's
related to sysfs.

Feel free to add a stable tag when you post your patches.  I'll try to
remember to add it when merging this.  When it's relevant, it's nice
to include the SHA1 of the commit that introduced the bug.  But in
this case, I think it's been there "forever."

Bjorn

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

[Bjorn Helgaas]

On Mon, Jul 6, 2015 at 12:31 PM, Rustad, Mark D <mark.d.rustad@intel.com> wrote:
>> On Jun 26, 2015, at 11:04 AM, Rustad, Mark D <mark.d.rustad@intel.com> wrote:
>>
>>> Sorry, Mark, I've just been busy with other issues and haven't had a
>>> chance to look at this yet.
>>
>> Is there any chance of this getting into this merge window?
>
> Well, it has missed the merge window, but this really is a bug fix. These patches address problems that, under race conditions, can corrupt VPD data and under other conditions can cause hangs. In fact I would submit that the reason that the VPD operations have been made interruptible is directly related to hangs caused by the sharing of VPD capability registers between functions. You see, if one function ever performs a VPD write, any subsequent read on any other function that shares those registers will definitely hang.

As a rule, after the merge window closes, I only merge fixes for
things we broke during the merge window or drivers for brand-new
things, where there's no chance of breaking something that used to
work.

It would be nice to have a description of what a user might see when
tripping over this problem and maybe some pointers to problem reports.
I see it fixes concurrent access problems, and I infer that it's
related to sysfs.

Feel free to add a stable tag when you post your patches.  I'll try to
remember to add it when merging this.  When it's relevant, it's nice
to include the SHA1 of the commit that introduced the bug.  But in
this case, I think it's been there "forever."

Bjorn

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

[Alexander Duyck]

On 06/17/2015 09:29 AM, Rustad, Mark D wrote:
> + Alex
>
>> On Jun 5, 2015, at 2:59 PM, Rustad, Mark D <mark.d.rustad@intel.com> wrote:
>>
>>> On Jun 3, 2015, at 11:46 AM, Mark D Rustad <mark.d.rustad@intel.com> 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.
>>>
>>> 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
>> So does this series look acceptable now? I think I addressed the issues that Alex raised. Can these also be considered for -stable?
> More than a week has passed without any comment. Is this going to be accepted or is there still an issue?

Yeah, this looks like it has addressed most of the corner cases so I am 
good with it.

Acked-by: Alexander Duyck <alexander.h.duyck@redhat.com>

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

[Alexander Duyck]

On 06/17/2015 09:29 AM, Rustad, Mark D wrote:
> + Alex
>
>> On Jun 5, 2015, at 2:59 PM, Rustad, Mark D <mark.d.rustad@intel.com> wrote:
>>
>>> On Jun 3, 2015, at 11:46 AM, Mark D Rustad <mark.d.rustad@intel.com> 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.
>>>
>>> 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
>> So does this series look acceptable now? I think I addressed the issues that Alex raised. Can these also be considered for -stable?
> More than a week has passed without any comment. Is this going to be accepted or is there still an issue?

Yeah, this looks like it has addressed most of the corner cases so I am 
good with it.

Acked-by: Alexander Duyck <alexander.h.duyck@redhat.com>