PCI: defer enablement of SRIOV BARS

All the PCI BARs of a device are enabled when the device is enabled using pci_enable_device(). This unnecessarily enables SRIOV BARs of the device.

On some platforms, which do not support SRIOV as yet, the pci_enable_device() fails to enable the device if its SRIOV BARs are not allocated resources correctly.

The following patch fixes the above problem. The SRIOV BARs are now enabled when IOV capability of the device is enabled in sriov_enable().

NOTE: Note, there is subtle change in the pci_enable_device() API. Any driver that depends on SRIOV BARS to be enabled in pci_enable_device() can fail.

The patch has been touch tested on power and x86 platform.

Signed-off-by Ram Pai <[email protected]>

---

Quoted 14 lines · On Sun, 6 Nov 2011 10:33:10 +0800 Ram Pai <[email protected]> wrote:

All the PCI BARs of a device are enabled when the device is enabled using pci_enable_device(). This unnecessarily enables SRIOV BARs of the device.

On some platforms, which do not support SRIOV as yet, the pci_enable_device() fails to enable the device if its SRIOV BARs are not allocated resources correctly.

The following patch fixes the above problem. The SRIOV BARs are now enabled when IOV capability of the device is enabled in sriov_enable().

NOTE: Note, there is subtle change in the pci_enable_device() API. Any driver that depends on SRIOV BARS to be enabled in pci_enable_device() can fail.

Quoted 4 lines

The patch has been touch tested on power and x86 platform.

Signed-off-by Ram Pai <[email protected]>

---

I think this is a good direction, but I'd like to get a tested-by or two from people using SR-IOV...

Quoted 17 lines · Jesse Barnes

Quoted 14 lines · On Sun, 6 Nov 2011 10:33:10 +0800 Ram Pai<[email protected]> wrote:

All the PCI BARs of a device are enabled when the device is enabled using pci_enable_device(). This unnecessarily enables SRIOV BARs of the device.

On some platforms, which do not support SRIOV as yet, the pci_enable_device() fails to enable the device if its SRIOV BARs are not allocated resources correctly.

The following patch fixes the above problem. The SRIOV BARs are now enabled when IOV capability of the device is enabled in sriov_enable().

NOTE: Note, there is subtle change in the pci_enable_device() API. Any driver that depends on SRIOV BARS to be enabled in pci_enable_device() can fail.

Quoted 6 lines

Quoted 4 lines

The patch has been touch tested on power and x86 platform.

Signed-off-by Ram Pai<[email protected]>

---

I think this is a good direction, but I'd like to get a tested-by or two from people using SR-IOV...

I have people within IBM who have tested this patch successfully. Michael Wang/Nish: Can you provide us a tested-by for this patch?

Prarit: Can you give this patch and the other patch a try?
the other patch is: [RFC PATCH 1/1] PCI:delay configuration of SRIOV capability

RP

All the PCI BARs of a device are enabled when the device is enabled using pci_enable_device(). This unnecessarily enables SRIOV BARs of the device.

On some platforms, which do not support SRIOV as yet, the pci_enable_device() fails to enable the device if its SRIOV BARs are not allocated resources correctly.

The following patch fixes the above problem. The SRIOV BARs are now enabled when IOV capability of the device is enabled in sriov_enable().

NOTE: Note, there is subtle change in the pci_enable_device() API. Any driver that depends on SRIOV BARS to be enabled in pci_enable_device() can fail.

The patch has been touch tested on power and x86 platform.

Signed-off-by Ram Pai <[email protected]>

---

Quoted 1 line · On Sun, 6 Nov 2011 10:33:10 +0800 Ram Pai <[email protected]> wrote:

Quoted 5 lines

  NOTE: Note, there is subtle change in the pci_enable_device() API. Any driver that depends on SRIOV BARS to be enabled in pci_enable_device() can fail.

---

Applied to my for-linus branch, thanks.

Quoted 4 lines · Jesse Barnes

Quoted 1 line · On Sun, 6 Nov 2011 10:33:10 +0800 Ram Pai <[email protected]> wrote:

Quoted 8 lines

