Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1259047 > unrolled thread

[PATCH v2 0/7] SR-IOV fixes and cleanup

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/7] SR-IOV fixes and cleanup Bjorn Helgaas <bhelgaas@google.com> - 2015-10-29 23:30 +0100
    [PATCH v2 7/7] PCI: Set NumVFs before computing how many buses VFs  require Bjorn Helgaas <bhelgaas@google.com> - 2015-10-29 23:30 +0100
      Re: [PATCH v2 7/7] PCI: Set NumVFs before computing how many buses  VFs require Wei Yang <weiyang@linux.vnet.ibm.com> - 2015-10-30 06:30 +0100
        Re: [PATCH v2 7/7] PCI: Set NumVFs before computing how many buses  VFs require Alexander Duyck <alexander.duyck@gmail.com> - 2015-10-30 17:10 +0100
    [PATCH v2 4/7] PCI: Reorder pcibios_sriov_disable() Bjorn Helgaas <bhelgaas@google.com> - 2015-10-29 23:30 +0100
      Re: [PATCH v2 4/7] PCI: Reorder pcibios_sriov_disable() Wei Yang <weiyang@linux.vnet.ibm.com> - 2015-10-30 06:20 +0100
    [PATCH v2 2/7] PCI: Remove redundant validation of SR-IOV  offset/stride registers Bjorn Helgaas <bhelgaas@google.com> - 2015-10-29 23:30 +0100
      Re: [PATCH v2 2/7] PCI: Remove redundant validation of SR-IOV  offset/stride registers Wei Yang <weiyang@linux.vnet.ibm.com> - 2015-10-30 06:10 +0100
    [PATCH v2 6/7] PCI: Fix sriov_enable() error path for  pcibios_enable_sriov() failures Bjorn Helgaas <bhelgaas@google.com> - 2015-10-29 23:30 +0100
      Re: [PATCH v2 6/7] PCI: Fix sriov_enable() error path for  pcibios_enable_sriov() failures Wei Yang <weiyang@linux.vnet.ibm.com> - 2015-10-30 06:20 +0100
    [PATCH v2 3/7] PCI: Remove VFs in reverse order if virtfn_add()  fails Bjorn Helgaas <bhelgaas@google.com> - 2015-10-29 23:30 +0100
      Re: [PATCH v2 3/7] PCI: Remove VFs in reverse order if virtfn_add()  fails Wei Yang <weiyang@linux.vnet.ibm.com> - 2015-10-30 06:10 +0100
    [PATCH v2 5/7] PCI: Wait 1 second between disabling VFs and  clearing NumVFs Bjorn Helgaas <bhelgaas@google.com> - 2015-10-29 23:30 +0100
      Re: [PATCH v2 5/7] PCI: Wait 1 second between disabling VFs and  clearing NumVFs Wei Yang <weiyang@linux.vnet.ibm.com> - 2015-10-30 06:20 +0100
        Re: [PATCH v2 5/7] PCI: Wait 1 second between disabling VFs and clearing  NumVFs ethan zhao <ethan.zhao@oracle.com> - 2015-10-30 07:10 +0100
          Re: [PATCH v2 5/7] PCI: Wait 1 second between disabling VFs and  clearing NumVFs Alexander Duyck <alexander.duyck@gmail.com> - 2015-10-30 17:00 +0100

#1259047 — [PATCH v2 0/7] SR-IOV fixes and cleanup

This is basically a revision of Alex's recent series:
http://lkml.kernel.org/r/20151027204607.14626.59671.stgit@localhost.localdomain

I squashed some fixes into the original patch, dropped two, and split
the rest up a little differently:

  iov: Update virtfn_max_buses to validate offset and stride
    Folded into the patch it fixed

  iov: Reset resources to 0 if totalVFs increases after enabling ARI
    Dropped on the theory that the resource allocation code
    should already handle this case

  iov: Fix sriov_enable exception handling path
    Split into a couple patches to make it easier to review

  iov: Variable and loop cleanup for sriov_disable and sriov_enable
    Dropped for readability

  iov: Update sriov_enable to correctly handle offset and stride
    Split into the piece that removes code made redundant by "PCI: Set
    SR-IOV NumVFs to zero after enumeration" and the piece that fixes
    the bug (using offset/stride before setting numVFs)

