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

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

[Donald Dutile]

Currently, VF enablement by SRIOV-capable PCIe devices is done
in driver-specific module parameters.  If not setup in modprobe files,
it requires admin to unload & reload PF drivers with number of desired
VFs to enable.  Additionally, the enablement is system wide: all
devices controlled by the same driver have the same number of VFs
enabled.  Although the latter is probably desired, there are PCI
configurations setup by system BIOS that may not enable that to occur.

Two files are created if a PCIe device has SRIOV support:
sriov_totalvfs -- cat-ing this file returns the maximum number
                  of VFs a PCIe device supports.
sriov_numvfs -- echo'ing a positive number to this file enables 
                & configures this number of VFs for this given PCIe
                device.
	     -- echo'ing 0 to this file disables and deconfigures
                all VFs for this given PCIe device.
             -- cat-ing this file will return the number of VFs
                currently enabled on this PCIe device.

VF enable and disablement is invoked much like other PCIe
configuration functions -- via a registered callback in the driver,
i.e., probe, release, etc.  In this case, sriov_configure

PATCH v1->v2:
-- incorporate more feedback from Ben Hutchings.
-- (hopefully) correct From & Signed-by for Yinghai Lu's patches (1/8 & 2/8)

RFC V3->PATCH:
-- incorporate feedback from Ben Hutchings.
-- clean up poor RFC patches & sanitize through checkpatch.pl

RFC v2->v3:
-- change the file names to reflect the names used in the SRIOV spec
-- change to a single file for enable & disable; 
   change driver interface to a single interface.
-- add more informative messages on failures
-- add a core method that a driver can invoke to modify
   the totalvfs reported & supported by a driver.
-- a set of patches for ixgbe provided by Greg Rose to use the
   new interfaces; the last patch modified from the original
   two file, enable/disable interface to the current single file
   enable/disable. Greg will eventually post the final version
   of these patches via Intel's usual process for driver patches.
   Provided here as an example, and enable other SRIOV drivers
   to see how adoption of the interface can be added.

RFC v1->v2:
This patch is based on previous 2 patches by Yinghai Lu
that cleaned up the vga attributes for PCI devices under sysfs,
and uses visibility-checking group attributes as recommended by
Greg K-H.

Signed-off-by: Donald Dutile <ddutile@redhat.com>
---
 drivers/pci/iov.c       |  48 ++++++++++++++++
 drivers/pci/pci-sysfs.c | 179 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 drivers/pci/pci.h       |   2 +
 drivers/pci/probe.c     |   1 +
 include/linux/pci.h     |  11 ++++
 5 files changed, 230 insertions(+), 11 deletions(-)

[PATCH 1/4] PCI: add pci_device_type to pdev's device struct

[Donald Dutile]

From: Yinghai Lu <yinghai@kernel.org>

Need type filled in device structure so it
can be used for visible attribute control in
syfsfs for pci_dev.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/pci-sysfs.c | 24 ++++++++++++++++++++++++
 drivers/pci/pci.h       |  1 +
 drivers/pci/probe.c     |  1 +
 3 files changed, 26 insertions(+)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 02d107b..3d160aa 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1411,3 +1411,27 @@ static int __init pci_sysfs_init(void)
 }
 
 late_initcall(pci_sysfs_init);
+
+static struct attribute *pci_dev_dev_attrs[] = {
+	NULL,
+};
+
+static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
+						struct attribute *a, int n)
+{
+	return a->mode;
+}
+
+static struct attribute_group pci_dev_attr_group = {
+	.attrs = pci_dev_dev_attrs,
+	.is_visible = pci_dev_attrs_are_visible,
+};
+
+static const struct attribute_group *pci_dev_attr_groups[] = {
+	&pci_dev_attr_group,
+	NULL,
+};
+
+struct device_type pci_dev_type = {
+	.groups = pci_dev_attr_groups,
+};
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index bacbcba..6f6cd14 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -157,6 +157,7 @@ static inline int pci_no_d1d2(struct pci_dev *dev)
 }
 extern struct device_attribute pci_dev_attrs[];
 extern struct device_attribute pcibus_dev_attrs[];
+extern struct device_type pci_dev_type;
 #ifdef CONFIG_HOTPLUG
 extern struct bus_attribute pci_bus_attrs[];
 #else
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ec909af..0312f1c48 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -975,6 +975,7 @@ int pci_setup_device(struct pci_dev *dev)
 	dev->sysdata = dev->bus->sysdata;
 	dev->dev.parent = dev->bus->bridge;
 	dev->dev.bus = &pci_bus_type;
+	dev->dev.type = &pci_dev_type;
 	dev->hdr_type = hdr_type & 0x7f;
 	dev->multifunction = !!(hdr_type & 0x80);
 	dev->error_state = pci_channel_io_normal;
-- 
1.7.10.2.552.gaa3bb87

[PATCH 2/4] PCI,sys: Use is_visible() with boot_vga attribute for pci_dev

[Donald Dutile]

From: Yinghai Lu <yinghai@kernel.org>

Should make pci_creae_sysfs_dev_files simpler.
Also fix possible memleak in remove path.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/pci-sysfs.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 3d160aa..fbbb97f 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1303,29 +1303,20 @@ int __must_check pci_create_sysfs_dev_files (struct pci_dev *pdev)
 		pdev->rom_attr = attr;
 	}
 
-	if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) {
-		retval = device_create_file(&pdev->dev, &vga_attr);
-		if (retval)
-			goto err_rom_file;
-	}
-
 	/* add platform-specific attributes */
 	retval = pcibios_add_platform_entries(pdev);
 	if (retval)
-		goto err_vga_file;
+		goto err_rom_file;
 
 	/* add sysfs entries for various capabilities */
 	retval = pci_create_capabilities_sysfs(pdev);
 	if (retval)
-		goto err_vga_file;
+		goto err_rom_file;
 
 	pci_create_firmware_label_files(pdev);
 
 	return 0;
 
-err_vga_file:
-	if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
-		device_remove_file(&pdev->dev, &vga_attr);
 err_rom_file:
 	if (rom_size) {
 		sysfs_remove_bin_file(&pdev->dev.kobj, pdev->rom_attr);
@@ -1413,12 +1404,20 @@ static int __init pci_sysfs_init(void)
 late_initcall(pci_sysfs_init);
 
 static struct attribute *pci_dev_dev_attrs[] = {
+	&vga_attr.attr,
 	NULL,
 };
 
 static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
 						struct attribute *a, int n)
 {
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (a == &vga_attr.attr)
+		if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
+			return 0;
+
 	return a->mode;
 }
 
-- 
1.7.10.2.552.gaa3bb87

[PATCH 3/4] PCI,sys: SRIOV control and status via sysfs

[Donald Dutile]

Provide files under sysfs to determine the max number of vfs
an SRIOV-capable PCIe device supports, and methods to enable and
disable the vfs on a per device basis.

Currently, VF enablement by SRIOV-capable PCIe devices is done
in driver-specific module parameters.  If not setup in modprobe files,
it requires admin to unload & reload PF drivers with number of desired
VFs to enable.  Additionally, the enablement is system wide: all
devices controlled by the same driver have the same number of VFs
enabled.  Although the latter is probably desired, there are PCI
configurations setup by system BIOS that may not enable that to occur.

Three files are created if a PCIe device has SRIOV support:
sriov_totalvfs -- cat-ing this file returns the maximum number
                  of VFs a PCIe device supports as reported by
                  the TotalVFs in the SRIOV ext cap structure.
sriov_numvfs -- echo'ing a positive number to this file enables this
                number of VFs for this given PCIe device.
             -- echo'ing a 0 to this file disables
                any previously enabled VFs for this PCIe device.
             -- cat-ing this file will return the number of VFs
                currently enabled on this PCIe device, i.e.,
                the NumVFs field in SRIOV ext. cap structure.

VF enable and disablement is invoked much like other PCIe
configuration functions -- via registered callbacks in the driver,
i.e., probe, release, etc.

Signed-off-by: Donald Dutile <ddutile@redhat.com>
---
 drivers/pci/pci-sysfs.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h     |   1 +
 2 files changed, 135 insertions(+)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index fbbb97f..cbcdd8d 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -404,6 +404,113 @@ static ssize_t d3cold_allowed_show(struct device *dev,
 }
 #endif
 
+#ifdef CONFIG_PCI_IOV
+static ssize_t sriov_totalvfs_show(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct pci_dev *pdev;
+	u16 total;
+
+	pdev = to_pci_dev(dev);
+	total = pdev->sriov->total;
+	return sprintf(buf, "%u\n", total);
+}
+
+
+static ssize_t sriov_numvfs_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct pci_dev *pdev;
+
+	pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pdev->sriov->nr_virtfn);
+}
+
+/*
+ * 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.
+ */
+static ssize_t sriov_numvfs_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct pci_dev *pdev;
+	int num_vfs_enabled = 0;
+	int num_vfs;
+	int ret = 0;
+	u16 total;
+
+	pdev = to_pci_dev(dev);
+
+	if (kstrtoint(buf, 0, &num_vfs) < 0)
+		return -EINVAL;
+
+	/* 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");
+		return -ENOSYS;
+	}
+
+	/* if enabling vf's ... */
+	total = pdev->sriov->total;
+	/* Requested VFs to enable < totalvfs and none enabled already */
+	if ((num_vfs > 0) && (num_vfs <= total)) {
+		if (pdev->sriov->nr_virtfn == 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->nr_virtfn) {
+			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->nr_virtfn, num_vfs);
+			return -EINVAL;
+		}
+	}
+
+	/* disable vfs */
+	if (num_vfs == 0) {
+		if (pdev->sriov->nr_virtfn != 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;
+		}
+	}
+
+	dev_err(&pdev->dev,
+		"Invalid value for number of VFs to enable: %d\n", num_vfs);
+
+	return -EINVAL;
+}
+
+static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
+static struct device_attribute sriov_numvfs_attr =
+		__ATTR(sriov_numvfs, (S_IRUGO|S_IWUSR|S_IWGRP),
+		       sriov_numvfs_show, sriov_numvfs_store);
+#endif /* CONFIG_PCI_IOV */
+
 struct device_attribute pci_dev_attrs[] = {
 	__ATTR_RO(resource),
 	__ATTR_RO(vendor),
@@ -1421,6 +1528,30 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
 	return a->mode;
 }
 
