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

[Don Dutile <ddutile@redhat.com>]

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