This is all on my pci/virtualization branch, and I'm still hoping to
merge this for v4.4.

---

Alexander Duyck (7):
      PCI: Set SR-IOV NumVFs to zero after enumeration
      PCI: Remove redundant validation of SR-IOV offset/stride registers
      PCI: Remove VFs in reverse order if virtfn_add() fails
      PCI: Reorder pcibios_sriov_disable()
      PCI: Wait 1 second between disabling VFs and clearing NumVFs
      PCI: Fix sriov_enable() error path for pcibios_enable_sriov() failures
      PCI: Set NumVFs before computing how many buses VFs require


 drivers/pci/iov.c |  100 ++++++++++++++++++++++++++---------------------------
 1 file changed, 49 insertions(+), 51 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1259050 — [PATCH v2 7/7] PCI: Set NumVFs before computing how many buses VFs require

From: Alexander Duyck <aduyck@mirantis.com>

VF bus numbers depend on the First VF Offset and VF Stride, and per
sections 3.3.9 and 3.3.10 of the SR-IOV spec r1.1, these depend on the
NumVF value.

Wait until after we set NumVFs to compute and validate the bus number of
the last VF.

[bhelgaas: changelog, add spec reference, split to separate patch for
reviewability]
Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/iov.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index bd1c4fa..9d29712 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -274,13 +274,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 		return -ENOMEM;
 	}
 
-	bus = pci_iov_virtfn_bus(dev, nr_virtfn - 1);
-	if (bus > dev->bus->busn_res.end) {
-		dev_err(&dev->dev, "can't enable %d VFs (bus %02x out of range of %pR)\n",
-			nr_virtfn, bus, &dev->bus->busn_res);
-		return -ENOMEM;
-	}
-
 	if (pci_enable_resources(dev, bars)) {
 		dev_err(&dev->dev, "SR-IOV: IOV BARS not allocated\n");
 		return -ENOMEM;
@@ -304,6 +297,15 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 	}
 
 	pci_iov_set_numvfs(dev, nr_virtfn);
+
+	bus = pci_iov_virtfn_bus(dev, nr_virtfn - 1);
+	if (bus > dev->bus->busn_res.end) {
+		dev_err(&dev->dev, "can't enable %d VFs (bus %02x out of range of %pR)\n",
+			nr_virtfn, bus, &dev->bus->busn_res);
+		rc = -ENOMEM;
+		goto err_bus;
+	}
+
 	iov->ctrl |= PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE;
 	pci_cfg_access_lock(dev);
 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
@@ -342,7 +344,7 @@ err_pcibios:
 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
 	ssleep(1);
 	pci_cfg_access_unlock(dev);
-
+err_bus:
 	if (iov->link != dev->devfn)
 		sysfs_remove_link(&dev->dev.kobj, "dep_link");
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259211 — Re: [PATCH v2 7/7] PCI: Set NumVFs before computing how many buses VFs require

On Thu, Oct 29, 2015 at 05:23:36PM -0500, Bjorn Helgaas wrote:
>From: Alexander Duyck <aduyck@mirantis.com>
>
>VF bus numbers depend on the First VF Offset and VF Stride, and per
>sections 3.3.9 and 3.3.10 of the SR-IOV spec r1.1, these depend on the
>NumVF value.
>
>Wait until after we set NumVFs to compute and validate the bus number of
>the last VF.
>
>[bhelgaas: changelog, add spec reference, split to separate patch for
>reviewability]
>Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
>Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>---
> drivers/pci/iov.c |   18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>index bd1c4fa..9d29712 100644
>--- a/drivers/pci/iov.c
>+++ b/drivers/pci/iov.c
>@@ -274,13 +274,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
> 		return -ENOMEM;
> 	}
>
>-	bus = pci_iov_virtfn_bus(dev, nr_virtfn - 1);
>-	if (bus > dev->bus->busn_res.end) {
>-		dev_err(&dev->dev, "can't enable %d VFs (bus %02x out of range of %pR)\n",
>-			nr_virtfn, bus, &dev->bus->busn_res);
>-		return -ENOMEM;
>-	}
>-
> 	if (pci_enable_resources(dev, bars)) {
> 		dev_err(&dev->dev, "SR-IOV: IOV BARS not allocated\n");
> 		return -ENOMEM;
>@@ -304,6 +297,15 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
> 	}
>
> 	pci_iov_set_numvfs(dev, nr_virtfn);