Quoted 5 lines

  NOTE: Note, there is subtle change in the pci_enable_device() API. Any driver that depends on SRIOV BARS to be enabled in pci_enable_device() can fail.

---

Applied to my for-linus branch, thanks.

please don't push to linus now.

this one causes regression.

please check attached patch.

Thanks

Yinghai

[PATCH] pci: Fix hotplug of Express Module with pci bridges

Found hotplug of one setup does not work with recent change in pci tree.

After checking the bridge conf setup, found bridges get assigned, but not get enabled.

Finally found following commit, simplely ignore bridge resource when enabling pci device.

| commit bbef98ab0f019f1b0c25c1acdf1683c68933d41b
| Author: Ram Pai <[email protected]>
| Date:   Sun Nov 6 10:33:10 2011 +0800
|
|    PCI: defer enablement of SRIOV BARS
|...
|    NOTE: Note, there is subtle change in the pci_enable_device() API.  Any
|    driver that depends on SRIOV BARS to be enabled in pci_enable_device()
|    can fail.

Put back bridge resource and ROM resource checking to fix the problem.

That should fix regression like BIOS does not assign correct resource to bridge.

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/pci/pci.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================

drivers/pci/pci.c +0 -0

@@ -1139,7 +1139,11 @@ static int __pci_enable_device_flags(str
 	if (atomic_add_return(1, &dev->enable_cnt) > 1)
 		return 0;		/* already enabled */

Quoted 60 lines · Yinghai Lu

Quoted 4 lines · Jesse Barnes

Quoted 1 line · On Sun, 6 Nov 2011 10:33:10 +0800 Ram Pai <[email protected]> wrote:

Quoted 8 lines

Quoted 5 lines

  NOTE: Note, there is subtle change in the pci_enable_device() API. Any driver that depends on SRIOV BARS to be enabled in pci_enable_device() can fail.

---

Applied to my for-linus branch, thanks.

please don't push to linus now.

this one causes regression.

please check attached patch.

Thanks

Yinghai

[PATCH] pci: Fix hotplug of Express Module with pci bridges

Found hotplug of one setup does not work with recent change in pci tree.

After checking the bridge conf setup, found bridges get assigned, but not get enabled.

Finally found following commit, simplely ignore bridge resource when enabling pci device.

| commit bbef98ab0f019f1b0c25c1acdf1683c68933d41b
| Author: Ram Pai <[email protected]>
| Date:   Sun Nov 6 10:33:10 2011 +0800
|
|    PCI: defer enablement of SRIOV BARS
|...
|    NOTE: Note, there is subtle change in the pci_enable_device() API.  Any
|    driver that depends on SRIOV BARS to be enabled in pci_enable_device()
|    can fail.

Put back bridge resource and ROM resource checking to fix the problem.

That should fix regression like BIOS does not assign correct resource to bridge.

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/pci/pci.c |    6 +++++-

 1 file changed, 5 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================

drivers/pci/pci.c +0 -0

@@ -1139,7 +1139,11 @@ static int __pci_enable_device_flags(str
       if (atomic_add_return(1, &dev->enable_cnt) > 1)

              return 0;               /* already enabled */

Quoted 9 lines

-     for (i = 0; i < PCI_ROM_RESOURCE; i++)
+     /* only skip sriov related */
+     for (i = 0; i <= PCI_ROM_RESOURCE; i++)
+             if (dev->resource[i].flags & flags)
+                     bars |= (1 << i);
+     for (i = PCI_BRIDGE_RESOURCES; i < DEVICE_COUNT_RESOURCE; i++) if (dev->resource[i].flags & flags)
                      bars |= (1 << i);

Oops. My patch inadvertently dropped ROM and BRIDGE resources.

This patch is right. However would it help if we did something like this to avoid some code duplication?

       for (i = 0; i < DEVICE_COUNT_RESOURCE;
               i == PCI_ROM_RESOURCE ? PCI_BRIDGE_RESOURCES: i++) if (dev->resource[i].flags & flags)
                       bars |= (1 << i);

Quoted 59 lines · Yinghai Lu

Quoted 4 lines · Jesse Barnes

Quoted 1 line · On Sun, 6 Nov 2011 10:33:10 +0800 Ram Pai<[email protected]> wrote:

Quoted 8 lines

Quoted 5 lines

NOTE: Note, there is subtle change in the pci_enable_device() API. Any driver that depends on SRIOV BARS to be enabled in pci_enable_device() can fail.

---

Applied to my for-linus branch, thanks.

please don't push to linus now.

this one causes regression.

please check attached patch.

Thanks

Yinghai

[PATCH] pci: Fix hotplug of Express Module with pci bridges

Found hotplug of one setup does not work with recent change in pci tree.

After checking the bridge conf setup, found bridges get assigned, but not get enabled.

Finally found following commit, simplely ignore bridge resource when enabling pci device.

| commit bbef98ab0f019f1b0c25c1acdf1683c68933d41b
| Author: Ram Pai<[email protected]>
| Date:   Sun Nov 6 10:33:10 2011 +0800
|
|    PCI: defer enablement of SRIOV BARS
|...
|    NOTE: Note, there is subtle change in the pci_enable_device() API.  Any
|    driver that depends on SRIOV BARS to be enabled in pci_enable_device()
|    can fail.

Put back bridge resource and ROM resource checking to fix the problem.

That should fix regression like BIOS does not assign correct resource to bridge.

Signed-off-by Yinghai Lu<[email protected]>

---
  drivers/pci/pci.c |    6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================

drivers/pci/pci.c +0 -0

@@ -1139,7 +1139,11 @@ static int __pci_enable_device_flags(str
  	if (atomic_add_return(1,&dev->enable_cnt)>  1)
  		return 0;		/* already enabled */

drivers/pci/pci.c static int __pci_enable_device_flags(str · line 1149

Quoted 10 lines

drivers/pci/pci.c +5 -1

 
-	for (i = 0; i<  PCI_ROM_RESOURCE; i++)
+	/* only skip sriov related */
+	for (i = 0; i<= PCI_ROM_RESOURCE; i++)
+		if (dev->resource[i].flags&  flags)
+			bars |= (1<<  i);
+	for (i = PCI_BRIDGE_RESOURCES; i<  DEVICE_COUNT_RESOURCE; i++)
  		if (dev->resource[i].flags&  flags)
  			bars |= (1<<  i);
 

Oops. My patch inadvertently dropped ROM and BRIDGE resources.

This patch is right. However would it help if we did something like this to avoid some code duplication?

	for (i = 0; i<  DEVICE_COUNT_RESOURCE;
		i == PCI_ROM_RESOURCE ? PCI_BRIDGE_RESOURCES: i++)
		if (dev->resource[i].flags&  flags)
			bars |= (1<<  i);

RP

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 73 lines · Ram Pai

Quoted 59 lines · Yinghai Lu

Quoted 4 lines · Jesse Barnes

Quoted 1 line · On Sun, 6 Nov 2011 10:33:10 +0800 Ram Pai<[email protected]> wrote:

Quoted 8 lines

Quoted 5 lines

NOTE: Note, there is subtle change in the pci_enable_device() API. Any driver that depends on SRIOV BARS to be enabled in pci_enable_device() can fail.

---

Applied to my for-linus branch, thanks.

please don't push to linus now.

this one causes regression.

please check attached patch.

Thanks

Yinghai

[PATCH] pci: Fix hotplug of Express Module with pci bridges

Found hotplug of one setup does not work with recent change in pci tree.

After checking the bridge conf setup, found bridges get assigned, but not get enabled.

Finally found following commit, simplely ignore bridge resource when enabling pci device.

| commit bbef98ab0f019f1b0c25c1acdf1683c68933d41b
| Author: Ram Pai<[email protected]>
| Date:   Sun Nov 6 10:33:10 2011 +0800
|
|    PCI: defer enablement of SRIOV BARS
|...
|    NOTE: Note, there is subtle change in the pci_enable_device() API.  Any
|    driver that depends on SRIOV BARS to be enabled in pci_enable_device()
|    can fail.

Put back bridge resource and ROM resource checking to fix the problem.

That should fix regression like BIOS does not assign correct resource to bridge.

Signed-off-by Yinghai Lu<[email protected]>

---
 drivers/pci/pci.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================

drivers/pci/pci.c +0 -0

@@ -1139,7 +1139,11 @@ static int __pci_enable_device_flags(str
 	if (atomic_add_return(1,&dev->enable_cnt)>  1)
 		return 0;		/* already enabled */

drivers/pci/pci.c static int __pci_enable_device_flags(str · line 1149

Quoted 10 lines

drivers/pci/pci.c +5 -1

 
-	for (i = 0; i<  PCI_ROM_RESOURCE; i++)
+	/* only skip sriov related */
+	for (i = 0; i<= PCI_ROM_RESOURCE; i++)
+		if (dev->resource[i].flags&  flags)
+			bars |= (1<<  i);
+	for (i = PCI_BRIDGE_RESOURCES; i<  DEVICE_COUNT_RESOURCE; i++)
 		if (dev->resource[i].flags&  flags)
 			bars |= (1<<  i);
 

Oops. My patch inadvertently dropped ROM and BRIDGE resources.

Quoted 8 lines

This patch is right. However would it help if we did something like this to avoid some code duplication?

	for (i = 0; i<  DEVICE_COUNT_RESOURCE;
		i == PCI_ROM_RESOURCE ? PCI_BRIDGE_RESOURCES: i++)
		if (dev->resource[i].flags&  flags)
			bars |= (1<<  i);

why not something more explicit like:

	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
		if ((i >=  PCI_IOV_RESOURCES) && (i <= PCI_IOV_RESOURCE_END))
			continue; /* skip sriov related resources */
                if (dev->resource[i].flags & flags)
                        bars |= (1 << i);
	}

Quoted 83 lines · Don Dutile

Quoted 73 lines · Ram Pai

Quoted 59 lines · Yinghai Lu

Quoted 4 lines · Jesse Barnes

Quoted 1 line · On Sun, 6 Nov 2011 10:33:10 +0800 Ram Pai<[email protected]> wrote:

Quoted 8 lines

Quoted 5 lines

NOTE: Note, there is subtle change in the pci_enable_device() API. Any driver that depends on SRIOV BARS to be enabled in pci_enable_device() can fail.

---

Applied to my for-linus branch, thanks.

please don't push to linus now.

this one causes regression.

please check attached patch.

Thanks

Yinghai

[PATCH] pci: Fix hotplug of Express Module with pci bridges

Found hotplug of one setup does not work with recent change in pci tree.

After checking the bridge conf setup, found bridges get assigned, but not get enabled.

Finally found following commit, simplely ignore bridge resource when enabling pci device.

| commit bbef98ab0f019f1b0c25c1acdf1683c68933d41b
| Author: Ram Pai<[email protected]>
| Date:   Sun Nov 6 10:33:10 2011 +0800
|
|    PCI: defer enablement of SRIOV BARS
|...
|    NOTE: Note, there is subtle change in the pci_enable_device() API.  Any
|    driver that depends on SRIOV BARS to be enabled in pci_enable_device()
|    can fail.

Put back bridge resource and ROM resource checking to fix the problem.

That should fix regression like BIOS does not assign correct resource to bridge.

Signed-off-by Yinghai Lu<[email protected]>

---
  drivers/pci/pci.c |    6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================

drivers/pci/pci.c +0 -0

@@ -1139,7 +1139,11 @@ static int __pci_enable_device_flags(str
  	if (atomic_add_return(1,&dev->enable_cnt)>   1)
  		return 0;		/* already enabled */

drivers/pci/pci.c static int __pci_enable_device_flags(str · line 1149

Quoted 10 lines

drivers/pci/pci.c +5 -1

 
-	for (i = 0; i<   PCI_ROM_RESOURCE; i++)
+	/* only skip sriov related */
+	for (i = 0; i<= PCI_ROM_RESOURCE; i++)
+		if (dev->resource[i].flags&   flags)
+			bars |= (1<<   i);
+	for (i = PCI_BRIDGE_RESOURCES; i<   DEVICE_COUNT_RESOURCE; i++)
  		if (dev->resource[i].flags&   flags)
  			bars |= (1<<   i);
 

Oops. My patch inadvertently dropped ROM and BRIDGE resources.

Quoted 8 lines

This patch is right. However would it help if we did something like this to avoid some code duplication?

	for (i = 0; i<   DEVICE_COUNT_RESOURCE;
		i == PCI_ROM_RESOURCE ? PCI_BRIDGE_RESOURCES: i++)
		if (dev->resource[i].flags&   flags)
			bars |= (1<<   i);

Quoted 8 lines

why not something more explicit like:

	for (i = 0; i<  DEVICE_COUNT_RESOURCE; i++) {
		if ((i>=  PCI_IOV_RESOURCES)&&  (i<= PCI_IOV_RESOURCE_END))
			continue; /* skip sriov related resources */
                 if (dev->resource[i].flags&  flags)
                         bars |= (1<<  i);
	}

I agree with the more explicit approach, self-commenting and easier to read. Although, you may want to add a comment as to *why* we're skipping IOV BARs here.

-Nish

why not something more explicit like:

       for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
               if ((i >=  PCI_IOV_RESOURCES) && (i <= PCI_IOV_RESOURCE_END)) continue; /* skip sriov related resources */

               if (dev->resource[i].flags & flags) bars |= (1 << i);
       }

Quoted 9 lines · Don Dutile

why not something more explicit like:

       for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
               if ((i >=  PCI_IOV_RESOURCES) && (i <= PCI_IOV_RESOURCE_END)) continue; /* skip sriov related resources */

               if (dev->resource[i].flags & flags) bars |= (1 << i);
       }

Quoted 10 lines · Yinghai Lu

Quoted 9 lines · Don Dutile

why not something more explicit like:

       for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
               if ((i >=  PCI_IOV_RESOURCES) && (i <= PCI_IOV_RESOURCE_END)) continue; /* skip sriov related resources */

               if (dev->resource[i].flags & flags) bars |= (1 << i);
       }

I like this approach too. Offcourse the SRIOV skipping has to be one under #ifdef CONFIG_PCI_IOV.

Yinghai/Jesse, do you want to make a patch on top of the current jesse's to-linus tree or want to revert my patch and apply your fix? If it is the former, do you want me to make the patch?

Quoted 8 lines · On Thu, 8 Dec 2011 10:50:39 +0800 Ram Pai <[email protected]> wrote:

Quoted 7 lines · Yinghai Lu

Quoted 6 lines · Don Dutile

why not something more explicit like:

       for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
               if ((i >=  PCI_IOV_RESOURCES) && (i <= PCI_IOV_RESOURCE_END)) continue; /* skip sriov related resources */

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

               if (dev->resource[i].flags & flags) bars |= (1 << i);
       }

I like this approach too. Offcourse the SRIOV skipping has to be one under #ifdef CONFIG_PCI_IOV.

Quoted 4 lines

Yinghai/Jesse, do you want to make a patch on top of the current jesse's to-linus tree or want to revert my patch and apply your fix? If it is the former, do you want me to make the patch?

I like the above best as well with the comment.

Can you send me a replacement patch?  I'd rather have that than some breakage in the patches I have in my for-linus branch.

Quoted 7 lines · Yinghai Lu

Quoted 6 lines · Don Dutile

why not something more explicit like:

        for (i = 0; i<  DEVICE_COUNT_RESOURCE; i++) {
                if ((i>=  PCI_IOV_RESOURCES)&&  (i<= PCI_IOV_RESOURCE_END))
                        continue; /* skip sriov related resources */

Quoted 3 lines

Quoted 3 lines

                if (dev->resource[i].flags&  flags)
                        bars |= (1<<  i);
        }

I like this approach too. Offcourse the SRIOV skipping has to be one under #ifdef CONFIG_PCI_IOV.

Yinghai/Jesse, do you want to make a patch on top of the current jesse's to-linus tree or want to revert my patch and apply your fix? If it is the former, do you want me to make the patch?

RP

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

The SRIOV capability, namely page size and total_vfs of a device are configured during enumeration phase of the device.
This can potentially interfere with the PCI operations of the platform, if the IOV capability of the device is not enabled.

The following patch postpones the configuration of the IOV capability of the device to a later point, when the IOV capability is explicitly enabled by the device driver.

The patch is tested on x86 and power platform.

Signed-off-by Ram Pai <[email protected]>

---
 drivers/pci/iov.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/pci/iov.c +2 -1

index b0446dd..c1a6f5c 100644
@@ -346,6 +346,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 			return rc;
 	}
 
+	pci_write_config_dword(dev, iov->pos + PCI_SRIOV_SYS_PGSIZE, iov->pgsz);
+
 	iov->ctrl |= PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE;
 	pci_block_user_cfg_access(dev);
 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);

@@ -451,7 +453,6 @@ static int sriov_init(struct pci_dev *dev, int pos)
 
 found:
 	pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);
