| Message ID | 20121217232439.GA9746@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Mon, 17 Dec 2012 16:24:39 -0700 Bjorn Helgaas <bhelgaas@google.com> 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@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; > } Maybe return -EPERM instead? > > - 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); Looks good to me. Thanks, - Greg -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Overall, yet-another-good-clean-up-by-Bjorn ! :) nits below... oh, you can add Tested-by: Donald Dutile <ddutile@redhat.com> if you'd to the final commit. 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@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 */ 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 19, 2012 at 3:44 PM, Don Dutile <ddutile@redhat.com> wrote: > Overall, yet-another-good-clean-up-by-Bjorn ! :) > nits below... > > oh, you can add > Tested-by: Donald Dutile <ddutile@redhat.com> > > 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@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 */ > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Bjorn Helgaas [mailto:bhelgaas@google.com] > Sent: Thursday, December 20, 2012 1:48 PM > To: Don Dutile > Cc: Rose, Gregory V; linux-pci@vger.kernel.org; Yuval Mintz; > bhutchings@solarflare.com; yinghai@kernel.org; davem@davemloft.net > Subject: Re: [PATCH v2] PCI SRIOV device enable and disable via sysfs > > On Wed, Dec 19, 2012 at 3:44 PM, Don Dutile <ddutile@redhat.com> wrote: > > Overall, yet-another-good-clean-up-by-Bjorn ! :) nits below... > > > > oh, you can add > > Tested-by: Donald Dutile <ddutile@redhat.com> > > > > 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 Thanks! - Greg -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 20, 2012 at 2:47 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > 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. After a little off-list discussion about the merits of EINVAL, EPERM, EBUSY, etc., I adopted Ben's suggestion of EBUSY for this case: + 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 -EBUSY; where the idea is "the device is already busy providing N VFs, so you can't configure it to serve M VFs" Does that sound agreeable to everybody? Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Bjorn Helgaas [mailto:bhelgaas@google.com] > Sent: Friday, December 21, 2012 11:49 AM > To: Don Dutile > Cc: Rose, Gregory V; linux-pci@vger.kernel.org; Yuval Mintz; > bhutchings@solarflare.com; yinghai@kernel.org; davem@davemloft.net > Subject: Re: [PATCH v2] PCI SRIOV device enable and disable via sysfs > > On Thu, Dec 20, 2012 at 2:47 PM, Bjorn Helgaas <bhelgaas@google.com> > wrote: > > > 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. > > After a little off-list discussion about the merits of EINVAL, EPERM, > EBUSY, etc., I adopted Ben's suggestion of EBUSY for this case: > > + 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 -EBUSY; > > where the idea is "the device is already busy providing N VFs, so you > can't configure it to serve M VFs" > > Does that sound agreeable to everybody? That makes sense, I'm fine with it. - Greg > > Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/21/2012 02:49 PM, Bjorn Helgaas wrote: > On Thu, Dec 20, 2012 at 2:47 PM, Bjorn Helgaas<bhelgaas@google.com> wrote: > >> 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. > > After a little off-list discussion about the merits of EINVAL, EPERM, > EBUSY, etc., I adopted Ben's suggestion of EBUSY for this case: > > + 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 -EBUSY; > > where the idea is "the device is already busy providing N VFs, so you > can't configure it to serve M VFs" > > Does that sound agreeable to everybody? > > Bjorn Ack! -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);