How about move it up?

>+
>+	bus = pci_iov_virtfn_bus(dev, nr_virtfn - 1);
>+	if (bus > dev->bus->busn_res.end) {
>+		dev_err(&dev->dev, "can't enable %d VFs (bus %02x out of range of %pR)\n",
>+			nr_virtfn, bus, &dev->bus->busn_res);
>+		rc = -ENOMEM;
>+		goto err_bus;
>+	}
>+
> 	iov->ctrl |= PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE;
> 	pci_cfg_access_lock(dev);
> 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
>@@ -342,7 +344,7 @@ err_pcibios:
> 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
> 	ssleep(1);
> 	pci_cfg_access_unlock(dev);
>-
>+err_bus:
> 	if (iov->link != dev->devfn)
> 		sysfs_remove_link(&dev->dev.kobj, "dep_link");
>

-- 
Richard Yang
Help you, Help me

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259602 — Re: [PATCH v2 7/7] PCI: Set NumVFs before computing how many buses VFs require

On 10/29/2015 10:22 PM, Wei Yang wrote:
> On Thu, Oct 29, 2015 at 05:23:36PM -0500, Bjorn Helgaas wrote:
>> From: Alexander Duyck <aduyck@mirantis.com>
>>
>> VF bus numbers depend on the First VF Offset and VF Stride, and per
>> sections 3.3.9 and 3.3.10 of the SR-IOV spec r1.1, these depend on the
>> NumVF value.
>>
>> Wait until after we set NumVFs to compute and validate the bus number of
>> the last VF.
>>
>> [bhelgaas: changelog, add spec reference, split to separate patch for
>> reviewability]
>> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> ---
>> drivers/pci/iov.c |   18 ++++++++++--------
>> 1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index bd1c4fa..9d29712 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -274,13 +274,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
>> 		return -ENOMEM;
>> 	}
>>
>> -	bus = pci_iov_virtfn_bus(dev, nr_virtfn - 1);
>> -	if (bus > dev->bus->busn_res.end) {
>> -		dev_err(&dev->dev, "can't enable %d VFs (bus %02x out of range of %pR)\n",
>> -			nr_virtfn, bus, &dev->bus->busn_res);
>> -		return -ENOMEM;
>> -	}
>> -
>> 	if (pci_enable_resources(dev, bars)) {
>> 		dev_err(&dev->dev, "SR-IOV: IOV BARS not allocated\n");
>> 		return -ENOMEM;
>> @@ -304,6 +297,15 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
>> 	}
>>
>> 	pci_iov_set_numvfs(dev, nr_virtfn);
> How about move it up?

The idea with moving the write down is to keep the pollution of the 
SR-IOV capability to a minimum.  Basically we have addressed all of the 
possible software issues at this point so all that remains is possible 
hardware complications.  In addition by moving this code down we only 
have to modify this code instead of adding "rc=X; goto foo;" in places 
where "return X;" was used.

Also this is an exception case.  There really isn't much point in 
optimizing for something that should never really happen.