-	pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, total);
 	pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET, &offset);
 	pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE, &stride);
 	if (!offset || (total > 1 && !stride))

Quoted 43 lines · On Sun, 6 Nov 2011 10:33:57 +0800 Ram Pai<[email protected]> wrote:

The SRIOV capability, namely page size and total_vfs of a device are configured during enumeration phase of the device.
This can potentially interfere with the PCI operations of the platform, if the IOV capability of the device is not enabled.

The following patch postpones the configuration of the IOV capability of the device to a later point, when the IOV capability is explicitly enabled by the device driver.

The patch is tested on x86 and power platform.

Signed-off-by Ram Pai<[email protected]>

---
  drivers/pci/iov.c |    4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)

drivers/pci/iov.c +2 -2

index b0446dd..c1a6f5c 100644
@@ -346,6 +346,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
  			return rc;
  	}
 
+	pci_write_config_dword(dev, iov->pos + PCI_SRIOV_SYS_PGSIZE, iov->pgsz);
+
  	iov->ctrl |= PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE;
  	pci_block_user_cfg_access(dev);
  	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);

@@ -451,7 +453,6 @@ static int sriov_init(struct pci_dev *dev, int pos)
 
  found:
  	pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);
-	pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, total);
  	pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET,&offset);
  	pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE,&stride);
  	if (!offset || (total>  1&&  !stride))

