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@xxxxxxxxx> > > > >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@xxxxxxxxx> > > 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxx> Link: http://lkml.kernel.org/r/20121214101911.00002f59@unknown Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> 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); -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