>> +
>> +	bus = pci_iov_virtfn_bus(dev, nr_virtfn - 1);
>> +	if (bus > dev->bus->busn_res.end) {
>> +		dev_err(&dev->dev, "can't enable %d VFs (bus %02x out of range of %pR)\n",
>> +			nr_virtfn, bus, &dev->bus->busn_res);
>> +		rc = -ENOMEM;
>> +		goto err_bus;
>> +	}
>> +
>> 	iov->ctrl |= PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE;
>> 	pci_cfg_access_lock(dev);
>> 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
>> @@ -342,7 +344,7 @@ err_pcibios:
>> 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
>> 	ssleep(1);
>> 	pci_cfg_access_unlock(dev);
>> -
>> +err_bus:
>> 	if (iov->link != dev->devfn)
>> 		sysfs_remove_link(&dev->dev.kobj, "dep_link");
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259051 — [PATCH v2 4/7] PCI: Reorder pcibios_sriov_disable()

From: Alexander Duyck <aduyck@mirantis.com>

Move pcibios_sriov_disable() up so it's defined before a future use.

[bhelgaas: split to separate patch for reviewability]
Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/iov.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index c86d94c..fada98d 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -227,7 +227,12 @@ static void virtfn_remove(struct pci_dev *dev, int id, int reset)
 
 int __weak pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
 {
-       return 0;
+	return 0;
+}
+
+int __weak pcibios_sriov_disable(struct pci_dev *pdev)
+{
+	return 0;
 }
 
 static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
@@ -344,11 +349,6 @@ failed:
 	return rc;
 }
 
-int __weak pcibios_sriov_disable(struct pci_dev *pdev)
-{
-       return 0;
-}
-
 static void sriov_disable(struct pci_dev *dev)
 {
 	int i;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259207 — Re: [PATCH v2 4/7] PCI: Reorder pcibios_sriov_disable()

On Thu, Oct 29, 2015 at 05:23:15PM -0500, Bjorn Helgaas wrote:
>From: Alexander Duyck <aduyck@mirantis.com>
>
>Move pcibios_sriov_disable() up so it's defined before a future use.
>
>[bhelgaas: split to separate patch for reviewability]
>Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
>Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Wei Yang <weiyang@linux.vnet.ibm.com>

>---
> drivers/pci/iov.c |   12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>index c86d94c..fada98d 100644
>--- a/drivers/pci/iov.c
>+++ b/drivers/pci/iov.c
>@@ -227,7 +227,12 @@ static void virtfn_remove(struct pci_dev *dev, int id, int reset)
>
> int __weak pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
> {
>-       return 0;
>+	return 0;
>+}
>+
>+int __weak pcibios_sriov_disable(struct pci_dev *pdev)
>+{
>+	return 0;
> }
>
> static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
>@@ -344,11 +349,6 @@ failed:
> 	return rc;
> }
>
>-int __weak pcibios_sriov_disable(struct pci_dev *pdev)
>-{
>-       return 0;
>-}
>-
> static void sriov_disable(struct pci_dev *dev)
> {
> 	int i;

-- 
Richard Yang
Help you, Help me

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259052 — [PATCH v2 2/7] PCI: Remove redundant validation of SR-IOV offset/stride registers

From: Alexander Duyck <aduyck@mirantis.com>

Previously, we read, validated, and cached PCI_SRIOV_VF_OFFSET and
PCI_SRIOV_VF_STRIDE in sriov_enable().  But sriov_init() now does
that via compute_max_vf_buses(), so we don't need to do it again.

Remove the PCI_SRIOV_VF_OFFSET and PCI_SRIOV_VF_STRIDE config reads from
sriov_enable().  The pci_sriov structure already contains the offset and
stride corresponding to the current NumVFs.

[bhelgaas: split to separate patch for reviewability]
Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/iov.c |   10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 1b1acc2..ca400a9 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -235,7 +235,7 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 	int rc;
 	int i, j;
 	int nres;
-	u16 offset, stride, initial;
+	u16 initial;
 	struct resource *res;
 	struct pci_dev *pdev;
 	struct pci_sriov *iov = dev->sriov;
@@ -258,11 +258,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 	    (!(iov->cap & PCI_SRIOV_CAP_VFM) && (nr_virtfn > initial)))
 		return -EINVAL;
 