@@ -464,7 +465,6 @@ found:
  		return -EIO;
 
  	pgsz&= ~(pgsz - 1);
-	pci_write_config_dword(dev, pos + PCI_SRIOV_SYS_PGSIZE, pgsz);
 

drivers/pci/iov.c found: · line 470

Quoted 2 lines

drivers/pci/iov.c +0 -0

  	nres = 0;
  	for (i = 0; i<  PCI_SRIOV_NUM_BARS; i++) {

Anyone want to volunteer a tested-by for this one?

Thanks,

Quoted 46 lines · Jesse Barnes

Quoted 43 lines · On Sun, 6 Nov 2011 10:33:57 +0800 Ram Pai<[email protected]> wrote:

The SRIOV capability, namely page size and total_vfs of a device are configured during enumeration phase of the device.
This can potentially interfere with the PCI operations of the platform, if the IOV capability of the device is not enabled.

The following patch postpones the configuration of the IOV capability of the device to a later point, when the IOV capability is explicitly enabled by the device driver.

The patch is tested on x86 and power platform.

Signed-off-by Ram Pai<[email protected]>

---
 drivers/pci/iov.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/pci/iov.c +2 -2

index b0446dd..c1a6f5c 100644
@@ -346,6 +346,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 			return rc;
 	}
 
