Re: [PATCH 3.2 035/107] PCI: Add dev_flags bit to access VPD through function 0

2015-10-09 Thread Rustad, Mark D 
Ben Hutchings  wrote:

> They're bit masks, not bit numbers, both in 3.2 and upstream.  In
> mainline, bits 3-7 have already been assigned to other flags.  I don't
> see the need to renumber or write the value differently when
> backporting.

No problem. I just saw the different representations and was wondering how you 
felt about it. Next time I'll know. Thanks.

--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 3.2 035/107] PCI: Add dev_flags bit to access VPD through function 0

2015-10-09 Thread Rustad, Mark D 
Ben Hutchings  wrote:

> They're bit masks, not bit numbers, both in 3.2 and upstream.  In
> mainline, bits 3-7 have already been assigned to other flags.  I don't
> see the need to renumber or write the value differently when
> backporting.

No problem. I just saw the different representations and was wondering how you 
felt about it. Next time I'll know. Thanks.

--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 3.2 035/107] PCI: Add dev_flags bit to access VPD through function 0

2015-10-08 Thread Ben Hutchings 
On Fri, 2015-10-09 at 00:26 +, Rustad, Mark D wrote:
> Ben Hutchings  wrote:
> 
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -176,6 +176,8 @@ enum pci_dev_flags {
> > > > PCI_DEV_FLAGS_NO_D3 = (__force pci_dev_flags_t) 2,
> > > > /* Provide indication device is assigned by a Virtual Machine 
> > Manager */
> > > > PCI_DEV_FLAGS_ASSIGNED = (__force pci_dev_flags_t) 4,
> > +> >> > /* Get VPD from function 0 VPD */
> > +> >> > PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 
> > 8),
> > };
> > 
> > enum pci_irq_reroute_variant {
> 
> In this hunk I happened to notice the change in how these values are
> assigned. Should the new value remain (1 << 8) or should it fall in
> line with the older implementation and simply be 8? Or should it be
> 256? It depends on which kind of consistency you prefer for the
> backport.

They're bit masks, not bit numbers, both in 3.2 and upstream.  In
mainline, bits 3-7 have already been assigned to other flags.  I don't
see the need to renumber or write the value differently when
backporting.

Ben.

-- 
Ben Hutchings
If the facts do not conform to your theory, they must be disposed of.

signature.asc
Description: This is a digitally signed message part

[PATCH 3.2 035/107] PCI: Add dev_flags bit to access VPD through function 0

2015-10-08 Thread Ben Hutchings 
3.2.72-rc1 review patch.  If anyone has any objections, please let me know.

--

From: Mark Rustad 

commit 932c435caba8a2ce473a91753bad0173269ef334 upstream.

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 
Signed-off-by: Bjorn Helgaas 
Acked-by: Alexander Duyck 
Signed-off-by: Ben Hutchings 
---
 drivers/pci/access.c | 61 +++-
 include/linux/pci.h  |  2 ++
 2 files changed, 62 insertions(+), 1 deletion(-)

--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -355,6 +355,56 @@ static const struct pci_vpd_ops pci_vpd_
.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;
@@ -363,12 +413,21 @@ int pci_vpd_pci22_init(struct pci_dev *d
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 = _vpd_pci22_ops;
+   if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0)
+   vpd->base.ops = _vpd_f0_ops;
+   else
+   vpd->base.ops = _vpd_pci22_ops;
mutex_init(>lock);
vpd->cap = cap;
vpd->busy = false;
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -176,6 +176,8 @@ enum pci_dev_flags {
PCI_DEV_FLAGS_NO_D3 = (__force pci_dev_flags_t) 2,
/* Provide indication device is assigned by a Virtual Machine Manager */
PCI_DEV_FLAGS_ASSIGNED = (__force pci_dev_flags_t) 4,
+   /* 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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3.2 035/107] PCI: Add dev_flags bit to access VPD through function 0

2015-10-08 Thread Rustad, Mark D 
Ben Hutchings  wrote:

> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -176,6 +176,8 @@ enum pci_dev_flags {
>   PCI_DEV_FLAGS_NO_D3 = (__force pci_dev_flags_t) 2,
>   /* Provide indication device is assigned by a Virtual Machine Manager */
>   PCI_DEV_FLAGS_ASSIGNED = (__force pci_dev_flags_t) 4,
> + /* Get VPD from function 0 VPD */
> + PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
> };
> 
> enum pci_irq_reroute_variant {

In this hunk I happened to notice the change in how these values are assigned. 
Should the new value remain (1 << 8) or should it fall in line with the older 
implementation and simply be 8? Or should it be 256? It depends on which kind 
of consistency you prefer for the backport.

--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 3.2 035/107] PCI: Add dev_flags bit to access VPD through function 0

2015-10-08 Thread Rustad, Mark D 
Ben Hutchings  wrote:

> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -176,6 +176,8 @@ enum pci_dev_flags {
>   PCI_DEV_FLAGS_NO_D3 = (__force pci_dev_flags_t) 2,
>   /* Provide indication device is assigned by a Virtual Machine Manager */
>   PCI_DEV_FLAGS_ASSIGNED = (__force pci_dev_flags_t) 4,
> + /* Get VPD from function 0 VPD */
> + PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
> };
> 
> enum pci_irq_reroute_variant {

In this hunk I happened to notice the change in how these values are assigned. 
Should the new value remain (1 << 8) or should it fall in line with the older 
implementation and simply be 8? Or should it be 256? It depends on which kind 
of consistency you prefer for the backport.

--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 3.2 035/107] PCI: Add dev_flags bit to access VPD through function 0

2015-10-08 Thread Ben Hutchings 
On Fri, 2015-10-09 at 00:26 +, Rustad, Mark D wrote:
> Ben Hutchings  wrote:
> 
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -176,6 +176,8 @@ enum pci_dev_flags {
> > > > PCI_DEV_FLAGS_NO_D3 = (__force pci_dev_flags_t) 2,
> > > > /* Provide indication device is assigned by a Virtual Machine 
> > Manager */
> > > > PCI_DEV_FLAGS_ASSIGNED = (__force pci_dev_flags_t) 4,
> > +> >> > /* Get VPD from function 0 VPD */
> > +> >> > PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 
> > 8),
> > };
> > 
> > enum pci_irq_reroute_variant {
> 
> In this hunk I happened to notice the change in how these values are
> assigned. Should the new value remain (1 << 8) or should it fall in
> line with the older implementation and simply be 8? Or should it be
> 256? It depends on which kind of consistency you prefer for the
> backport.

They're bit masks, not bit numbers, both in 3.2 and upstream.  In
mainline, bits 3-7 have already been assigned to other flags.  I don't
see the need to renumber or write the value differently when
backporting.

Ben.

-- 
Ben Hutchings
If the facts do not conform to your theory, they must be disposed of.

signature.asc
Description: This is a digitally signed message part

[PATCH 3.2 035/107] PCI: Add dev_flags bit to access VPD through function 0

2015-10-08 Thread Ben Hutchings 
3.2.72-rc1 review patch.  If anyone has any objections, please let me know.

--

From: Mark Rustad 

commit 932c435caba8a2ce473a91753bad0173269ef334 upstream.

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 
Signed-off-by: Bjorn Helgaas 
Acked-by: Alexander Duyck 
Signed-off-by: Ben Hutchings 
---
 drivers/pci/access.c | 61 +++-
 include/linux/pci.h  |  2 ++
 2 files changed, 62 insertions(+), 1 deletion(-)

--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -355,6 +355,56 @@ static const struct pci_vpd_ops pci_vpd_
.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;
@@ -363,12 +413,21 @@ int pci_vpd_pci22_init(struct pci_dev *d
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 = _vpd_pci22_ops;
+   if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0)
+   vpd->base.ops = _vpd_f0_ops;
+   else
+   vpd->base.ops = _vpd_pci22_ops;
mutex_init(>lock);
vpd->cap = cap;
vpd->busy = false;
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -176,6 +176,8 @@ enum pci_dev_flags {
PCI_DEV_FLAGS_NO_D3 = (__force pci_dev_flags_t) 2,
/* Provide indication device is assigned by a Virtual Machine Manager */
PCI_DEV_FLAGS_ASSIGNED = (__force pci_dev_flags_t) 4,
+   /* 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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/