-	pci_read_config_word(dev, iov->pos + PCI_SRIOV_VF_OFFSET, &offset);
-	pci_read_config_word(dev, iov->pos + PCI_SRIOV_VF_STRIDE, &stride);
-	if (!offset || (nr_virtfn > 1 && !stride))
-		return -EIO;
-
 	nres = 0;
 	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
 		bars |= (1 << (i + PCI_IOV_RESOURCES));
@@ -275,9 +270,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 		return -ENOMEM;
 	}
 
-	iov->offset = offset;
-	iov->stride = stride;
-
 	bus = pci_iov_virtfn_bus(dev, nr_virtfn - 1);
 	if (bus > dev->bus->busn_res.end) {
 		dev_err(&dev->dev, "can't enable %d VFs (bus %02x out of range of %pR)\n",

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259204 — Re: [PATCH v2 2/7] PCI: Remove redundant validation of SR-IOV offset/stride registers

On Thu, Oct 29, 2015 at 05:23:01PM -0500, Bjorn Helgaas wrote:
>From: Alexander Duyck <aduyck@mirantis.com>
>
>Previously, we read, validated, and cached PCI_SRIOV_VF_OFFSET and
>PCI_SRIOV_VF_STRIDE in sriov_enable().  But sriov_init() now does
>that via compute_max_vf_buses(), so we don't need to do it again.
>
>Remove the PCI_SRIOV_VF_OFFSET and PCI_SRIOV_VF_STRIDE config reads from
>sriov_enable().  The pci_sriov structure already contains the offset and
>stride corresponding to the current NumVFs.
>
>[bhelgaas: split to separate patch for reviewability]
>Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
>Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Wei Yang <weiyang@linux.vnet.ibm.com>

>---
> drivers/pci/iov.c |   10 +---------
> 1 file changed, 1 insertion(+), 9 deletions(-)
>
>diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>index 1b1acc2..ca400a9 100644
>--- a/drivers/pci/iov.c
>+++ b/drivers/pci/iov.c
>@@ -235,7 +235,7 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
> 	int rc;
> 	int i, j;
> 	int nres;
>-	u16 offset, stride, initial;
>+	u16 initial;
> 	struct resource *res;
> 	struct pci_dev *pdev;
> 	struct pci_sriov *iov = dev->sriov;
>@@ -258,11 +258,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
> 	    (!(iov->cap & PCI_SRIOV_CAP_VFM) && (nr_virtfn > initial)))
> 		return -EINVAL;
>
>-	pci_read_config_word(dev, iov->pos + PCI_SRIOV_VF_OFFSET, &offset);
>-	pci_read_config_word(dev, iov->pos + PCI_SRIOV_VF_STRIDE, &stride);
>-	if (!offset || (nr_virtfn > 1 && !stride))
>-		return -EIO;
>-
> 	nres = 0;
> 	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> 		bars |= (1 << (i + PCI_IOV_RESOURCES));
>@@ -275,9 +270,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
> 		return -ENOMEM;
> 	}
>
>-	iov->offset = offset;
>-	iov->stride = stride;
>-
> 	bus = pci_iov_virtfn_bus(dev, nr_virtfn - 1);
> 	if (bus > dev->bus->busn_res.end) {
> 		dev_err(&dev->dev, "can't enable %d VFs (bus %02x out of range of %pR)\n",

-- 
Richard Yang
Help you, Help me

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259055 — [PATCH v2 6/7] PCI: Fix sriov_enable() error path for pcibios_enable_sriov() failures

From: Alexander Duyck <aduyck@mirantis.com>

Disable VFs if pcibios_enable_sriov() fails, just like we do for other
errors in sriov_enable().  Call pcibios_sriov_disable() if virtfn_add()
fails.

[bhelgaas: changelog, split to separate patch for reviewability]
Fixes: 995df527f399 ("PCI: Add pcibios_sriov_enable() and pcibios_sriov_disable()")
Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/iov.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 24428d5..bd1c4fa 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -246,7 +246,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 	struct pci_sriov *iov = dev->sriov;
 	int bars = 0;
 	int bus;
-	int retval;
 
 	if (!nr_virtfn)
 		return 0;
@@ -315,10 +314,10 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 	if (nr_virtfn < initial)
 		initial = nr_virtfn;
 
-	if ((retval = pcibios_sriov_enable(dev, initial))) {
-		dev_err(&dev->dev, "failure %d from pcibios_sriov_enable()\n",
-			retval);
-		return retval;
+	rc = pcibios_sriov_enable(dev, initial);
+	if (rc) {
+		dev_err(&dev->dev, "failure %d from pcibios_sriov_enable()\n", rc);
+		goto err_pcibios;
 	}
 
 	for (i = 0; i < initial; i++) {
@@ -336,6 +335,8 @@ failed:
 	while (i--)
 		virtfn_remove(dev, i, 0);
 
+	pcibios_sriov_disable(dev);
+err_pcibios:
 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
 	pci_cfg_access_lock(dev);
 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259208 — Re: [PATCH v2 6/7] PCI: Fix sriov_enable() error path for pcibios_enable_sriov() failures

On Thu, Oct 29, 2015 at 05:23:29PM -0500, Bjorn Helgaas wrote:
>From: Alexander Duyck <aduyck@mirantis.com>
>
>Disable VFs if pcibios_enable_sriov() fails, just like we do for other
>errors in sriov_enable().  Call pcibios_sriov_disable() if virtfn_add()
>fails.
>
>[bhelgaas: changelog, split to separate patch for reviewability]
>Fixes: 995df527f399 ("PCI: Add pcibios_sriov_enable() and pcibios_sriov_disable()")
>Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
>Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Wei Yang <weiyang@linux.vnet.ibm.com>

>---
> drivers/pci/iov.c |   11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>index 24428d5..bd1c4fa 100644
>--- a/drivers/pci/iov.c
>+++ b/drivers/pci/iov.c
>@@ -246,7 +246,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
> 	struct pci_sriov *iov = dev->sriov;
> 	int bars = 0;
> 	int bus;
>-	int retval;
>
> 	if (!nr_virtfn)
> 		return 0;
>@@ -315,10 +314,10 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
> 	if (nr_virtfn < initial)
> 		initial = nr_virtfn;
>
>-	if ((retval = pcibios_sriov_enable(dev, initial))) {
>-		dev_err(&dev->dev, "failure %d from pcibios_sriov_enable()\n",
>-			retval);
>-		return retval;
>+	rc = pcibios_sriov_enable(dev, initial);
>+	if (rc) {
>+		dev_err(&dev->dev, "failure %d from pcibios_sriov_enable()\n", rc);
>+		goto err_pcibios;
> 	}
>
> 	for (i = 0; i < initial; i++) {
>@@ -336,6 +335,8 @@ failed:
> 	while (i--)
> 		virtfn_remove(dev, i, 0);
>
>+	pcibios_sriov_disable(dev);
>+err_pcibios:
> 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
> 	pci_cfg_access_lock(dev);
> 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);

-- 
Richard Yang
Help you, Help me

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259056 — [PATCH v2 3/7] PCI: Remove VFs in reverse order if virtfn_add() fails

From: Alexander Duyck <aduyck@mirantis.com>

If virtfn_add() fails, we call virtfn_remove() for any previously added
devices.  Remove the devices in reverse order (first-added is
last-removed), which is more natural and doesn't require an additional
variable.

[bhelgaas: changelog, split to separate patch for reviewability]
Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/iov.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index ca400a9..c86d94c 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -233,7 +233,7 @@ int __weak pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
 static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 {
 	int rc;
-	int i, j;
+	int i;
 	int nres;
 	u16 initial;
 	struct resource *res;
@@ -328,8 +328,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 	return 0;
 
 failed:
-	for (j = 0; j < i; j++)
-		virtfn_remove(dev, j, 0);
+	while (i--)
+		virtfn_remove(dev, i, 0);
 
 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
 	pci_cfg_access_lock(dev);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259202 — Re: [PATCH v2 3/7] PCI: Remove VFs in reverse order if virtfn_add() fails

On Thu, Oct 29, 2015 at 05:23:08PM -0500, Bjorn Helgaas wrote:
>From: Alexander Duyck <aduyck@mirantis.com>
>
>If virtfn_add() fails, we call virtfn_remove() for any previously added
>devices.  Remove the devices in reverse order (first-added is
>last-removed), which is more natural and doesn't require an additional
>variable.
>
>[bhelgaas: changelog, split to separate patch for reviewability]
>Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
>Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Interesting.

Reviewed-by: Wei Yang <weiyang@linux.vnet.ibm.com>

>---
> drivers/pci/iov.c |    6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>index ca400a9..c86d94c 100644
>--- a/drivers/pci/iov.c
>+++ b/drivers/pci/iov.c
>@@ -233,7 +233,7 @@ int __weak pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
> static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
> {
> 	int rc;
>-	int i, j;
>+	int i;
> 	int nres;
> 	u16 initial;
> 	struct resource *res;
>@@ -328,8 +328,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
> 	return 0;
>
> failed:
>-	for (j = 0; j < i; j++)
>-		virtfn_remove(dev, j, 0);
>+	while (i--)
>+		virtfn_remove(dev, i, 0);
>
> 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
> 	pci_cfg_access_lock(dev);

-- 
Richard Yang
Help you, Help me

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259057 — [PATCH v2 5/7] PCI: Wait 1 second between disabling VFs and clearing NumVFs

From: Alexander Duyck <aduyck@mirantis.com>

Per sec 3.3.3.1 of the SR-IOV spec, r1.1, we must allow 1.0s after clearing
VF Enable before reading any field in the SR-IOV Extended Capability.

Wait 1 second before calling pci_iov_set_numvfs(), which reads
PCI_SRIOV_VF_OFFSET and PCI_SRIOV_VF_STRIDE after it sets PCI_SRIOV_NUM_VF.

[bhelgaas: split to separate patch for reviewability, add spec reference]
Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/iov.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index fada98d..24428d5 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -339,13 +339,13 @@ failed:
 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
 	pci_cfg_access_lock(dev);
 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
-	pci_iov_set_numvfs(dev, 0);
 	ssleep(1);
 	pci_cfg_access_unlock(dev);
 
 	if (iov->link != dev->devfn)
 		sysfs_remove_link(&dev->dev.kobj, "dep_link");
 
+	pci_iov_set_numvfs(dev, 0);
 	return rc;
 }
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259209 — Re: [PATCH v2 5/7] PCI: Wait 1 second between disabling VFs and clearing NumVFs

On Thu, Oct 29, 2015 at 05:23:22PM -0500, Bjorn Helgaas wrote:
>From: Alexander Duyck <aduyck@mirantis.com>
>
>Per sec 3.3.3.1 of the SR-IOV spec, r1.1, we must allow 1.0s after clearing
>VF Enable before reading any field in the SR-IOV Extended Capability.
>
>Wait 1 second before calling pci_iov_set_numvfs(), which reads
>PCI_SRIOV_VF_OFFSET and PCI_SRIOV_VF_STRIDE after it sets PCI_SRIOV_NUM_VF.
>
>[bhelgaas: split to separate patch for reviewability, add spec reference]
>Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
>Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>---
> drivers/pci/iov.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>index fada98d..24428d5 100644
>--- a/drivers/pci/iov.c
>+++ b/drivers/pci/iov.c
>@@ -339,13 +339,13 @@ failed:
> 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
> 	pci_cfg_access_lock(dev);
> 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
>-	pci_iov_set_numvfs(dev, 0);
> 	ssleep(1);
> 	pci_cfg_access_unlock(dev);
>
> 	if (iov->link != dev->devfn)
> 		sysfs_remove_link(&dev->dev.kobj, "dep_link");
>
>+	pci_iov_set_numvfs(dev, 0);

One small question, any specific reason put it here instead of just after
sleep()?

> 	return rc;
> }
>

-- 
Richard Yang
Help you, Help me

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259226 — Re: [PATCH v2 5/7] PCI: Wait 1 second between disabling VFs and clearing NumVFs

Wei,

On 2015/10/30 13:14, Wei Yang wrote:
> On Thu, Oct 29, 2015 at 05:23:22PM -0500, Bjorn Helgaas wrote:
>> From: Alexander Duyck <aduyck@mirantis.com>
>>
>> Per sec 3.3.3.1 of the SR-IOV spec, r1.1, we must allow 1.0s after clearing
>> VF Enable before reading any field in the SR-IOV Extended Capability.
>>
>> Wait 1 second before calling pci_iov_set_numvfs(), which reads
>> PCI_SRIOV_VF_OFFSET and PCI_SRIOV_VF_STRIDE after it sets PCI_SRIOV_NUM_VF.
>>
>> [bhelgaas: split to separate patch for reviewability, add spec reference]
>> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> ---
>> drivers/pci/iov.c |    2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index fada98d..24428d5 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -339,13 +339,13 @@ failed:
>> 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
>> 	pci_cfg_access_lock(dev);
>> 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
>> -	pci_iov_set_numvfs(dev, 0);
>> 	ssleep(1);
>> 	pci_cfg_access_unlock(dev);
>>
>> 	if (iov->link != dev->devfn)
>> 		sysfs_remove_link(&dev->dev.kobj, "dep_link");
>>
>> +	pci_iov_set_numvfs(dev, 0);
> One small question, any specific reason put it here instead of just after
> sleep()?
  Agree,  pci_iov_set_numvfs(dev, 0) should be put before 
pci_cfg_access_unlock(dev) to avoid race,  because "NumVFs may only be 
written while VF Enable is Clear"

  Thanks,
  Ethan
>> 	return rc;
>> }
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259599 — Re: [PATCH v2 5/7] PCI: Wait 1 second between disabling VFs and clearing NumVFs

On 10/29/2015 11:00 PM, ethan zhao wrote:
> Wei,
>
> On 2015/10/30 13:14, Wei Yang wrote:
>> On Thu, Oct 29, 2015 at 05:23:22PM -0500, Bjorn Helgaas wrote:
>>> From: Alexander Duyck <aduyck@mirantis.com>
>>>
>>> Per sec 3.3.3.1 of the SR-IOV spec, r1.1, we must allow 1.0s after 
>>> clearing
>>> VF Enable before reading any field in the SR-IOV Extended Capability.
>>>
>>> Wait 1 second before calling pci_iov_set_numvfs(), which reads
>>> PCI_SRIOV_VF_OFFSET and PCI_SRIOV_VF_STRIDE after it sets 
>>> PCI_SRIOV_NUM_VF.
>>>
>>> [bhelgaas: split to separate patch for reviewability, add spec 
>>> reference]
>>> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>> ---
>>> drivers/pci/iov.c |    2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>>> index fada98d..24428d5 100644
>>> --- a/drivers/pci/iov.c
>>> +++ b/drivers/pci/iov.c
>>> @@ -339,13 +339,13 @@ failed:
>>>     iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
>>>     pci_cfg_access_lock(dev);
>>>     pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
>>> -    pci_iov_set_numvfs(dev, 0);
>>>     ssleep(1);
>>>     pci_cfg_access_unlock(dev);
>>>
>>>     if (iov->link != dev->devfn)
>>>         sysfs_remove_link(&dev->dev.kobj, "dep_link");
>>>
>>> +    pci_iov_set_numvfs(dev, 0);
>> One small question, any specific reason put it here instead of just 
>> after
>> sleep()?
>  Agree,  pci_iov_set_numvfs(dev, 0) should be put before 
> pci_cfg_access_unlock(dev) to avoid race,  because "NumVFs may only be 
> written while VF Enable is Clear"

We are already guaranteeing that aren't we?  I'm assuming there is 
already code in place here somewhere that prevents us from both enabling 
and disabling SR-IOV from more than one thread.  Otherwise how could we 
hope to have any sort of consistent state?

I'm fine with us being more explicit about it if we want to be, but if 
we are going to do it we should probably update all 3 spots where we 
update NumVFs after init instead of just this one.  Perhaps it should be 
a separate patch.

- Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web