+#ifdef CONFIG_PCI_IOV
+static struct attribute *sriov_dev_attrs[] = {
+	&sriov_totalvfs_attr.attr,
+	&sriov_numvfs_attr.attr,
+	NULL,
+};
+
+static umode_t sriov_attrs_are_visible(struct kobject *kobj,
+					 struct attribute *a, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+
+	if (!dev_is_pf(dev))
+		return 0;
+
+	return a->mode;
+}
+
+static struct attribute_group sriov_dev_attr_group = {
+	.attrs = sriov_dev_attrs,
+	.is_visible = sriov_attrs_are_visible,
+};
+#endif /* CONFIG_PCI_IOV */
+
 static struct attribute_group pci_dev_attr_group = {
 	.attrs = pci_dev_dev_attrs,
 	.is_visible = pci_dev_attrs_are_visible,
@@ -1428,6 +1559,9 @@ static struct attribute_group pci_dev_attr_group = {
 
 static const struct attribute_group *pci_dev_attr_groups[] = {
 	&pci_dev_attr_group,
+#ifdef CONFIG_PCI_IOV
+	&sriov_dev_attr_group,
+#endif
 	NULL,
 };
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index ee21795..7ef8fba 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -573,6 +573,7 @@ struct pci_driver {
 	int  (*resume_early) (struct pci_dev *dev);
 	int  (*resume) (struct pci_dev *dev);	                /* Device woken up */
 	void (*shutdown) (struct pci_dev *dev);
+	int (*sriov_configure) (struct pci_dev *dev, int num_vfs); /* PF pdev */
 	const struct pci_error_handlers *err_handler;
 	struct device_driver	driver;
 	struct pci_dynids dynids;
-- 
1.7.10.2.552.gaa3bb87

Re: [PATCH 3/4] PCI,sys: SRIOV control and status via sysfs

[Yinghai Lu]

On Mon, Nov 5, 2012 at 12:20 PM, Donald Dutile <ddutile@redhat.com> wrote:
> Provide files under sysfs to determine the max number of vfs
> an SRIOV-capable PCIe device supports, and methods to enable and
> disable the vfs on a per device basis.
>
> Currently, VF enablement by SRIOV-capable PCIe devices is done
> in driver-specific module parameters.  If not setup in modprobe files,
> it requires admin to unload & reload PF drivers with number of desired
> VFs to enable.  Additionally, the enablement is system wide: all
> devices controlled by the same driver have the same number of VFs
> enabled.  Although the latter is probably desired, there are PCI
> configurations setup by system BIOS that may not enable that to occur.
>
> Three files are created if a PCIe device has SRIOV support:
> sriov_totalvfs -- cat-ing this file returns the maximum number
>                   of VFs a PCIe device supports as reported by
>                   the TotalVFs in the SRIOV ext cap structure.
> sriov_numvfs -- echo'ing a positive number to this file enables this
>                 number of VFs for this given PCIe device.
>              -- echo'ing a 0 to this file disables
>                 any previously enabled VFs for this PCIe device.
>              -- cat-ing this file will return the number of VFs
>                 currently enabled on this PCIe device, i.e.,
>                 the NumVFs field in SRIOV ext. cap structure.
>
> VF enable and disablement is invoked much like other PCIe
> configuration functions -- via registered callbacks in the driver,
> i.e., probe, release, etc.
>
> Signed-off-by: Donald Dutile <ddutile@redhat.com>
> ---
>  drivers/pci/pci-sysfs.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h     |   1 +
>  2 files changed, 135 insertions(+)
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index fbbb97f..cbcdd8d 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -404,6 +404,113 @@ static ssize_t d3cold_allowed_show(struct device *dev,
>  }
>  #endif
>
> +#ifdef CONFIG_PCI_IOV
> +static ssize_t sriov_totalvfs_show(struct device *dev,
> +                                  struct device_attribute *attr,
> +                                  char *buf)
> +{
> +       struct pci_dev *pdev;
> +       u16 total;
> +
> +       pdev = to_pci_dev(dev);
> +       total = pdev->sriov->total;
> +       return sprintf(buf, "%u\n", total);
> +}
> +
> +
> +static ssize_t sriov_numvfs_show(struct device *dev,
> +                                struct device_attribute *attr,
> +                                char *buf)
> +{
> +       struct pci_dev *pdev;
> +
> +       pdev = to_pci_dev(dev);
> +
> +       return sprintf(buf, "%u\n", pdev->sriov->nr_virtfn);
> +}
> +
> +/*
> + * 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.
> + */
> +static ssize_t sriov_numvfs_store(struct device *dev,
> +                                 struct device_attribute *attr,
> +                                 const char *buf, size_t count)
> +{
> +       struct pci_dev *pdev;
> +       int num_vfs_enabled = 0;
> +       int num_vfs;
> +       int ret = 0;
> +       u16 total;
> +
> +       pdev = to_pci_dev(dev);
> +
> +       if (kstrtoint(buf, 0, &num_vfs) < 0)
> +               return -EINVAL;
> +
> +       /* 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");
> +               return -ENOSYS;
> +       }
> +
> +       /* if enabling vf's ... */
> +       total = pdev->sriov->total;
> +       /* Requested VFs to enable < totalvfs and none enabled already */
> +       if ((num_vfs > 0) && (num_vfs <= total)) {
> +               if (pdev->sriov->nr_virtfn == 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->nr_virtfn) {
> +                       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->nr_virtfn, num_vfs);
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       /* disable vfs */
> +       if (num_vfs == 0) {
> +               if (pdev->sriov->nr_virtfn != 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;
> +               }
> +       }
> +
> +       dev_err(&pdev->dev,
> +               "Invalid value for number of VFs to enable: %d\n", num_vfs);
> +
> +       return -EINVAL;
> +}
> +
> +static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
> +static struct device_attribute sriov_numvfs_attr =
> +               __ATTR(sriov_numvfs, (S_IRUGO|S_IWUSR|S_IWGRP),
> +                      sriov_numvfs_show, sriov_numvfs_store);
> +#endif /* CONFIG_PCI_IOV */
> +
>  struct device_attribute pci_dev_attrs[] = {
>         __ATTR_RO(resource),
>         __ATTR_RO(vendor),
> @@ -1421,6 +1528,30 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
>         return a->mode;
>  }
>
> +#ifdef CONFIG_PCI_IOV
> +static struct attribute *sriov_dev_attrs[] = {
> +       &sriov_totalvfs_attr.attr,
> +       &sriov_numvfs_attr.attr,
> +       NULL,
> +};
> +
> +static umode_t sriov_attrs_are_visible(struct kobject *kobj,
> +                                        struct attribute *a, int n)
> +{
> +       struct device *dev = container_of(kobj, struct device, kobj);
> +
> +       if (!dev_is_pf(dev))
> +               return 0;
> +
> +       return a->mode;
> +}
> +
> +static struct attribute_group sriov_dev_attr_group = {
> +       .attrs = sriov_dev_attrs,
> +       .is_visible = sriov_attrs_are_visible,
> +};
> +#endif /* CONFIG_PCI_IOV */
> +
>  static struct attribute_group pci_dev_attr_group = {
>         .attrs = pci_dev_dev_attrs,
>         .is_visible = pci_dev_attrs_are_visible,
> @@ -1428,6 +1559,9 @@ static struct attribute_group pci_dev_attr_group = {
>
>  static const struct attribute_group *pci_dev_attr_groups[] = {
>         &pci_dev_attr_group,
> +#ifdef CONFIG_PCI_IOV
> +       &sriov_dev_attr_group,
> +#endif

should move sriov_dev_attr_group related code to
drivers/pci/iov.c
or driver/pci/iov_sysfs.c

and kill those CONFIG_IOV.

Re: [PATCH 3/4] PCI,sys: SRIOV control and status via sysfs

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 592 bytes --]

On Fri, Nov 9, 2012 at 10:47 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Mon, Nov 5, 2012 at 12:20 PM, Donald Dutile <ddutile@redhat.com> wrote:
>>
>>  static const struct attribute_group *pci_dev_attr_groups[] = {
>>         &pci_dev_attr_group,
>> +#ifdef CONFIG_PCI_IOV
>> +       &sriov_dev_attr_group,
>> +#endif
>
> should move sriov_dev_attr_group related code to
> drivers/pci/iov.c
> or driver/pci/iov_sysfs.c
>
> and kill those CONFIG_IOV.

Bjorn,

please check attached patch that separate sriov_dev_attr_group out.
it is against to your pci/don-sriov branch.

Thanks

Yinghai

[-- Attachment #2: iov_sysfs.patch --]
[-- Type: application/octet-stream, Size: 9058 bytes --]

Subject: [PATCH] PCI, IOV: Move sriov dev attr to iov_sysfs.c

to avoid CONFIG_IOV in pci-sysfs.c and make that pci-sysfs.c smaller.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/Makefile    |    2 
 drivers/pci/iov_sysfs.c |  123 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci-sysfs.c |  128 ------------------------------------------------
 drivers/pci/pci.h       |    3 +
 4 files changed, 128 insertions(+), 128 deletions(-)

Index: linux-2.6/drivers/pci/iov_sysfs.c
===================================================================
--- /dev/null
+++ linux-2.6/drivers/pci/iov_sysfs.c
@@ -0,0 +1,123 @@
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/fs.h>
+#include "pci.h"
+
+static ssize_t sriov_totalvfs_show(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pci_sriov_get_totalvfs(pdev));
+}
+
+static ssize_t sriov_numvfs_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pdev->sriov->num_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.
+ */
+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;
+
+	if (kstrtoint(buf, 0, &num_vfs) < 0)
+		return -EINVAL;
+
+	/* 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");
+		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;
+		}
+	}
+
+	/* 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;
+		}
+	}
+
+	dev_err(&pdev->dev,
+		"Invalid value for number of VFs to enable: %d\n", num_vfs);
+
+	return -EINVAL;
+}
+
+static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
+static struct device_attribute sriov_numvfs_attr =
+		__ATTR(sriov_numvfs, (S_IRUGO|S_IWUSR|S_IWGRP),
+		       sriov_numvfs_show, sriov_numvfs_store);
+
+static struct attribute *sriov_dev_attrs[] = {
+	&sriov_totalvfs_attr.attr,
+	&sriov_numvfs_attr.attr,
+	NULL,
+};
+
+static umode_t sriov_attrs_are_visible(struct kobject *kobj,
+					 struct attribute *a, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+
+	if (!dev_is_pf(dev))
+		return 0;
+
+	return a->mode;
+}
+
+struct attribute_group sriov_dev_attr_group = {
+	.attrs = sriov_dev_attrs,
+	.is_visible = sriov_attrs_are_visible,
+};
Index: linux-2.6/drivers/pci/pci-sysfs.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci-sysfs.c
+++ linux-2.6/drivers/pci/pci-sysfs.c
@@ -404,106 +404,6 @@ static ssize_t d3cold_allowed_show(struc
 }
 #endif
 
-#ifdef CONFIG_PCI_IOV
-static ssize_t sriov_totalvfs_show(struct device *dev,
-				   struct device_attribute *attr,
-				   char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%u\n", pci_sriov_get_totalvfs(pdev));
-}
-
-
-static ssize_t sriov_numvfs_show(struct device *dev,
-				 struct device_attribute *attr,
-				 char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%u\n", pdev->sriov->num_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.
- */
-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;
-
-	if (kstrtoint(buf, 0, &num_vfs) < 0)
-		return -EINVAL;
-
-	/* 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");
-		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;
-		}
-	}
-
-	/* 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;
-		}
-	}
-
-	dev_err(&pdev->dev,
-		"Invalid value for number of VFs to enable: %d\n", num_vfs);
-
-	return -EINVAL;
-}
-
-static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
-static struct device_attribute sriov_numvfs_attr =
-		__ATTR(sriov_numvfs, (S_IRUGO|S_IWUSR|S_IWGRP),
-		       sriov_numvfs_show, sriov_numvfs_store);
-#endif /* CONFIG_PCI_IOV */
-
 struct device_attribute pci_dev_attrs[] = {
 	__ATTR_RO(resource),
 	__ATTR_RO(vendor),
@@ -1487,30 +1387,6 @@ static umode_t pci_dev_attrs_are_visible
 	return a->mode;
 }
 
-#ifdef CONFIG_PCI_IOV
-static struct attribute *sriov_dev_attrs[] = {
-	&sriov_totalvfs_attr.attr,
-	&sriov_numvfs_attr.attr,
-	NULL,
-};
-
-static umode_t sriov_attrs_are_visible(struct kobject *kobj,
-					 struct attribute *a, int n)
-{
-	struct device *dev = container_of(kobj, struct device, kobj);
-
-	if (!dev_is_pf(dev))
-		return 0;
-
-	return a->mode;
-}
-
-static struct attribute_group sriov_dev_attr_group = {
-	.attrs = sriov_dev_attrs,
-	.is_visible = sriov_attrs_are_visible,
-};
-#endif /* CONFIG_PCI_IOV */
-
 static struct attribute_group pci_dev_attr_group = {
 	.attrs = pci_dev_dev_attrs,
 	.is_visible = pci_dev_attrs_are_visible,
@@ -1518,9 +1394,7 @@ static struct attribute_group pci_dev_at
 
 static const struct attribute_group *pci_dev_attr_groups[] = {
 	&pci_dev_attr_group,
-#ifdef CONFIG_PCI_IOV
-	&sriov_dev_attr_group,
-#endif
+	SRIOV_DEV_ATTR_GROUP,
 	NULL,
 };
 
Index: linux-2.6/drivers/pci/pci.h
===================================================================
--- linux-2.6.orig/drivers/pci/pci.h
+++ linux-2.6/drivers/pci/pci.h
@@ -267,6 +267,8 @@ extern resource_size_t pci_sriov_resourc
 						    int resno);
 extern void pci_restore_iov_state(struct pci_dev *dev);
 extern int pci_iov_bus_range(struct pci_bus *bus);
+extern struct attribute_group sriov_dev_attr_group;
+#define SRIOV_DEV_ATTR_GROUP	(&sriov_dev_attr_group)
 
 #else
 static inline int pci_iov_init(struct pci_dev *dev)
@@ -289,6 +291,7 @@ static inline int pci_iov_bus_range(stru
 {
 	return 0;
 }
+#define SRIOV_DEV_ATTR_GROUP	(NULL)
 
 #endif /* CONFIG_PCI_IOV */
 
Index: linux-2.6/drivers/pci/Makefile
===================================================================
--- linux-2.6.orig/drivers/pci/Makefile
+++ linux-2.6/drivers/pci/Makefile
@@ -30,7 +30,7 @@ obj-$(CONFIG_PCI_MSI) += msi.o
 obj-$(CONFIG_HT_IRQ) += htirq.o
 
 obj-$(CONFIG_PCI_ATS) += ats.o
-obj-$(CONFIG_PCI_IOV) += iov.o
+obj-$(CONFIG_PCI_IOV) += iov.o iov_sysfs.o
 
 #
 # Some architectures use the generic PCI setup functions

Re: [PATCH 3/4] PCI,sys: SRIOV control and status via sysfs

[Bjorn Helgaas]

On Sat, Nov 10, 2012 at 12:31 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Nov 9, 2012 at 10:47 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Mon, Nov 5, 2012 at 12:20 PM, Donald Dutile <ddutile@redhat.com> wrote:
>>>
>>>  static const struct attribute_group *pci_dev_attr_groups[] = {
>>>         &pci_dev_attr_group,
>>> +#ifdef CONFIG_PCI_IOV
>>> +       &sriov_dev_attr_group,
>>> +#endif
>>
>> should move sriov_dev_attr_group related code to
>> drivers/pci/iov.c
>> or driver/pci/iov_sysfs.c
>>
>> and kill those CONFIG_IOV.
>
> Bjorn,
>
> please check attached patch that separate sriov_dev_attr_group out.
> it is against to your pci/don-sriov branch.

The convention is to include patches inline, which makes them easier
to review.  I know gmail makes that hard.  I use mutt or stacked git
to do it.

I'm not opposed to moving the SR-IOV sysfs stuff out of pci-sysfs.c.
Since these patches haven't been merged yet, you should work it out
with Don first, so we can merge it in the right place to begin with,
rather than merging them and then immediately moving them.

Personally, I would put it all in pci/iov.c rather than creating a new
file just for this.  That would also avoid the question of whether it
should be "iov_sysfs.c" or "iov-sysfs.c".  All the other files in
drivers/pci use a hyphen.  All the files in subdirectories of
drivers/pci use an underscore.  A minor but annoying difference.

Bjorn

Re: [PATCH 3/4] PCI,sys: SRIOV control and status via sysfs

[Yinghai Lu]

On Sat, Nov 10, 2012 at 1:16 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Sat, Nov 10, 2012 at 12:31 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Fri, Nov 9, 2012 at 10:47 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> On Mon, Nov 5, 2012 at 12:20 PM, Donald Dutile <ddutile@redhat.com> wrote:
>>>>
>>>>  static const struct attribute_group *pci_dev_attr_groups[] = {
>>>>         &pci_dev_attr_group,
>>>> +#ifdef CONFIG_PCI_IOV
>>>> +       &sriov_dev_attr_group,
>>>> +#endif
>>>
>>> should move sriov_dev_attr_group related code to
>>> drivers/pci/iov.c
>>> or driver/pci/iov_sysfs.c
>>>
>>> and kill those CONFIG_IOV.
>>
>> Bjorn,
>>
>> please check attached patch that separate sriov_dev_attr_group out.
>> it is against to your pci/don-sriov branch.
>
> The convention is to include patches inline, which makes them easier
> to review.  I know gmail makes that hard.  I use mutt or stacked git
> to do it.

yes, gmail web client should support plain attachment.

so you are using mutt with smtp enabled ?

>
> I'm not opposed to moving the SR-IOV sysfs stuff out of pci-sysfs.c.
> Since these patches haven't been merged yet, you should work it out
> with Don first, so we can merge it in the right place to begin with,
> rather than merging them and then immediately moving them.

sure. it should be out of pci-sysfs.c at first place.

I thought that you are going to merge your pci/don-sriov.

BTW, when you redo pci/don-sriov, please add acked-by from GregKH for
first two patches.

>
> Personally, I would put it all in pci/iov.c rather than creating a new
> file just for this.  That would also avoid the question of whether it
> should be "iov_sysfs.c" or "iov-sysfs.c".  All the other files in
> drivers/pci use a hyphen.  All the files in subdirectories of
> drivers/pci use an underscore.  A minor but annoying difference.

yeah, looks like should be iov-sysfs.c to keep them consistent.

iov.c is already about 800 lines. better not to stuff more code into it anymore.

Yinghai

Re: [PATCH 3/4] PCI,sys: SRIOV control and status via sysfs

[Don Dutile]

On 11/10/2012 02:31 AM, Yinghai Lu wrote:
> On Fri, Nov 9, 2012 at 10:47 PM, Yinghai Lu<yinghai@kernel.org>  wrote:
>> On Mon, Nov 5, 2012 at 12:20 PM, Donald Dutile<ddutile@redhat.com>  wrote:
>>>
>>>   static const struct attribute_group *pci_dev_attr_groups[] = {
>>>          &pci_dev_attr_group,
>>> +#ifdef CONFIG_PCI_IOV
>>> +&sriov_dev_attr_group,
>>> +#endif
>>
>> should move sriov_dev_attr_group related code to
>> drivers/pci/iov.c
>> or driver/pci/iov_sysfs.c
>>
>> and kill those CONFIG_IOV.
>
> Bjorn,
>
> please check attached patch that separate sriov_dev_attr_group out.
> it is against to your pci/don-sriov branch.
>
> Thanks
>
> Yinghai

Since the sriov-related files are created at the same time as
the other <DOMAIN:B:D.F> files, I would prefer to keep their
generation in the pci-sysfs.c file.
If it it moved, then put it into iov.c; the 'virtfn[X]' sysfs
files are created/destroyed in that file already, so if the
majority want it out of pci-sysfs, then I would favor a move to iov.c.
iov-sysfs.c creates yet-another file to search for SRIOV-related code,
and it's fairly centric to iov.c now.

If we're going to toss stuff out of pci-sysfs, then toss out the vga-attributes
crap, as well as other non-pci-specific files like bus-affinity(numa -- which
should go into PCI-root-bus related core code, e.g., acpi for x86; idk in ppc, arm, s390, etc.).
I'm not saying that following bad decisions of the past
is a justification for keeping the sriov related sysfs in pci-sysfs;
I am saying sriov *is* PCI-specific/architected, so it seems like
it should reside in pci-sysfs.c .

Other opinions/votes/preferences ???

- Don

[PATCH 4/4] PCI,sriov: provide method to reduce the number of total VFs supported

[Donald Dutile]

Some implementations of SRIOV provide a capability structure
value of TotalVFs that is greater than what the software can support.
Provide a method to reduce the capability structure reported value
to the value the driver can support.
This ensures sysfs reports the current capability of the system,
hardware and software.
Example for its use: igb & ixgbe -- report 8 & 64 as TotalVFs,
but drivers only support 7 & 63 maximum.

Signed-off-by: Donald Dutile <ddutile@redhat.com>
---
 drivers/pci/iov.c       | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci-sysfs.c |  4 ++--
 drivers/pci/pci.h       |  1 +
 include/linux/pci.h     | 10 ++++++++++
 4 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index aeccc91..3b4a905 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -735,3 +735,51 @@ int pci_num_vf(struct pci_dev *dev)
 		return dev->sriov->nr_virtfn;
 }
 EXPORT_SYMBOL_GPL(pci_num_vf);
+
+/**
+ * pci_sriov_set_totalvfs -- reduce the TotalVFs available
+ * @dev: the PCI PF device
+ * numvfs: number that should be used for TotalVFs supported
+ *
+ * Should be called from PF driver's probe routine with
+ * device's mutex held.
+ *
+ * Returns 0 if PF is an SRIOV-capable device and
+ * value of numvfs valid. If not a PF with VFS, return -EINVAL;
+ * if VFs already enabled, return -EBUSY.
+ */
+int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
+{
+	if (!dev || !dev->is_physfn || (numvfs > dev->sriov->total))
+		return -EINVAL;
+
+	/* Shouldn't change if VFs already enabled */
+	if (dev->sriov->ctrl & PCI_SRIOV_CTRL_VFE)
+		return -EBUSY;
+	else
+		dev->sriov->drvttl = numvfs;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_sriov_set_totalvfs);
+
+/**
+ * pci_sriov_get_totalvfs -- get total VFs supported on this devic3
+ * @dev: the PCI PF device
+ *
+ * For a PCIe device with SRIOV support, return the PCIe
+ * SRIOV capability value of TotalVFs or the value of drvttl
+ * if the driver reduced it.  Otherwise, -EINVAL.
+ */
+int pci_sriov_get_totalvfs(struct pci_dev *dev)
+{
+	if (!dev || !dev->is_physfn)
+		return -EINVAL;
+
+	if (dev->sriov->drvttl)
+		return dev->sriov->drvttl;
+	else
+		return dev->sriov->total;
+}
+EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
+
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index cbcdd8d..e9c967f 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -413,7 +413,7 @@ static ssize_t sriov_totalvfs_show(struct device *dev,
 	u16 total;
 
 	pdev = to_pci_dev(dev);
-	total = pdev->sriov->total;
+	total = pci_sriov_get_totalvfs(pdev);
 	return sprintf(buf, "%u\n", total);
 }
 
@@ -459,7 +459,7 @@ static ssize_t sriov_numvfs_store(struct device *dev,
 	}
 
 	/* if enabling vf's ... */
-	total = pdev->sriov->total;
+	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->nr_virtfn == 0) {
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6f6cd14..553bbba 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -240,6 +240,7 @@ struct pci_sriov {
 	u16 stride;		/* following VF stride */
 	u32 pgsz;		/* page size for BAR alignment */
 	u8 link;		/* Function Dependency Link */
+	u16 drvttl;		/* max num VFs driver supports */
 	struct pci_dev *dev;	/* lowest numbered PF */
 	struct pci_dev *self;	/* this PF */
 	struct mutex lock;	/* lock for VF bus */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 7ef8fba..1ad8249 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1611,6 +1611,8 @@ extern int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
 extern void pci_disable_sriov(struct pci_dev *dev);
 extern irqreturn_t pci_sriov_migration(struct pci_dev *dev);
 extern int pci_num_vf(struct pci_dev *dev);
+extern int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
+extern int pci_sriov_get_totalvfs(struct pci_dev *dev);
 #else
 static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
 {
@@ -1627,6 +1629,14 @@ static inline int pci_num_vf(struct pci_dev *dev)
 {
 	return 0;
 }
+static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
+{
+	return 0;
+}
+static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
+{
+	return 0;
+}
 #endif
 
 #if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE)
-- 
1.7.10.2.552.gaa3bb87

Re: [PATCH 4/4] PCI,sriov: provide method to reduce the number of total VFs supported

[Bjorn Helgaas]

On Mon, Nov 5, 2012 at 1:20 PM, Donald Dutile <ddutile@redhat.com> wrote:
> Some implementations of SRIOV provide a capability structure
> value of TotalVFs that is greater than what the software can support.
> Provide a method to reduce the capability structure reported value
> to the value the driver can support.
> This ensures sysfs reports the current capability of the system,
> hardware and software.
> Example for its use: igb & ixgbe -- report 8 & 64 as TotalVFs,
> but drivers only support 7 & 63 maximum.
>
> Signed-off-by: Donald Dutile <ddutile@redhat.com>

I don't really understand the purpose of pci_sriov_set_totalvfs().  I
think a driver should enforce its limit at the point where it enables
the VFs.  I think the driver should do that to be defensive regardless
of whether we add pci_sriov_set_totalvfs().

So is this just to make the driver's limit visible to user-space?  How
is it better than having the user specify the number he'd like, and
having the driver reduce that if necessary?  The user will be able to
read sriov_numvfs to learn how many the driver enabled, right?

If we allow sriov_totalvfs to contain a different number than the
SR-IOV capability has (as seen via "lspci"), then we have to explain
to users why they might be different.

I'm playing devil's advocate a bit here because I really don't know
that much about SR-IOV or what the administrative interfaces look
like.

>  drivers/pci/iov.c       | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci-sysfs.c |  4 ++--
>  drivers/pci/pci.h       |  1 +
>  include/linux/pci.h     | 10 ++++++++++
>  4 files changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index aeccc91..3b4a905 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -735,3 +735,51 @@ int pci_num_vf(struct pci_dev *dev)
>                 return dev->sriov->nr_virtfn;
>  }
>  EXPORT_SYMBOL_GPL(pci_num_vf);
> +
> +/**
> + * pci_sriov_set_totalvfs -- reduce the TotalVFs available
> + * @dev: the PCI PF device
> + * numvfs: number that should be used for TotalVFs supported
> + *
> + * Should be called from PF driver's probe routine with
> + * device's mutex held.
> + *
> + * Returns 0 if PF is an SRIOV-capable device and
> + * value of numvfs valid. If not a PF with VFS, return -EINVAL;
> + * if VFs already enabled, return -EBUSY.
> + */
> +int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
> +{
> +       if (!dev || !dev->is_physfn || (numvfs > dev->sriov->total))
> +               return -EINVAL;
> +
> +       /* Shouldn't change if VFs already enabled */
> +       if (dev->sriov->ctrl & PCI_SRIOV_CTRL_VFE)
> +               return -EBUSY;
> +       else
> +               dev->sriov->drvttl = numvfs;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_sriov_set_totalvfs);
> +
> +/**
> + * pci_sriov_get_totalvfs -- get total VFs supported on this devic3
> + * @dev: the PCI PF device
> + *
> + * For a PCIe device with SRIOV support, return the PCIe
> + * SRIOV capability value of TotalVFs or the value of drvttl
> + * if the driver reduced it.  Otherwise, -EINVAL.
> + */
> +int pci_sriov_get_totalvfs(struct pci_dev *dev)
> +{
> +       if (!dev || !dev->is_physfn)
> +               return -EINVAL;
> +
> +       if (dev->sriov->drvttl)
> +               return dev->sriov->drvttl;
> +       else
> +               return dev->sriov->total;
> +}
> +EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
> +
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index cbcdd8d..e9c967f 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -413,7 +413,7 @@ static ssize_t sriov_totalvfs_show(struct device *dev,
>         u16 total;
>
>         pdev = to_pci_dev(dev);
> -       total = pdev->sriov->total;
> +       total = pci_sriov_get_totalvfs(pdev);
>         return sprintf(buf, "%u\n", total);
>  }
>
> @@ -459,7 +459,7 @@ static ssize_t sriov_numvfs_store(struct device *dev,
>         }
>
>         /* if enabling vf's ... */
> -       total = pdev->sriov->total;
> +       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->nr_virtfn == 0) {
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 6f6cd14..553bbba 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -240,6 +240,7 @@ struct pci_sriov {
>         u16 stride;             /* following VF stride */
>         u32 pgsz;               /* page size for BAR alignment */
>         u8 link;                /* Function Dependency Link */
> +       u16 drvttl;             /* max num VFs driver supports */
>         struct pci_dev *dev;    /* lowest numbered PF */
>         struct pci_dev *self;   /* this PF */
>         struct mutex lock;      /* lock for VF bus */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 7ef8fba..1ad8249 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1611,6 +1611,8 @@ extern int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
>  extern void pci_disable_sriov(struct pci_dev *dev);
>  extern irqreturn_t pci_sriov_migration(struct pci_dev *dev);
>  extern int pci_num_vf(struct pci_dev *dev);
> +extern int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
> +extern int pci_sriov_get_totalvfs(struct pci_dev *dev);
>  #else
>  static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
>  {
> @@ -1627,6 +1629,14 @@ static inline int pci_num_vf(struct pci_dev *dev)
>  {
>         return 0;
>  }
> +static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
> +{
> +       return 0;
> +}
> +static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
> +{
> +       return 0;
> +}
>  #endif
>
>  #if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE)
> --
> 1.7.10.2.552.gaa3bb87
>

Re: [PATCH 4/4] PCI,sriov: provide method to reduce the number of total VFs supported

[Don Dutile]

On 11/10/2012 04:33 PM, Bjorn Helgaas wrote:
> On Mon, Nov 5, 2012 at 1:20 PM, Donald Dutile<ddutile@redhat.com>  wrote:
>> Some implementations of SRIOV provide a capability structure
>> value of TotalVFs that is greater than what the software can support.
>> Provide a method to reduce the capability structure reported value
>> to the value the driver can support.
>> This ensures sysfs reports the current capability of the system,
>> hardware and software.
>> Example for its use: igb&  ixgbe -- report 8&  64 as TotalVFs,
>> but drivers only support 7&  63 maximum.
>>
>> Signed-off-by: Donald Dutile<ddutile@redhat.com>
>
> I don't really understand the purpose of pci_sriov_set_totalvfs().  I
> think a driver should enforce its limit at the point where it enables
> the VFs.  I think the driver should do that to be defensive regardless
> of whether we add pci_sriov_set_totalvfs().
>
I received feedback from the driver folks that putting this check
into the core reduces dependencies on drivers doing the right check
at the right time.  It's similar to a similar argument made that the
core ought to call pci_sriov_enable/disable(), and not the driver(s).

> So is this just to make the driver's limit visible to user-space?  How
Yes.
> is it better than having the user specify the number he'd like, and
> having the driver reduce that if necessary?  The user will be able to
> read sriov_numvfs to learn how many the driver enabled, right?
>
Most drivers don't enable-what-can-be-enabled;  the request succeeds for
the number of VFS specified, or it tears all the VFs configured up to the
failure, and returns failure, i.e., all or nothing.  I would tend to agree
with this logic, since SRIOV resources (BARS,MSI, etc.) are architected
as n-resources/VF*num_vfs_enabled.

The primary purpose of the patch set is to drive SRIOV/VF enablement
from userspace.  To simplify userspace, it's best to present to it
what *can* be enabled.  Right now, that's read totalvfs & check numvfs;
Having userspace space do 'trial & error' by 'try this number, nope, ok,
try this number'.... vs. totalvfs = numvfs == num-of-vfs-that-can-be-enabled
seems more predictable.

> If we allow sriov_totalvfs to contain a different number than the
> SR-IOV capability has (as seen via "lspci"), then we have to explain
> to users why they might be different.
>
The difference already has to be explained, since this state already
exists today, and on two of the first SRIOV devices in the market.
Now, if we want to quirk this info vs providing a driver interface,
we can change that part of the design.  The interface lets the driver
(policy) change with the driver vs having to do a driver change & a quirk change.
But, we know it exists from day-1, so it should be handled as cleanly
as possible wrt userspace tools from day-1.

> I'm playing devil's advocate a bit here because I really don't know
> that much about SR-IOV or what the administrative interfaces look
> like.
>
>>   drivers/pci/iov.c       | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/pci/pci-sysfs.c |  4 ++--
>>   drivers/pci/pci.h       |  1 +
>>   include/linux/pci.h     | 10 ++++++++++
>>   4 files changed, 61 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index aeccc91..3b4a905 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -735,3 +735,51 @@ int pci_num_vf(struct pci_dev *dev)
>>                  return dev->sriov->nr_virtfn;
>>   }
>>   EXPORT_SYMBOL_GPL(pci_num_vf);
>> +
>> +/**
>> + * pci_sriov_set_totalvfs -- reduce the TotalVFs available
>> + * @dev: the PCI PF device
>> + * numvfs: number that should be used for TotalVFs supported
>> + *
>> + * Should be called from PF driver's probe routine with
>> + * device's mutex held.
>> + *
>> + * Returns 0 if PF is an SRIOV-capable device and
>> + * value of numvfs valid. If not a PF with VFS, return -EINVAL;
>> + * if VFs already enabled, return -EBUSY.
>> + */
>> +int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
>> +{
>> +       if (!dev || !dev->is_physfn || (numvfs>  dev->sriov->total))
>> +               return -EINVAL;
>> +
>> +       /* Shouldn't change if VFs already enabled */
>> +       if (dev->sriov->ctrl&  PCI_SRIOV_CTRL_VFE)
>> +               return -EBUSY;
>> +       else
>> +               dev->sriov->drvttl = numvfs;
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(pci_sriov_set_totalvfs);
>> +
>> +/**
>> + * pci_sriov_get_totalvfs -- get total VFs supported on this devic3
>> + * @dev: the PCI PF device
>> + *
>> + * For a PCIe device with SRIOV support, return the PCIe
>> + * SRIOV capability value of TotalVFs or the value of drvttl
>> + * if the driver reduced it.  Otherwise, -EINVAL.
>> + */
>> +int pci_sriov_get_totalvfs(struct pci_dev *dev)
>> +{
>> +       if (!dev || !dev->is_physfn)
>> +               return -EINVAL;
>> +
>> +       if (dev->sriov->drvttl)
>> +               return dev->sriov->drvttl;
>> +       else
>> +               return dev->sriov->total;
>> +}
>> +EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
>> +
>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> index cbcdd8d..e9c967f 100644
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
>> @@ -413,7 +413,7 @@ static ssize_t sriov_totalvfs_show(struct device *dev,
>>          u16 total;
>>
>>          pdev = to_pci_dev(dev);
>> -       total = pdev->sriov->total;
>> +       total = pci_sriov_get_totalvfs(pdev);
>>          return sprintf(buf, "%u\n", total);
>>   }
>>
>> @@ -459,7 +459,7 @@ static ssize_t sriov_numvfs_store(struct device *dev,
>>          }
>>
>>          /* if enabling vf's ... */
>> -       total = pdev->sriov->total;
>> +       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->nr_virtfn == 0) {
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 6f6cd14..553bbba 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -240,6 +240,7 @@ struct pci_sriov {
>>          u16 stride;             /* following VF stride */
>>          u32 pgsz;               /* page size for BAR alignment */
>>          u8 link;                /* Function Dependency Link */
>> +       u16 drvttl;             /* max num VFs driver supports */
>>          struct pci_dev *dev;    /* lowest numbered PF */
>>          struct pci_dev *self;   /* this PF */
>>          struct mutex lock;      /* lock for VF bus */
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 7ef8fba..1ad8249 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1611,6 +1611,8 @@ extern int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
>>   extern void pci_disable_sriov(struct pci_dev *dev);
>>   extern irqreturn_t pci_sriov_migration(struct pci_dev *dev);
>>   extern int pci_num_vf(struct pci_dev *dev);
>> +extern int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>> +extern int pci_sriov_get_totalvfs(struct pci_dev *dev);
>>   #else
>>   static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
>>   {
>> @@ -1627,6 +1629,14 @@ static inline int pci_num_vf(struct pci_dev *dev)
>>   {
>>          return 0;
>>   }
>> +static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
>> +{
>> +       return 0;
>> +}
>> +static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
>> +{
>> +       return 0;
>> +}
>>   #endif
>>
>>   #if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE)
>> --
>> 1.7.10.2.552.gaa3bb87
>>

Re: [PATCH 4/4] PCI,sriov: provide method to reduce the number of total VFs supported

[Greg Rose]

On Mon, 12 Nov 2012 11:33:39 -0500
Don Dutile <ddutile@redhat.com> wrote:

> On 11/10/2012 04:33 PM, Bjorn Helgaas wrote:
> > On Mon, Nov 5, 2012 at 1:20 PM, Donald Dutile<ddutile@redhat.com>
> > wrote:
> >> Some implementations of SRIOV provide a capability structure
> >> value of TotalVFs that is greater than what the software can
> >> support. Provide a method to reduce the capability structure
> >> reported value to the value the driver can support.
> >> This ensures sysfs reports the current capability of the system,
> >> hardware and software.
> >> Example for its use: igb&  ixgbe -- report 8&  64 as TotalVFs,
> >> but drivers only support 7&  63 maximum.
> >>
> >> Signed-off-by: Donald Dutile<ddutile@redhat.com>
> >
> > I don't really understand the purpose of pci_sriov_set_totalvfs().
> > I think a driver should enforce its limit at the point where it
> > enables the VFs.  I think the driver should do that to be defensive
> > regardless of whether we add pci_sriov_set_totalvfs().
> >
> I received feedback from the driver folks that putting this check
> into the core reduces dependencies on drivers doing the right check
> at the right time.  It's similar to a similar argument made that the
> core ought to call pci_sriov_enable/disable(), and not the driver(s).
> 
> > So is this just to make the driver's limit visible to user-space?
> > How
> Yes.
> > is it better than having the user specify the number he'd like, and
> > having the driver reduce that if necessary?  The user will be able
> > to read sriov_numvfs to learn how many the driver enabled, right?
> >
> Most drivers don't enable-what-can-be-enabled;  the request succeeds
> for the number of VFS specified, or it tears all the VFs configured
> up to the failure, and returns failure, i.e., all or nothing.  I
> would tend to agree with this logic, since SRIOV resources (BARS,MSI,
> etc.) are architected as n-resources/VF*num_vfs_enabled.
> 
> The primary purpose of the patch set is to drive SRIOV/VF enablement
> from userspace.  To simplify userspace, it's best to present to it
> what *can* be enabled.  Right now, that's read totalvfs & check
> numvfs; Having userspace space do 'trial & error' by 'try this
> number, nope, ok, try this number'.... vs. totalvfs = numvfs ==
> num-of-vfs-that-can-be-enabled seems more predictable.
> 
> > If we allow sriov_totalvfs to contain a different number than the
> > SR-IOV capability has (as seen via "lspci"), then we have to explain
> > to users why they might be different.
> >
> The difference already has to be explained, since this state already
> exists today, and on two of the first SRIOV devices in the market.
> Now, if we want to quirk this info vs providing a driver interface,
> we can change that part of the design.  The interface lets the driver
> (policy) change with the driver vs having to do a driver change & a
> quirk change. But, we know it exists from day-1, so it should be
> handled as cleanly as possible wrt userspace tools from day-1.

I'm in agreement with Don on this one.  The callback that allows the
driver to notify the PCI subsystem of the number of usable VFs (vs the
number advertised through the PCIe SR-IOV capability) makes sense.
Management SW doesn't have to go through a discovery process to figure
out what number of VFs it can actually allocate.  And SR-IOV capable
devices have a number of reasons why they might want to advertise to SW
fewer VFs than the device might actually support in HW.  Virtual
functions use resources from the device that are no longer available to
the physical function.  In order to preserve some level of capability
that the physical function can reduce the number of VFs advertised and
reserve those resources to itself.

- Greg

> 
> > I'm playing devil's advocate a bit here because I really don't know
> > that much about SR-IOV or what the administrative interfaces look
> > like.
> >
> >>   drivers/pci/iov.c       | 48
> >> ++++++++++++++++++++++++++++++++++++++++++++++++
> >> drivers/pci/pci-sysfs.c |  4 ++-- drivers/pci/pci.h       |  1 +
> >>   include/linux/pci.h     | 10 ++++++++++
> >>   4 files changed, 61 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> >> index aeccc91..3b4a905 100644
> >> --- a/drivers/pci/iov.c
> >> +++ b/drivers/pci/iov.c
> >> @@ -735,3 +735,51 @@ int pci_num_vf(struct pci_dev *dev)
> >>                  return dev->sriov->nr_virtfn;
> >>   }
> >>   EXPORT_SYMBOL_GPL(pci_num_vf);
> >> +
> >> +/**
> >> + * pci_sriov_set_totalvfs -- reduce the TotalVFs available
> >> + * @dev: the PCI PF device
> >> + * numvfs: number that should be used for TotalVFs supported
> >> + *
> >> + * Should be called from PF driver's probe routine with
> >> + * device's mutex held.
> >> + *
> >> + * Returns 0 if PF is an SRIOV-capable device and
> >> + * value of numvfs valid. If not a PF with VFS, return -EINVAL;
> >> + * if VFs already enabled, return -EBUSY.
> >> + */
> >> +int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
> >> +{
> >> +       if (!dev || !dev->is_physfn || (numvfs>
> >> dev->sriov->total))
> >> +               return -EINVAL;
> >> +
> >> +       /* Shouldn't change if VFs already enabled */
> >> +       if (dev->sriov->ctrl&  PCI_SRIOV_CTRL_VFE)
> >> +               return -EBUSY;
> >> +       else
> >> +               dev->sriov->drvttl = numvfs;
> >> +
> >> +       return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(pci_sriov_set_totalvfs);
> >> +
> >> +/**
> >> + * pci_sriov_get_totalvfs -- get total VFs supported on this
> >> devic3
> >> + * @dev: the PCI PF device
> >> + *
> >> + * For a PCIe device with SRIOV support, return the PCIe
> >> + * SRIOV capability value of TotalVFs or the value of drvttl
> >> + * if the driver reduced it.  Otherwise, -EINVAL.
> >> + */
> >> +int pci_sriov_get_totalvfs(struct pci_dev *dev)
> >> +{
> >> +       if (!dev || !dev->is_physfn)
> >> +               return -EINVAL;
> >> +
> >> +       if (dev->sriov->drvttl)
> >> +               return dev->sriov->drvttl;
> >> +       else
> >> +               return dev->sriov->total;
> >> +}
> >> +EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
> >> +
> >> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> >> index cbcdd8d..e9c967f 100644
> >> --- a/drivers/pci/pci-sysfs.c
> >> +++ b/drivers/pci/pci-sysfs.c
> >> @@ -413,7 +413,7 @@ static ssize_t sriov_totalvfs_show(struct
> >> device *dev, u16 total;
> >>
> >>          pdev = to_pci_dev(dev);
> >> -       total = pdev->sriov->total;
> >> +       total = pci_sriov_get_totalvfs(pdev);
> >>          return sprintf(buf, "%u\n", total);
> >>   }
> >>
> >> @@ -459,7 +459,7 @@ static ssize_t sriov_numvfs_store(struct
> >> device *dev, }
> >>
> >>          /* if enabling vf's ... */
> >> -       total = pdev->sriov->total;
> >> +       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->nr_virtfn == 0) {
> >> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> >> index 6f6cd14..553bbba 100644
> >> --- a/drivers/pci/pci.h
> >> +++ b/drivers/pci/pci.h
> >> @@ -240,6 +240,7 @@ struct pci_sriov {
> >>          u16 stride;             /* following VF stride */
> >>          u32 pgsz;               /* page size for BAR alignment */
> >>          u8 link;                /* Function Dependency Link */
> >> +       u16 drvttl;             /* max num VFs driver supports */
> >>          struct pci_dev *dev;    /* lowest numbered PF */
> >>          struct pci_dev *self;   /* this PF */
> >>          struct mutex lock;      /* lock for VF bus */
> >> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >> index 7ef8fba..1ad8249 100644
> >> --- a/include/linux/pci.h
> >> +++ b/include/linux/pci.h
> >> @@ -1611,6 +1611,8 @@ extern int pci_enable_sriov(struct pci_dev
> >> *dev, int nr_virtfn); extern void pci_disable_sriov(struct pci_dev
> >> *dev); extern irqreturn_t pci_sriov_migration(struct pci_dev *dev);
> >>   extern int pci_num_vf(struct pci_dev *dev);
> >> +extern int pci_sriov_set_totalvfs(struct pci_dev *dev, u16
> >> numvfs); +extern int pci_sriov_get_totalvfs(struct pci_dev *dev);
> >>   #else
> >>   static inline int pci_enable_sriov(struct pci_dev *dev, int
> >> nr_virtfn) {
> >> @@ -1627,6 +1629,14 @@ static inline int pci_num_vf(struct pci_dev
> >> *dev) {
> >>          return 0;
> >>   }
> >> +static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16
> >> numvfs) +{
> >> +       return 0;
> >> +}
> >> +static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
> >> +{
> >> +       return 0;
> >> +}
> >>   #endif
> >>
> >>   #if defined(CONFIG_HOTPLUG_PCI) ||
> >> defined(CONFIG_HOTPLUG_PCI_MODULE) --
> >> 1.7.10.2.552.gaa3bb87
> >>
> 

[PATCH 5/8] ixgbe: refactor mailbox ops init

[Donald Dutile]

There is no actual dependency on initialization of the mailbox ops on
whether SR-IOV is enabled or not and it doesn't hurt to go ahead and
initialize ops unconditionally.  Move the initialization into the device
probe so that the mailbox ops are initialized at the time we have the
board info necessary to do it.

Note: This is part of a 4-part patch set that demonstrates
      how a PF driver can enable its VF devices via sysfs.
      The final patch will be submitted by Intel via
      the linux-net mailing list.

Signed-off-by: Greg Rose <gregory.v.rose@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  | 5 ++++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 9 +--------
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h | 3 +--
 3 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 868af69..8d2a507 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7249,7 +7249,10 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
 	}
 
 #ifdef CONFIG_PCI_IOV
-	ixgbe_enable_sriov(adapter, ii);
+	/* Mailbox */
+	ixgbe_init_mbx_params_pf(hw);
+	memcpy(&hw->mbx.ops, ii->mbx_ops, sizeof(hw->mbx.ops));
+	ixgbe_enable_sriov(adapter);
 
 #endif
 	netdev->features = NETIF_F_SG |
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
index dce48bf..9aeb929 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -44,8 +44,7 @@
 #include "ixgbe_sriov.h"
 
 #ifdef CONFIG_PCI_IOV
-void ixgbe_enable_sriov(struct ixgbe_adapter *adapter,
-			 const struct ixgbe_info *ii)
+void ixgbe_enable_sriov(struct ixgbe_adapter *adapter);
 {
 	struct ixgbe_hw *hw = &adapter->hw;
 	int num_vf_macvlans, i;
@@ -124,12 +123,6 @@ void ixgbe_enable_sriov(struct ixgbe_adapter *adapter,
 		kcalloc(adapter->num_vfs,
 			sizeof(struct vf_data_storage), GFP_KERNEL);
 	if (adapter->vfinfo) {
-		/* Now that we're sure SR-IOV is enabled
-		 * and memory allocated set up the mailbox parameters
-		 */
-		ixgbe_init_mbx_params_pf(hw);
-		memcpy(&hw->mbx.ops, ii->mbx_ops, sizeof(hw->mbx.ops));
-
 		/* limit trafffic classes based on VFs enabled */
 		if ((adapter->hw.mac.type == ixgbe_mac_82599EB) &&
 		    (adapter->num_vfs < 16)) {
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
index 1be1d30..d96ead2 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
@@ -43,8 +43,7 @@ int ixgbe_ndo_get_vf_config(struct net_device *netdev,
 void ixgbe_check_vf_rate_limit(struct ixgbe_adapter *adapter);
 void ixgbe_disable_sriov(struct ixgbe_adapter *adapter);
 #ifdef CONFIG_PCI_IOV
-void ixgbe_enable_sriov(struct ixgbe_adapter *adapter,
-			const struct ixgbe_info *ii);
+void ixgbe_enable_sriov(struct ixgbe_adapter *adapter);
 #endif
 
 
-- 
1.7.10.2.552.gaa3bb87

[PATCH 6/8] ixgbe: refactor SRIOV enable and disable for sysfs interface

[Donald Dutile]

In preparation for enable/disable of SR-IOV via the pci sysfs interface
move some core SR-IOV enablement code that would be common to module
parameter usage or callback from the pci bus driver to a separate
function so that it can be used by either method.

Note: This is part of a 4-part patch set that demonstrates
      how a PF driver can enable its VF devices via sysfs.
      The final patch will be submitted by Intel via
      the linux-net mailing list.

Signed-off-by: Greg Rose <gregory.v.rose@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 98 +++++++++++++++-----------
 1 file changed, 58 insertions(+), 40 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
index 9aeb929..7c068b4 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -44,49 +44,11 @@
 #include "ixgbe_sriov.h"
 
 #ifdef CONFIG_PCI_IOV
-void ixgbe_enable_sriov(struct ixgbe_adapter *adapter);
+static int __ixgbe_enable_sriov(struct ixgbe_adapter *adapter)
 {
 	struct ixgbe_hw *hw = &adapter->hw;
 	int num_vf_macvlans, i;
 	struct vf_macvlans *mv_list;
-	int pre_existing_vfs = 0;
-
-	pre_existing_vfs = pci_num_vf(adapter->pdev);
-	if (!pre_existing_vfs && !adapter->num_vfs)
-		return;
-
-	/* If there are pre-existing VFs then we have to force
-	 * use of that many because they were not deleted the last
-	 * time someone removed the PF driver.  That would have
-	 * been because they were allocated to guest VMs and can't
-	 * be removed.  Go ahead and just re-enable the old amount.
-	 * If the user wants to change the number of VFs they can
-	 * use ethtool while making sure no VFs are allocated to
-	 * guest VMs... i.e. the right way.
-	 */
-	if (pre_existing_vfs) {
-		adapter->num_vfs = pre_existing_vfs;
-		dev_warn(&adapter->pdev->dev, "Virtual Functions already "
-			 "enabled for this device - Please reload all "
-			 "VF drivers to avoid spoofed packet errors\n");
-	} else {
-		int err;
-		/*
-		 * The 82599 supports up to 64 VFs per physical function
-		 * but this implementation limits allocation to 63 so that
-		 * basic networking resources are still available to the
-		 * physical function.  If the user requests greater thn
-		 * 63 VFs then it is an error - reset to default of zero.
-		 */
-		adapter->num_vfs = min_t(unsigned int, adapter->num_vfs, 63);
-
-		err = pci_enable_sriov(adapter->pdev, adapter->num_vfs);
-		if (err) {
-			e_err(probe, "Failed to enable PCI sriov: %d\n", err);
-			adapter->num_vfs = 0;
-			return;
-		}
-	}
 
 	adapter->flags |= IXGBE_FLAG_SRIOV_ENABLED;
 	e_info(probe, "SR-IOV enabled with %d VFs\n", adapter->num_vfs);
@@ -156,10 +118,66 @@ void ixgbe_enable_sriov(struct ixgbe_adapter *adapter);
 		/* enable spoof checking for all VFs */
 		for (i = 0; i < adapter->num_vfs; i++)
 			adapter->vfinfo[i].spoofchk_enabled = true;
+		return 0;
+	}
+
+	return -ENOMEM;
+}
+
+/*
+ * Note this function is called when the user wants to enable SR-IOV
+ * VFs using the now deprecated module parameter
+ */
+void ixgbe_enable_sriov(struct ixgbe_adapter *adapter)
+{
+	int pre_existing_vfs = 0;
+
+	pre_existing_vfs = pci_num_vf(adapter->pdev);
+	if (!pre_existing_vfs && !adapter->num_vfs)
 		return;
+
+	if (!pre_existing_vfs)
+		dev_warn(&adapter->pdev->dev,
+		 	 "Enabling SR-IOV VFs using the module parameter is deprecated - please use the pci sysfs interface.\n");
+
+	/*
+	 * If there are pre-existing VFs then we have to force
+	 * use of that many - over ride any module parameter value.
+	 * This may result from the user unloading the PF driver
+	 * while VFs were assigned to guest VMs or because the VFs
+	 * have been created via the new PCI SR-IOV sysfs interface.
+	 */
+	if (pre_existing_vfs) {
+		adapter->num_vfs = pre_existing_vfs;
+		dev_warn(&adapter->pdev->dev, "Virtual Functions already "
+			 "enabled for this device - Please reload all "
+			 "VF drivers to avoid spoofed packet errors\n");
+	} else {
+		int err;
+		/*
+		 * The 82599 supports up to 64 VFs per physical function
+		 * but this implementation limits allocation to 63 so that
+		 * basic networking resources are still available to the
+		 * physical function.  If the user requests greater thn
+		 * 63 VFs then it is an error - reset to default of zero.
+		 */
+		adapter->num_vfs = min_t(unsigned int, adapter->num_vfs, 63);
+
+		err = pci_enable_sriov(adapter->pdev, adapter->num_vfs);
+		if (err) {
+			e_err(probe, "Failed to enable PCI sriov: %d\n", err);
+			adapter->num_vfs = 0;
+			return;
+		}
 	}
 
-	/* Oh oh */
+	if (!__ixgbe_enable_sriov(adapter))
+		return;
+
+	/*
+	 * If we have gotten to this point then there is no memory available
+	 * to manage the VF devices - print message and bail.
+	 */
 	e_err(probe, "Unable to allocate memory for VF Data Storage - "
 	      "SRIOV disabled\n");
 	ixgbe_disable_sriov(adapter);
-- 
1.7.10.2.552.gaa3bb87

[PATCH 7/8] ixgbe: sysfs sriov configuration callback support

[Donald Dutile]

Implement callbacks in the driver for the new PCI bus driver
interface that allows the user to enable/disable SR-IOV VFs
in a device via the sysfs interface.

Note: This is part of a 4-part patch set that demonstrates
      how a PF driver can enable its VF devices via sysfs.
      The final patch will be submitted by Intel via
      the linux-net mailing list.

Signed-off-by: Greg Rose <gregory.v.rose@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h       |  3 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  | 28 +++++++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 89 +++++++++++++++++++++++++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h |  6 +-
 4 files changed, 120 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 5bd2676..89fa3f8 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -753,5 +753,8 @@ extern int ixgbe_ptp_hwtstamp_ioctl(struct ixgbe_adapter *adapter,
 extern void ixgbe_ptp_start_cyclecounter(struct ixgbe_adapter *adapter);
 extern void ixgbe_ptp_check_pps_event(struct ixgbe_adapter *adapter, u32 eicr);
 #endif /* CONFIG_IXGBE_PTP */
+#ifdef CONFIG_PCI_IOV
+void ixgbe_sriov_reinit(struct ixgbe_adapter *adapter);
+#endif
 
 #endif /* _IXGBE_H_ */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 8d2a507..5d868e4 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -6784,6 +6784,26 @@ int ixgbe_setup_tc(struct net_device *dev, u8 tc)
 }
 
 #endif /* CONFIG_IXGBE_DCB */
+#ifdef CONFIG_PCI_IOV
+void ixgbe_sriov_reinit(struct ixgbe_adapter *adapter)
+{
+	struct net_device *netdev = adapter->netdev;
+
+	rtnl_lock();
+#ifdef CONFIG_IXGBE_DCB
+	ixgbe_setup_tc(netdev, netdev_get_num_tc(netdev));
+#else
+	if (netif_running(netdev))
+		ixgbe_close(netdev);
+	ixgbe_clear_interrupt_scheme(adapter);
+	ixgbe_init_interrupt_scheme(adapter);
+	if (netif_running(netdev))
+		ixgbe_open(netdev);
+#endif
+	rtnl_unlock();
+}
+
+#endif
 void ixgbe_do_reset(struct net_device *netdev)
 {
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
@@ -7519,7 +7539,12 @@ static void __devexit ixgbe_remove(struct pci_dev *pdev)
 	if (netdev->reg_state == NETREG_REGISTERED)
 		unregister_netdev(netdev);
 
-	ixgbe_disable_sriov(adapter);
+	/*
+	 * Only disable SR-IOV on unload if the user specified the now
+	 * deprecated max_vfs module parameter.
+	 */
+	if (max_vfs)
+		ixgbe_disable_sriov(adapter);
 
 	ixgbe_clear_interrupt_scheme(adapter);
 
@@ -7734,6 +7759,7 @@ static struct pci_driver ixgbe_driver = {
 	.resume   = ixgbe_resume,
 #endif
 	.shutdown = ixgbe_shutdown,
+	.sriov_configure = ixgbe_pci_sriov_configure,
 	.err_handler = &ixgbe_err_handler
 };
 
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
index 7c068b4..60a2f4f 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -217,11 +217,12 @@ static bool ixgbe_vfs_are_assigned(struct ixgbe_adapter *adapter)
 }
 
 #endif /* #ifdef CONFIG_PCI_IOV */
-void ixgbe_disable_sriov(struct ixgbe_adapter *adapter)
+int ixgbe_disable_sriov(struct ixgbe_adapter *adapter)
 {
 	struct ixgbe_hw *hw = &adapter->hw;
 	u32 gpie;
 	u32 vmdctl;
+	int rss;
 
 	/* set num VFs to 0 to prevent access to vfinfo */
 	adapter->num_vfs = 0;
@@ -236,7 +237,7 @@ void ixgbe_disable_sriov(struct ixgbe_adapter *adapter)
 
 	/* if SR-IOV is already disabled then there is nothing to do */
 	if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
-		return;
+		return 0;
 
 #ifdef CONFIG_PCI_IOV
 	/*
@@ -246,7 +247,7 @@ void ixgbe_disable_sriov(struct ixgbe_adapter *adapter)
 	 */
 	if (ixgbe_vfs_are_assigned(adapter)) {
 		e_dev_warn("Unloading driver while VFs are assigned - VFs will not be deallocated\n");
-		return;
+		return -EPERM;
 	}
 	/* disable iov and allow time for transactions to clear */
 	pci_disable_sriov(adapter->pdev);
@@ -269,10 +270,92 @@ void ixgbe_disable_sriov(struct ixgbe_adapter *adapter)
 		adapter->flags &= ~IXGBE_FLAG_VMDQ_ENABLED;
 	adapter->ring_feature[RING_F_VMDQ].offset = 0;
 
+	rss = min_t(int, IXGBE_MAX_RSS_INDICES, num_online_cpus());
+	adapter->ring_feature[RING_F_RSS].limit = rss;
+
 	/* take a breather then clean up driver data */
 	msleep(100);
 
 	adapter->flags &= ~IXGBE_FLAG_SRIOV_ENABLED;
+	return 0;
+}
+
+int ixgbe_pci_sriov_enable(struct pci_dev *dev, int num_vfs)
+{
+#ifdef CONFIG_PCI_IOV
+	struct ixgbe_adapter *adapter = pci_get_drvdata(dev);
+	int err = 0;
+	int i;
+	int pre_existing_vfs = pci_num_vf(dev);
+
+	if (pre_existing_vfs && pre_existing_vfs != num_vfs)
+		err = ixgbe_disable_sriov(adapter);
+	else if (pre_existing_vfs && pre_existing_vfs == num_vfs)
+		goto out;
+
+	if (err)
+		goto err_out; 
+
+	/*
+	 * While the SR-IOV capability structure reports total VFs to be
+	 * 64 we limit the actual number that can be allocated to 63 so
+	 * that some transmit/receive resources can be reserved to the
+	 * PF.  The PCI bus driver already checks for other values out of
+	 * range.
+	 */
+	if (num_vfs > 63) {
+		err = -EPERM;
+		goto err_out;
+	}
+
+	adapter->num_vfs = num_vfs;
+
+	if ((err = __ixgbe_enable_sriov(adapter)))
+		goto err_out;
+
+	for (i = 0; i < adapter->num_vfs; i++)
+		ixgbe_vf_configuration(dev, (i | 0x10000000));
+
+	err = pci_enable_sriov(dev, num_vfs);
+	if (err) {
+		e_dev_warn("Failed to enable PCI sriov: %d\n", err);
+		goto err_out;
+	}
+	ixgbe_sriov_reinit(adapter);
+
+out:
+	return num_vfs;
+
+err_out:
+	return err;
+#endif
+	return 0;
+}
+
+int ixgbe_pci_sriov_disable(struct pci_dev *dev)
+{
+	struct ixgbe_adapter *adapter = pci_get_drvdata(dev);
+	int err;
+	u32 current_flags = adapter->flags;
+
+	err = ixgbe_disable_sriov(adapter);
+
+	/* Only reinit if no error and state changed */
+	if (!err && current_flags != adapter->flags) {
+		/* ixgbe_disable_sriov() doesn't clear VMDQ flag */
+		adapter->flags &= ~IXGBE_FLAG_VMDQ_ENABLED;
+		ixgbe_sriov_reinit(adapter);
+	}
+
+	return err;
+}
+
+int ixgbe_pci_sriov_configure(struct pci_dev *dev, int num_vfs)
+{
+	if (num_vfs == 0)
+		return ixgbe_pci_sriov_disable(dev);
+	else
+		return ixgbe_pci_sriov_enable(dev, num_vfs);
 }
 
 static int ixgbe_set_vf_multicasts(struct ixgbe_adapter *adapter,
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
index d96ead2..3d0fb07 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
@@ -41,11 +41,13 @@ int ixgbe_ndo_set_vf_spoofchk(struct net_device *netdev, int vf, bool setting);
 int ixgbe_ndo_get_vf_config(struct net_device *netdev,
 			    int vf, struct ifla_vf_info *ivi);
 void ixgbe_check_vf_rate_limit(struct ixgbe_adapter *adapter);
-void ixgbe_disable_sriov(struct ixgbe_adapter *adapter);
+int ixgbe_disable_sriov(struct ixgbe_adapter *adapter);
 #ifdef CONFIG_PCI_IOV
 void ixgbe_enable_sriov(struct ixgbe_adapter *adapter);
 #endif
-
+int ixgbe_pci_sriov_enable(struct pci_dev *dev, int num_vfs);
+int ixgbe_pci_sriov_disable(struct pci_dev *dev);
+int ixgbe_pci_sriov_configure(struct pci_dev *dev, int num_vfs);
 
 #endif /* _IXGBE_SRIOV_H_ */
 
-- 
1.7.10.2.552.gaa3bb87

[PATCH 8/8] ixgbe: change totalvfs to match support in driver

[Donald Dutile]

 ixgbe claims it supports 64 VFs in its SRIOV capability
 structure, but the driver only supports 63.  Adjust it
 so sysfs sriov configuration checking will check with
 the proper totalvf value.

Note: This is part of a 4-part patch set that demonstrates
      how a PF driver can enable its VF devices via sysfs.
      The final patch will be submitted by Intel via
      the linux-net mailing list.

Signed-off-by: Donald Dutile <ddutile@redhat.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 5d868e4..69e0cff 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7273,6 +7273,7 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
 	ixgbe_init_mbx_params_pf(hw);
 	memcpy(&hw->mbx.ops, ii->mbx_ops, sizeof(hw->mbx.ops));
 	ixgbe_enable_sriov(adapter);
+	pci_sriov_set_totalvfs(pdev, 63);
 
 #endif
 	netdev->features = NETIF_F_SG |
-- 
1.7.10.2.552.gaa3bb87

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

[Bjorn Helgaas]

On Mon, Nov 5, 2012 at 1:20 PM, Donald Dutile <ddutile@redhat.com> wrote:
> Currently, VF enablement by SRIOV-capable PCIe devices is done
> in driver-specific module parameters.  If not setup in modprobe files,
> it requires admin to unload & reload PF drivers with number of desired
> VFs to enable.  Additionally, the enablement is system wide: all
> devices controlled by the same driver have the same number of VFs
> enabled.  Although the latter is probably desired, there are PCI
> configurations setup by system BIOS that may not enable that to occur.
>
> Two files are created if a PCIe device has SRIOV support:
> sriov_totalvfs -- cat-ing this file returns the maximum number
>                   of VFs a PCIe device supports.
> sriov_numvfs -- echo'ing a positive number to this file enables
>                 & configures this number of VFs for this given PCIe
>                 device.
>              -- echo'ing 0 to this file disables and deconfigures
>                 all VFs for this given PCIe device.
>              -- cat-ing this file will return the number of VFs
>                 currently enabled on this PCIe device.
>
> VF enable and disablement is invoked much like other PCIe
> configuration functions -- via a registered callback in the driver,
> i.e., probe, release, etc.  In this case, sriov_configure
>
> PATCH v1->v2:
> -- incorporate more feedback from Ben Hutchings.
> -- (hopefully) correct From & Signed-by for Yinghai Lu's patches (1/8 & 2/8)
>
> RFC V3->PATCH:
> -- incorporate feedback from Ben Hutchings.
> -- clean up poor RFC patches & sanitize through checkpatch.pl
>
> RFC v2->v3:
> -- change the file names to reflect the names used in the SRIOV spec
> -- change to a single file for enable & disable;
>    change driver interface to a single interface.
> -- add more informative messages on failures
> -- add a core method that a driver can invoke to modify
>    the totalvfs reported & supported by a driver.
> -- a set of patches for ixgbe provided by Greg Rose to use the
>    new interfaces; the last patch modified from the original
>    two file, enable/disable interface to the current single file
>    enable/disable. Greg will eventually post the final version
>    of these patches via Intel's usual process for driver patches.
>    Provided here as an example, and enable other SRIOV drivers
>    to see how adoption of the interface can be added.
>
> RFC v1->v2:
> This patch is based on previous 2 patches by Yinghai Lu
> that cleaned up the vga attributes for PCI devices under sysfs,
> and uses visibility-checking group attributes as recommended by
> Greg K-H.
>
> Signed-off-by: Donald Dutile <ddutile@redhat.com>
> ---
>  drivers/pci/iov.c       |  48 ++++++++++++++++
>  drivers/pci/pci-sysfs.c | 179 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  drivers/pci/pci.h       |   2 +
>  drivers/pci/probe.c     |   1 +
>  include/linux/pci.h     |  11 ++++
>  5 files changed, 230 insertions(+), 11 deletions(-)

I applied patches 1-4 to my pci/don-sriov patch, and they appeared in
next-20121114.

I am still expecting a Documentation/ABI update, but I wanted to get
the functional patches merged now because there will only be one more
-next release until 11/26, which will probably be after -rc7.

I'm expecting patches 5-8 to be posted via some other mechanism as
mentioned in those patches.

Thanks!

Bjorn

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

[Rose, Gregory V]

> -----Original Message-----
> From: Bjorn Helgaas [mailto:bhelgaas@google.com]
> Sent: Wednesday, November 14, 2012 12:47 PM
> To: Donald Dutile
> Cc: linux-pci@vger.kernel.org; yuvalmin@broadcom.com;
> bhutchings@solarflare.com; Rose, Gregory V; yinghai@kernel.org;
> davem@davemloft.net
> Subject: Re: [PATCH v2] PCI SRIOV device enable and disable via sysfs
> 
> On Mon, Nov 5, 2012 at 1:20 PM, Donald Dutile <ddutile@redhat.com> wrote:
> > Currently, VF enablement by SRIOV-capable PCIe devices is done in
> > driver-specific module parameters.  If not setup in modprobe files, it
> > requires admin to unload & reload PF drivers with number of desired
> > VFs to enable.  Additionally, the enablement is system wide: all
> > devices controlled by the same driver have the same number of VFs
> > enabled.  Although the latter is probably desired, there are PCI
> > configurations setup by system BIOS that may not enable that to occur.
> >
> > Two files are created if a PCIe device has SRIOV support:
> > sriov_totalvfs -- cat-ing this file returns the maximum number
> >                   of VFs a PCIe device supports.
> > sriov_numvfs -- echo'ing a positive number to this file enables
> >                 & configures this number of VFs for this given PCIe
> >                 device.
> >              -- echo'ing 0 to this file disables and deconfigures
> >                 all VFs for this given PCIe device.
> >              -- cat-ing this file will return the number of VFs
> >                 currently enabled on this PCIe device.
> >
> > VF enable and disablement is invoked much like other PCIe
> > configuration functions -- via a registered callback in the driver,
> > i.e., probe, release, etc.  In this case, sriov_configure
> >
> > PATCH v1->v2:
> > -- incorporate more feedback from Ben Hutchings.
> > -- (hopefully) correct From & Signed-by for Yinghai Lu's patches (1/8
> > & 2/8)
> >
> > RFC V3->PATCH:
> > -- incorporate feedback from Ben Hutchings.
> > -- clean up poor RFC patches & sanitize through checkpatch.pl
> >
> > RFC v2->v3:
> > -- change the file names to reflect the names used in the SRIOV spec
> > -- change to a single file for enable & disable;
> >    change driver interface to a single interface.
> > -- add more informative messages on failures
> > -- add a core method that a driver can invoke to modify
> >    the totalvfs reported & supported by a driver.
> > -- a set of patches for ixgbe provided by Greg Rose to use the
> >    new interfaces; the last patch modified from the original
> >    two file, enable/disable interface to the current single file
> >    enable/disable. Greg will eventually post the final version
> >    of these patches via Intel's usual process for driver patches.
> >    Provided here as an example, and enable other SRIOV drivers
> >    to see how adoption of the interface can be added.
> >
> > RFC v1->v2:
> > This patch is based on previous 2 patches by Yinghai Lu that cleaned
> > up the vga attributes for PCI devices under sysfs, and uses
> > visibility-checking group attributes as recommended by Greg K-H.
> >
> > Signed-off-by: Donald Dutile <ddutile@redhat.com>
> > ---
> >  drivers/pci/iov.c       |  48 ++++++++++++++++
> >  drivers/pci/pci-sysfs.c | 179
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> >  drivers/pci/pci.h       |   2 +
> >  drivers/pci/probe.c     |   1 +
> >  include/linux/pci.h     |  11 ++++
> >  5 files changed, 230 insertions(+), 11 deletions(-)
> 
> I applied patches 1-4 to my pci/don-sriov patch, and they appeared in
> next-20121114.
> 
> I am still expecting a Documentation/ABI update, but I wanted to get the
> functional patches merged now because there will only be one more -next
> release until 11/26, which will probably be after -rc7.
> 
> I'm expecting patches 5-8 to be posted via some other mechanism as
> mentioned in those patches.

I have patches ready to post to netdev but we'll need to coordinate with Dave because they won't compile with these patches.

- Greg

> 
> Thanks!
> 
> Bjorn

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

[Greg Rose]

On Wed, 14 Nov 2012 12:46:57 -0800
Bjorn Helgaas <bhelgaas@google.com> wrote:

> On Mon, Nov 5, 2012 at 1:20 PM, Donald Dutile <ddutile@redhat.com>
> wrote:
> > Currently, VF enablement by SRIOV-capable PCIe devices is done
> > in driver-specific module parameters.  If not setup in modprobe
> > files, it requires admin to unload & reload PF drivers with number
> > of desired VFs to enable.  Additionally, the enablement is system
> > wide: all devices controlled by the same driver have the same
> > number of VFs enabled.  Although the latter is probably desired,
> > there are PCI configurations setup by system BIOS that may not
> > enable that to occur.
> >
> > Two files are created if a PCIe device has SRIOV support:
> > sriov_totalvfs -- cat-ing this file returns the maximum number
> >                   of VFs a PCIe device supports.
> > sriov_numvfs -- echo'ing a positive number to this file enables
> >                 & configures this number of VFs for this given PCIe
> >                 device.
> >              -- echo'ing 0 to this file disables and deconfigures
> >                 all VFs for this given PCIe device.
> >              -- cat-ing this file will return the number of VFs
> >                 currently enabled on this PCIe device.
> >
> > VF enable and disablement is invoked much like other PCIe
> > configuration functions -- via a registered callback in the driver,
> > i.e., probe, release, etc.  In this case, sriov_configure
> >
> > PATCH v1->v2:
> > -- incorporate more feedback from Ben Hutchings.
> > -- (hopefully) correct From & Signed-by for Yinghai Lu's patches
> > (1/8 & 2/8)
> >
> > RFC V3->PATCH:
> > -- incorporate feedback from Ben Hutchings.
> > -- clean up poor RFC patches & sanitize through checkpatch.pl
> >
> > RFC v2->v3:
> > -- change the file names to reflect the names used in the SRIOV spec
> > -- change to a single file for enable & disable;
> >    change driver interface to a single interface.
> > -- add more informative messages on failures
> > -- add a core method that a driver can invoke to modify
> >    the totalvfs reported & supported by a driver.
> > -- a set of patches for ixgbe provided by Greg Rose to use the
> >    new interfaces; the last patch modified from the original
> >    two file, enable/disable interface to the current single file
> >    enable/disable. Greg will eventually post the final version
> >    of these patches via Intel's usual process for driver patches.
> >    Provided here as an example, and enable other SRIOV drivers
> >    to see how adoption of the interface can be added.
> >
> > RFC v1->v2:
> > This patch is based on previous 2 patches by Yinghai Lu
> > that cleaned up the vga attributes for PCI devices under sysfs,
> > and uses visibility-checking group attributes as recommended by
> > Greg K-H.
> >
> > Signed-off-by: Donald Dutile <ddutile@redhat.com>
> > ---
> >  drivers/pci/iov.c       |  48 ++++++++++++++++
> >  drivers/pci/pci-sysfs.c | 179
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> > drivers/pci/pci.h       |   2 + drivers/pci/probe.c     |   1 +
> >  include/linux/pci.h     |  11 ++++
> >  5 files changed, 230 insertions(+), 11 deletions(-)
> 
> I applied patches 1-4 to my pci/don-sriov patch, and they appeared in
> next-20121114.
> 
> I am still expecting a Documentation/ABI update, but I wanted to get
> the functional patches merged now because there will only be one more
> -next release until 11/26, which will probably be after -rc7.
> 
> I'm expecting patches 5-8 to be posted via some other mechanism as
> mentioned in those patches.
> 
> Thanks!
> 
> Bjorn

Now that the merge has occurred I find that there is a small bug in
which a return code with a positive value isn't handled and thus an
error is generated even though no error occurred.  Please consider this
patch as a fix.

Thanks,

- Greg

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

 drivers/pci/pci-sysfs.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)


diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 05b78b1..2722a33 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -461,9 +461,10 @@ static ssize_t sriov_numvfs_store(struct device *dev,
 					 "Only %d VFs enabled\n",
 					 num_vfs_enabled);
 				return count;
-			} else if (num_vfs_enabled < 0)
-				/* error code from driver callback */
+			} else {
+				/* Return error code or number of VFs */
 				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",

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

[Don Dutile]

On 12/14/2012 01:19 PM, Greg Rose wrote:
> On Wed, 14 Nov 2012 12:46:57 -0800
> Bjorn Helgaas<bhelgaas@google.com>  wrote:
>
>> On Mon, Nov 5, 2012 at 1:20 PM, Donald Dutile<ddutile@redhat.com>
>> wrote:
>>> Currently, VF enablement by SRIOV-capable PCIe devices is done
>>> in driver-specific module parameters.  If not setup in modprobe
>>> files, it requires admin to unload&  reload PF drivers with number
>>> of desired VFs to enable.  Additionally, the enablement is system
>>> wide: all devices controlled by the same driver have the same
>>> number of VFs enabled.  Although the latter is probably desired,
>>> there are PCI configurations setup by system BIOS that may not
>>> enable that to occur.
>>>
>>> Two files are created if a PCIe device has SRIOV support:
>>> sriov_totalvfs -- cat-ing this file returns the maximum number
>>>                    of VFs a PCIe device supports.
>>> sriov_numvfs -- echo'ing a positive number to this file enables
>>>                  &  configures this number of VFs for this given PCIe
>>>                  device.
>>>               -- echo'ing 0 to this file disables and deconfigures
>>>                  all VFs for this given PCIe device.
>>>               -- cat-ing this file will return the number of VFs
>>>                  currently enabled on this PCIe device.
>>>
>>> VF enable and disablement is invoked much like other PCIe
>>> configuration functions -- via a registered callback in the driver,
>>> i.e., probe, release, etc.  In this case, sriov_configure
>>>
>>> PATCH v1->v2:
>>> -- incorporate more feedback from Ben Hutchings.
>>> -- (hopefully) correct From&  Signed-by for Yinghai Lu's patches
>>> (1/8&  2/8)
>>>
>>> RFC V3->PATCH:
>>> -- incorporate feedback from Ben Hutchings.
>>> -- clean up poor RFC patches&  sanitize through checkpatch.pl
>>>
>>> RFC v2->v3:
>>> -- change the file names to reflect the names used in the SRIOV spec
>>> -- change to a single file for enable&  disable;
>>>     change driver interface to a single interface.
>>> -- add more informative messages on failures
>>> -- add a core method that a driver can invoke to modify
>>>     the totalvfs reported&  supported by a driver.
>>> -- a set of patches for ixgbe provided by Greg Rose to use the
>>>     new interfaces; the last patch modified from the original
>>>     two file, enable/disable interface to the current single file
>>>     enable/disable. Greg will eventually post the final version
>>>     of these patches via Intel's usual process for driver patches.
>>>     Provided here as an example, and enable other SRIOV drivers
>>>     to see how adoption of the interface can be added.
>>>
>>> RFC v1->v2:
>>> This patch is based on previous 2 patches by Yinghai Lu
>>> that cleaned up the vga attributes for PCI devices under sysfs,
>>> and uses visibility-checking group attributes as recommended by
>>> Greg K-H.
>>>
>>> Signed-off-by: Donald Dutile<ddutile@redhat.com>
>>> ---
>>>   drivers/pci/iov.c       |  48 ++++++++++++++++
>>>   drivers/pci/pci-sysfs.c | 179
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>>> drivers/pci/pci.h       |   2 + drivers/pci/probe.c     |   1 +
>>>   include/linux/pci.h     |  11 ++++
>>>   5 files changed, 230 insertions(+), 11 deletions(-)
>>
>> I applied patches 1-4 to my pci/don-sriov patch, and they appeared in
>> next-20121114.
>>
>> I am still expecting a Documentation/ABI update, but I wanted to get
>> the functional patches merged now because there will only be one more
>> -next release until 11/26, which will probably be after -rc7.
>>
>> I'm expecting patches 5-8 to be posted via some other mechanism as
>> mentioned in those patches.
>>
>> Thanks!
>>
>> Bjorn
>
> Now that the merge has occurred I find that there is a small bug in
> which a return code with a positive value isn't handled and thus an
> error is generated even though no error occurred.  Please consider this
> patch as a fix.
>
> Thanks,
>
> - Greg
>
> 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>

> ---
>
>   drivers/pci/pci-sysfs.c |    5 +++--
>   1 files changed, 3 insertions(+), 2 deletions(-)
>
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 05b78b1..2722a33 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -461,9 +461,10 @@ static ssize_t sriov_numvfs_store(struct device *dev,
>   					 "Only %d VFs enabled\n",
>   					 num_vfs_enabled);
>   				return count;
> -			} else if (num_vfs_enabled<  0)
> -				/* error code from driver callback */
> +			} else {
> +				/* Return error code or number of VFs */
>   				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",
>
>
> --
> 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

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

[Bjorn Helgaas]

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

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

[Greg Rose]

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

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

[Don Dutile]

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

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

[Bjorn Helgaas]

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

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

[Rose, Gregory V]

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

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

[Bjorn Helgaas]

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

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

[Rose, Gregory V]

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

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

[Don Dutile]

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!

[RFC] SRIOV device enable and disable via sysfs

[Donald Dutile]

Provide files under sysfs to determine the max number of vfs
an SRIOV-capable PCIe device supports, and methods to enable and
disable the vfs on a per device basis.

Currently, VF enablement by SRIOV-capable PCIe devices is done
in driver-specific module parameters.  If not setup in modprobe files,
it requires admin to unload & reload PF drivers with number of desired
VFs to enable.  Additionally, the enablement is system wide: all
devices controlled by the same driver have the same number of VFs
enabled.  Although the latter is probably desired, there are PCI
configurations setup by system BIOS that may not enable that to occur.

Two files are created if a PCIe device has SRIOV support:
sriov_totalvfs -- cat-ing this file returns the maximum number
                  of VFs a PCIe device supports.
sriov_numvfs -- echo'ing a positive number to this file enables 
                & configures this number of VFs for this given PCIe
                device.
	     -- echo'ing 0 to this file disables and deconfigures
                all VFs for this given PCIe device.
             -- cat-ing this file will return the number of VFs
                currently enabled on this PCIe device.

VF enable and disablement is invoked much like other PCIe
configuration functions -- via a registered callback in the driver,
i.e., probe, release, etc.  In this case, sriov_configure

RFC V3->PATCH:
-- incorporate feedback from Ben Hutchings.
-- clean up poor RFC patches & sanitize through checkpatch.pl

RFC v2->v3:
-- change the file names to reflect the names used in the SRIOV spec
-- change to a single file for enable & disable; 
   change driver interface to a single interface.
-- add more informative messages on failures
-- add a core method that a driver can invoke to modify
   the totalvfs reported & supported by a driver.
-- a set of patches for ixgbe provided by Greg Rose to use the
   new interfaces; the last patch modified from the original
   two file, enable/disable interface to the current single file
   enable/disable. Greg will eventually post the final version
   of these patches via Intel's usual process for driver patches.
   Provided here as an example, and enable other SRIOV drivers
   to see how adoption of the interface can be added.

RFC v1->v2:
This patch is based on previous 2 patches by Yinghai Lu
that cleaned up the vga attributes for PCI devices under sysfs,
and uses visibility-checking group attributes as recommended by
Greg K-H.

Signed-off-by: Donald Dutile <ddutile@redhat.com>

[PATCH 3/4] PCI,sys: SRIOV control and status via sysfs

[Donald Dutile]

Provide files under sysfs to determine the max number of vfs
an SRIOV-capable PCIe device supports, and methods to enable and
disable the vfs on a per device basis.

Currently, VF enablement by SRIOV-capable PCIe devices is done
in driver-specific module parameters.  If not setup in modprobe files,
it requires admin to unload & reload PF drivers with number of desired
VFs to enable.  Additionally, the enablement is system wide: all
devices controlled by the same driver have the same number of VFs
enabled.  Although the latter is probably desired, there are PCI
configurations setup by system BIOS that may not enable that to occur.

Three files are created if a PCIe device has SRIOV support:
sriov_totalvfs -- cat-ing this file returns the maximum number
                  of VFs a PCIe device supports as reported by
                  the TotalVFs in the SRIOV ext cap structure.
sriov_numvfs -- echo'ing a positive number to this file enables this
                number of VFs for this given PCIe device.
             -- echo'ing a 0 to this file disables
                any previously enabled VFs for this PCIe device.
             -- cat-ing this file will return the number of VFs
                currently enabled on this PCIe device, i.e.,
                the NumVFs field in SRIOV ext. cap structure.

VF enable and disablement is invoked much like other PCIe
configuration functions -- via registered callbacks in the driver,
i.e., probe, release, etc.

Signed-off-by: Donald Dutile <ddutile@redhat.com>
---
 drivers/pci/pci-sysfs.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/pci.h     |   1 +
 2 files changed, 136 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index fbbb97f..83be8ce 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -14,7 +14,6 @@
  *
  */
 
-
 #include <linux/kernel.h>
 #include <linux/sched.h>
 #include <linux/pci.h>
@@ -404,6 +403,110 @@ static ssize_t d3cold_allowed_show(struct device *dev,
 }
 #endif
 
+#ifdef CONFIG_PCI_IOV
+static ssize_t sriov_totalvfs_show(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct pci_dev *pdev;
+	u16 total;
+
+	pdev = to_pci_dev(dev);
+	total = pdev->sriov->total;
+	return sprintf(buf, "%u\n", total);
+}
+
+
+static ssize_t sriov_numvfs_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct pci_dev *pdev;
+
+	pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pdev->sriov->nr_virtfn);
+}
+
+/*
+ * 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.
+ */
+static ssize_t sriov_numvfs_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct pci_dev *pdev;
+	int num_vfs_enabled = 0;
+	int num_vfs;
+	int ret = 0;
+	u16 total;
+
+	pdev = to_pci_dev(dev);
+
+	if (kstrtoint(buf, 0, &num_vfs) < 0)
+		return -EINVAL;
+
+	/* 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");
+		return -ENOSYS;
+	}
+
+	/* if enabling vf's ... */
+	total = pdev->sriov->total;
+	/* Requested VFs to enable < totalvfs and none enabled already */
+	if ((num_vfs > 0) && (num_vfs <= total)) {
+		if (pdev->sriov->nr_virtfn == 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 == pdev->sriov->nr_virtfn) {
+			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->nr_virtfn, num_vfs);
+			return -EINVAL;
+		}
+	}
+
+	/* disable vfs */
+	if (num_vfs == 0) {
+		if (pdev->sriov->nr_virtfn != 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;
+		}
+	}
+
+	dev_err(&pdev->dev,
+		"Invalid value for number of VFs to enable: %d\n", num_vfs);
+
+	return -EINVAL;
+}
+
+static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
+static struct device_attribute sriov_numvfs_attr =
+		__ATTR(sriov_numvfs, (S_IRUGO|S_IWUSR|S_IWGRP),
+		       sriov_numvfs_show, sriov_numvfs_store);
+#endif /* CONFIG_PCI_IOV */
+
 struct device_attribute pci_dev_attrs[] = {
 	__ATTR_RO(resource),
 	__ATTR_RO(vendor),
@@ -1409,7 +1512,7 @@ static struct attribute *pci_dev_dev_attrs[] = {
 };
 
 static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
-						struct attribute *a, int n)
+					 struct attribute *a, int n)
 {
 	struct device *dev = container_of(kobj, struct device, kobj);
 	struct pci_dev *pdev = to_pci_dev(dev);
@@ -1421,6 +1524,33 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
 	return a->mode;
 }
 
+#ifdef CONFIG_PCI_IOV
+static struct attribute *sriov_dev_attrs[] = {
+	&sriov_totalvfs_attr.attr,
+	&sriov_numvfs_attr.attr,
+	NULL,
+};
+
+static umode_t sriov_attrs_are_visible(struct kobject *kobj,
+					 struct attribute *a, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+
+	if ((a == &sriov_totalvfs_attr.attr) ||
+	    (a == &sriov_numvfs_attr.attr)) {
+		if (!dev_is_pf(dev))
+			return 0;
+	}
+
+	return a->mode;
+}
+
+static struct attribute_group sriov_dev_attr_group = {
+	.attrs = sriov_dev_attrs,
+	.is_visible = sriov_attrs_are_visible,
+};
+#endif /* CONFIG_PCI_IOV */
+
 static struct attribute_group pci_dev_attr_group = {
 	.attrs = pci_dev_dev_attrs,
 	.is_visible = pci_dev_attrs_are_visible,
@@ -1428,6 +1558,9 @@ static struct attribute_group pci_dev_attr_group = {
 
 static const struct attribute_group *pci_dev_attr_groups[] = {
 	&pci_dev_attr_group,
+#ifdef CONFIG_PCI_IOV
+	&sriov_dev_attr_group,
+#endif
 	NULL,
 };
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index ee21795..7ef8fba 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -573,6 +573,7 @@ struct pci_driver {
 	int  (*resume_early) (struct pci_dev *dev);
 	int  (*resume) (struct pci_dev *dev);	                /* Device woken up */
 	void (*shutdown) (struct pci_dev *dev);
+	int (*sriov_configure) (struct pci_dev *dev, int num_vfs); /* PF pdev */
 	const struct pci_error_handlers *err_handler;
 	struct device_driver	driver;
 	struct pci_dynids dynids;
-- 
1.7.10.2.552.gaa3bb87

Re: [PATCH 3/4] PCI,sys: SRIOV control and status via sysfs

[Ben Hutchings]

On Wed, 2012-10-31 at 17:19 -0400, Donald Dutile wrote:
> Provide files under sysfs to determine the max number of vfs
> an SRIOV-capable PCIe device supports, and methods to enable and
> disable the vfs on a per device basis.
> 
> Currently, VF enablement by SRIOV-capable PCIe devices is done
> in driver-specific module parameters.  If not setup in modprobe files,
> it requires admin to unload & reload PF drivers with number of desired
> VFs to enable.  Additionally, the enablement is system wide: all
> devices controlled by the same driver have the same number of VFs
> enabled.  Although the latter is probably desired, there are PCI
> configurations setup by system BIOS that may not enable that to occur.
> 
> Three files are created if a PCIe device has SRIOV support:
> sriov_totalvfs -- cat-ing this file returns the maximum number
>                   of VFs a PCIe device supports as reported by
>                   the TotalVFs in the SRIOV ext cap structure.
> sriov_numvfs -- echo'ing a positive number to this file enables this
>                 number of VFs for this given PCIe device.
>              -- echo'ing a 0 to this file disables
>                 any previously enabled VFs for this PCIe device.
>              -- cat-ing this file will return the number of VFs
>                 currently enabled on this PCIe device, i.e.,
>                 the NumVFs field in SRIOV ext. cap structure.
> 
> VF enable and disablement is invoked much like other PCIe
> configuration functions -- via registered callbacks in the driver,
> i.e., probe, release, etc.
> 
> Signed-off-by: Donald Dutile <ddutile@redhat.com>
> ---
>  drivers/pci/pci-sysfs.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/pci.h     |   1 +
>  2 files changed, 136 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index fbbb97f..83be8ce 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
[...]
> +static ssize_t sriov_numvfs_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	struct pci_dev *pdev;
> +	int num_vfs_enabled = 0;
> +	int num_vfs;
> +	int ret = 0;
> +	u16 total;
> +
> +	pdev = to_pci_dev(dev);
> +
> +	if (kstrtoint(buf, 0, &num_vfs) < 0)
> +		return -EINVAL;
> +
> +	/* 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");
> +		return -ENOSYS;
> +	}
> +
> +	/* if enabling vf's ... */
> +	total = pdev->sriov->total;
> +	/* Requested VFs to enable < totalvfs and none enabled already */
> +	if ((num_vfs > 0) && (num_vfs <= total)) {
> +		if (pdev->sriov->nr_virtfn == 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;

If num_vfs_enabled < 0 then it's an error value which should be returned
instead of count.

[...]
> @@ -1409,7 +1512,7 @@ static struct attribute *pci_dev_dev_attrs[] = {
>  };
>  
>  static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
> -						struct attribute *a, int n)
> +					 struct attribute *a, int n)
>  {
>  	struct device *dev = container_of(kobj, struct device, kobj);
>  	struct pci_dev *pdev = to_pci_dev(dev);

This hunk should be folded into patch 1.

> @@ -1421,6 +1524,33 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
>  	return a->mode;
>  }
>  
> +#ifdef CONFIG_PCI_IOV
> +static struct attribute *sriov_dev_attrs[] = {
> +	&sriov_totalvfs_attr.attr,
> +	&sriov_numvfs_attr.attr,
> +	NULL,
> +};
> +
> +static umode_t sriov_attrs_are_visible(struct kobject *kobj,
> +					 struct attribute *a, int n)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +
> +	if ((a == &sriov_totalvfs_attr.attr) ||
> +	    (a == &sriov_numvfs_attr.attr)) {
> +		if (!dev_is_pf(dev))
> +			return 0;
> +	}

Why do you check the attribute address?  The whole group should be
visible or invisible depending on dev_is_pf().  Any attributes that need
another condition belong in another group.

> +	return a->mode;
> +}
> +
> +static struct attribute_group sriov_dev_attr_group = {
> +	.attrs = sriov_dev_attrs,
> +	.is_visible = sriov_attrs_are_visible,
> +};
> +#endif /* CONFIG_PCI_IOV */
[...]

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 3/4] PCI,sys: SRIOV control and status via sysfs

[Don Dutile]

On 10/31/2012 07:47 PM, Ben Hutchings wrote:
> On Wed, 2012-10-31 at 17:19 -0400, Donald Dutile wrote:
>> Provide files under sysfs to determine the max number of vfs
>> an SRIOV-capable PCIe device supports, and methods to enable and
>> disable the vfs on a per device basis.
>>
>> Currently, VF enablement by SRIOV-capable PCIe devices is done
>> in driver-specific module parameters.  If not setup in modprobe files,
>> it requires admin to unload&  reload PF drivers with number of desired
>> VFs to enable.  Additionally, the enablement is system wide: all
>> devices controlled by the same driver have the same number of VFs
>> enabled.  Although the latter is probably desired, there are PCI
>> configurations setup by system BIOS that may not enable that to occur.
>>
>> Three files are created if a PCIe device has SRIOV support:
>> sriov_totalvfs -- cat-ing this file returns the maximum number
>>                    of VFs a PCIe device supports as reported by
>>                    the TotalVFs in the SRIOV ext cap structure.
>> sriov_numvfs -- echo'ing a positive number to this file enables this
>>                  number of VFs for this given PCIe device.
>>               -- echo'ing a 0 to this file disables
>>                  any previously enabled VFs for this PCIe device.
>>               -- cat-ing this file will return the number of VFs
>>                  currently enabled on this PCIe device, i.e.,
>>                  the NumVFs field in SRIOV ext. cap structure.
>>
>> VF enable and disablement is invoked much like other PCIe
>> configuration functions -- via registered callbacks in the driver,
>> i.e., probe, release, etc.
>>
>> Signed-off-by: Donald Dutile<ddutile@redhat.com>
>> ---
>>   drivers/pci/pci-sysfs.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++-
>>   include/linux/pci.h     |   1 +
>>   2 files changed, 136 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> index fbbb97f..83be8ce 100644
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
> [...]
>> +static ssize_t sriov_numvfs_store(struct device *dev,
>> +				  struct device_attribute *attr,
>> +				  const char *buf, size_t count)
>> +{
>> +	struct pci_dev *pdev;
>> +	int num_vfs_enabled = 0;
>> +	int num_vfs;
>> +	int ret = 0;
>> +	u16 total;
>> +
>> +	pdev = to_pci_dev(dev);
>> +
>> +	if (kstrtoint(buf, 0,&num_vfs)<  0)
>> +		return -EINVAL;
>> +
>> +	/* 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");
>> +		return -ENOSYS;
>> +	}
>> +
>> +	/* if enabling vf's ... */
>> +	total = pdev->sriov->total;
>> +	/* Requested VFs to enable<  totalvfs and none enabled already */
>> +	if ((num_vfs>  0)&&  (num_vfs<= total)) {
>> +		if (pdev->sriov->nr_virtfn == 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;
>
> If num_vfs_enabled<  0 then it's an error value which should be returned
> instead of count.
>
> [...]
agreed. will update.

>> @@ -1409,7 +1512,7 @@ static struct attribute *pci_dev_dev_attrs[] = {
>>   };
>>
>>   static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
>> -						struct attribute *a, int n)
>> +					 struct attribute *a, int n)
>>   {
>>   	struct device *dev = container_of(kobj, struct device, kobj);
>>   	struct pci_dev *pdev = to_pci_dev(dev);
>
> This hunk should be folded into patch 1.
>
i removed it.  will do a cleanup of this file in another patch.

>> @@ -1421,6 +1524,33 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
>>   	return a->mode;
>>   }
>>
>> +#ifdef CONFIG_PCI_IOV
>> +static struct attribute *sriov_dev_attrs[] = {
>> +	&sriov_totalvfs_attr.attr,
>> +	&sriov_numvfs_attr.attr,
>> +	NULL,
>> +};
>> +
>> +static umode_t sriov_attrs_are_visible(struct kobject *kobj,
>> +					 struct attribute *a, int n)
>> +{
>> +	struct device *dev = container_of(kobj, struct device, kobj);
>> +
>> +	if ((a ==&sriov_totalvfs_attr.attr) ||
>> +	    (a ==&sriov_numvfs_attr.attr)) {
>> +		if (!dev_is_pf(dev))
>> +			return 0;
>> +	}
>
> Why do you check the attribute address?  The whole group should be
> visible or invisible depending on dev_is_pf().  Any attributes that need
> another condition belong in another group.
>
agreed.  it's a leftover design when it was part of the uber pci-dev attrs,
but now that it's separate, dev_is_pf() is sufficient.
good cleanup.

>> +	return a->mode;
>> +}
>> +
>> +static struct attribute_group sriov_dev_attr_group = {
>> +	.attrs = sriov_dev_attrs,
>> +	.is_visible = sriov_attrs_are_visible,
>> +};
>> +#endif /* CONFIG_PCI_IOV */
> [...]
>