+	pci_write_config_dword(dev, iov->pos + PCI_SRIOV_SYS_PGSIZE, iov->pgsz);
+
 	iov->ctrl |= PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE;
 	pci_block_user_cfg_access(dev);
 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);

@@ -451,7 +453,6 @@ static int sriov_init(struct pci_dev *dev, int pos)
 
 found:
 	pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);
-	pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, total);
 	pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET,&offset);
 	pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE,&stride);
 	if (!offset || (total>  1&&  !stride))

@@ -464,7 +465,6 @@ found:
 		return -EIO;
 
 	pgsz&= ~(pgsz - 1);
-	pci_write_config_dword(dev, pos + PCI_SRIOV_SYS_PGSIZE, pgsz);
 

Quoted 6 lines

drivers/pci/iov.c found: · line 470

Quoted 2 lines

drivers/pci/iov.c +0 -0

 	nres = 0;
 	for (i = 0; i<  PCI_SRIOV_NUM_BARS; i++) {

Anyone want to volunteer a tested-by for this one?

Thanks,

I tested this patch along with Ram's other recent patch for SRIOV:
[RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS

With or without this patch, device assignment to a KVM guest using these patches worked for me. I used one PF in a quad-function i350, configuring 2 VFs per i350. I did 4 assign/deassigns to make sure multiple enable/disables worked.

So, the patch doesn't correct any error I was seeing, but it doesn't cause any regression either.

Quoted 57 lines · Don Dutile

Quoted 46 lines · Jesse Barnes

Quoted 43 lines · On Sun, 6 Nov 2011 10:33:57 +0800 Ram Pai<[email protected]> wrote:

The SRIOV capability, namely page size and total_vfs of a device are configured during enumeration phase of the device.
This can potentially interfere with the PCI operations of the platform, if the IOV capability of the device is not enabled.

The following patch postpones the configuration of the IOV capability of the device to a later point, when the IOV capability is explicitly enabled by the device driver.

The patch is tested on x86 and power platform.

Signed-off-by Ram Pai<[email protected]>

---
  drivers/pci/iov.c |    4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)

drivers/pci/iov.c +2 -2

index b0446dd..c1a6f5c 100644
@@ -346,6 +346,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
  			return rc;
  	}
 
