Re: [PATCH v2] PCI SRIOV device enable and disable via sysfs

[Bjorn Helgaas <bhelgaas@google.com>]

On Mon, Dec 17, 2012 at 02:59:15PM -0500, Don Dutile wrote:
> On 12/14/2012 01:19 PM, Greg Rose wrote:
> >pci: Fix return code
> >
> >From: Greg Rose<gregory.v.rose@intel.com>
> >
> >The return code from the sriov configure function was only returned if it
> >was less than zero indicating an error.  This caused the code to fall
> >through to the default return of an error code even though the sriov
> >configure function has returned the number of VFs it created - a positive
> >number indicating success.
> >
> >
> >Signed-off-by: Greg Rose<gregory.v.rose@intel.com>
> 
> Actually, it returned the number of VFs enabled if it exactly equalled
> the number to be enabled.  Otherwise, the basic testing would have failed.
> If the number of vf's enabled was positive but not the same
> as the number requested-to-be-enabled, then it incorrectly returned.
> 
> But, the patch corrects the base problem, so
> Acked-by: Donald Dutile <ddutile@redhat.com>

Alternate proposal below.  The patch is ugly; it might be easier to compare
the before (http://pastebin.com/zneG8AuD) and after
(http://pastebin.com/BEXEE8kc) versions.

commit fae71e5dc9064dcb3077b8da6e0ed9d800c1f527
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Thu Dec 13 20:22:44 2012 -0700

    PCI: Cleanup sriov_numvfs_show() and handle common case without error
    
    If we request "num_vfs" and the driver's sriov_configure() method enables
    exactly that number ("num_vfs_enabled"), we complain "Invalid value for
    number of VFs to enable" and return an error.  We should silently return
    success instead.
    
    Also, use kstrtou16() since numVFs is defined to be a 16-bit field and
    rework to simplify control flow.
    
    Reported-by: Greg Rose <gregory.v.rose@intel.com>
    Link: http://lkml.kernel.org/r/20121214101911.00002f59@unknown
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 05b78b1..5e8af12 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -422,77 +422,60 @@ static ssize_t sriov_numvfs_show(struct device *dev,
 }
 
 /*
- * num_vfs > 0; number of vfs to enable
- * num_vfs = 0; disable all vfs
+ * num_vfs > 0; number of VFs to enable
+ * num_vfs = 0; disable all VFs
  *
  * Note: SRIOV spec doesn't allow partial VF
- *       disable, so its all or none.
+ *       disable, so it's all or none.
  */
 static ssize_t sriov_numvfs_store(struct device *dev,
 				  struct device_attribute *attr,
 				  const char *buf, size_t count)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	int num_vfs_enabled = 0;
-	int num_vfs;
-	int ret = 0;
-	u16 total;
+	int ret;
+	u16 num_vfs;
 
-	if (kstrtoint(buf, 0, &num_vfs) < 0)
-		return -EINVAL;
+	ret = kstrtou16(buf, 0, &num_vfs);
+	if (ret < 0)
+		return ret;
+
+	if (num_vfs > pci_sriov_get_totalvfs(pdev))
+		return -ERANGE;
+
+	if (num_vfs == pdev->sriov->num_VFs)
+		return count;		/* no change */
 
 	/* is PF driver loaded w/callback */
 	if (!pdev->driver || !pdev->driver->sriov_configure) {
-		dev_info(&pdev->dev,
-			 "Driver doesn't support SRIOV configuration via sysfs\n");
+		dev_info(&pdev->dev, "Driver doesn't support SRIOV configuration via sysfs\n");
 		return -ENOSYS;
 	}
 
-	/* if enabling vf's ... */
-	total = pci_sriov_get_totalvfs(pdev);
-	/* Requested VFs to enable < totalvfs and none enabled already */
-	if ((num_vfs > 0) && (num_vfs <= total)) {
-		if (pdev->sriov->num_VFs == 0) {
-			num_vfs_enabled =
-				pdev->driver->sriov_configure(pdev, num_vfs);
-			if ((num_vfs_enabled >= 0) &&
-			    (num_vfs_enabled != num_vfs)) {
-				dev_warn(&pdev->dev,
-					 "Only %d VFs enabled\n",
-					 num_vfs_enabled);
-				return count;
-			} else if (num_vfs_enabled < 0)
-				/* error code from driver callback */
-				return num_vfs_enabled;
-		} else if (num_vfs == pdev->sriov->num_VFs) {
-			dev_warn(&pdev->dev,
-				 "%d VFs already enabled; no enable action taken\n",
-				 num_vfs);
-			return count;
-		} else {
-			dev_warn(&pdev->dev,
-				 "%d VFs already enabled. Disable before enabling %d VFs\n",
-				 pdev->sriov->num_VFs, num_vfs);
-			return -EINVAL;
-		}
+	if (num_vfs == 0) {
+		/* disable VFs */
+		ret = pdev->driver->sriov_configure(pdev, 0);
+		if (ret < 0)
+			return ret;
+		return count;
 	}
 
-	/* disable vfs */
-	if (num_vfs == 0) {
-		if (pdev->sriov->num_VFs != 0) {
-			ret = pdev->driver->sriov_configure(pdev, 0);
-			return ret ? ret : count;
-		} else {
-			dev_warn(&pdev->dev,
-				 "All VFs disabled; no disable action taken\n");
-			return count;
-		}
+	/* enable VFs */
+	if (pdev->sriov->num_VFs) {
+		dev_warn(&pdev->dev, "%d VFs already enabled. Disable before enabling %d VFs\n",
+			 pdev->sriov->num_VFs, num_vfs);
+		return -EINVAL;
 	}
 
-	dev_err(&pdev->dev,
-		"Invalid value for number of VFs to enable: %d\n", num_vfs);
+	ret = pdev->driver->sriov_configure(pdev, num_vfs);
+	if (ret < 0)
+		return ret;
 
-	return -EINVAL;
+	if (ret != num_vfs)
+		dev_warn(&pdev->dev, "%d VFs requested; only %d enabled\n",
+			 num_vfs, ret);
+
+	return count;
 }
 
 static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);