On Wed, Dec 19, 2012 at 3:44 PM, Don Dutile <ddutile@xxxxxxxxxx> wrote: > Overall, yet-another-good-clean-up-by-Bjorn ! :) > nits below... > > oh, you can add > Tested-by: Donald Dutile <ddutile@xxxxxxxxxx> > > if you'd to the final commit. I made the -EPERM change suggested by Greg and added this to my pci/for-3.8 branch. I'll ask Linus to pull it soon after v3.8-rc1. Bjorn > > On 12/17/2012 06:24 PM, Bjorn Helgaas wrote: >> >> 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 */ > > maybe worth putting a dev-info print stating num_vfs to <enable,disable> == > current state, > no action taken. ... may point to a user-level programming error. > >> >> /* 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; > > yes, rtn count when non-neg is what I found had to be done > not to hang the user cmd to sysfs. > > >> } >> >> - /* 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; > > ditto; need to rtn input 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
