Patchwork [v2] PCI SRIOV device enable and disable via sysfs

login
register
mail settings
Submitter Bjorn Helgaas
Date Dec. 17, 2012, 11:24 p.m.
Message ID <20121217232439.GA9746@google.com>
Download mbox | patch
Permalink /patch/207008/
State Accepted
Headers show

Comments

Bjorn Helgaas - Dec. 17, 2012, 11:24 p.m.
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>

--
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
Rose, Gregory V - Dec. 17, 2012, 11:38 p.m.
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
Don Dutile - Dec. 19, 2012, 10:44 p.m.
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
Bjorn Helgaas - Dec. 20, 2012, 9:47 p.m.
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
Rose, Gregory V - Dec. 20, 2012, 10:29 p.m.
> -----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
Bjorn Helgaas - Dec. 21, 2012, 7:49 p.m.
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
Rose, Gregory V - Dec. 21, 2012, 7:53 p.m.
> -----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
Don Dutile - Jan. 2, 2013, 5:08 p.m.
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

Patch

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);