+	pci_write_config_dword(dev, iov->pos + PCI_SRIOV_SYS_PGSIZE, iov->pgsz);
+
  	iov->ctrl |= PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE;
  	pci_block_user_cfg_access(dev);
  	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);

@@ -451,7 +453,6 @@ static int sriov_init(struct pci_dev *dev, int pos)
 
  found:
  	pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);
-	pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, total);
  	pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET,&offset);
  	pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE,&stride);
  	if (!offset || (total>   1&&   !stride))

@@ -464,7 +465,6 @@ found:
  		return -EIO;
 
  	pgsz&= ~(pgsz - 1);
-	pci_write_config_dword(dev, pos + PCI_SRIOV_SYS_PGSIZE, pgsz);
 

Quoted 6 lines

drivers/pci/iov.c found: · line 470

Quoted 2 lines

drivers/pci/iov.c +0 -0

  	nres = 0;
  	for (i = 0; i<   PCI_SRIOV_NUM_BARS; i++) {

Anyone want to volunteer a tested-by for this one?

Thanks,

I tested this patch along with Ram's other recent patch for SRIOV:
[RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS

Quoted 7 lines

With or without this patch, device assignment to a KVM guest using these patches worked for me. I used one PF in a quad-function i350, configuring 2 VFs per i350. I did 4 assign/deassigns to make sure multiple enable/disables worked.

So, the patch doesn't correct any error I was seeing, but it doesn't cause any regression either.

Thanks Don for your tested-by.

I recollect you had a issue, which I thought were not related or addressed by my patches. If I can get a dmesg of your machine, I probably will be able to see the relation.

RP

The SRIOV capability, namely page size and total_vfs of a device are configured during enumeration phase of the device.
This can potentially interfere with the PCI operations of the platform, if the IOV capability of the device is not enabled.

The following patch postpones the configuration of the IOV capability of the device to a later point, when the IOV capability is explicitly enabled by the device driver.

The patch is tested on x86 and power platform.

Signed-off-by Ram Pai <[email protected]>

---
 drivers/pci/iov.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/pci/iov.c +2 -1

index b0446dd..c1a6f5c 100644
@@ -346,6 +346,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 			return rc;
 	}
 
+	pci_write_config_dword(dev, iov->pos + PCI_SRIOV_SYS_PGSIZE, iov->pgsz);
+
 	iov->ctrl |= PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE;
 	pci_block_user_cfg_access(dev);
 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);

@@ -451,7 +453,6 @@ static int sriov_init(struct pci_dev *dev, int pos)
 
 found:
 	pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);
-	pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, total);
 	pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET, &offset);
 	pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE, &stride);
 	if (!offset || (total > 1 && !stride))