[RFC] SRIOV device enable and disable via sysfs

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

Authored-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Donald Dutile <ddutile@redhat.com>
---
 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: Don Dutile <ddutile@dddsys1.bos.redhat.com>

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

Authored-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Donald Dutile <ddutile@redhat.com>
---
 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

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

[Yinghai Lu]

On Wed, Oct 31, 2012 at 2:19 PM, Donald Dutile <ddutile@redhat.com> wrote:
> From: Don Dutile <ddutile@dddsys1.bos.redhat.com>

should be
From:  Yinghai Lu <yinghai@kernel.org>

>
> Should make pci_creae_sysfs_dev_files simpler.
> Also fix possible memleak in remove path.
>
> Authored-by: Yinghai Lu <yinghai@kernel.org>

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

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

patch 1 and 2 got acked-by from Greg KH

https://patchwork.kernel.org/patch/1542941/

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

[Don Dutile]

On 10/31/2012 05:53 PM, Yinghai Lu wrote:
> On Wed, Oct 31, 2012 at 2:19 PM, Donald Dutile<ddutile@redhat.com>  wrote:
>> From: Don Dutile<ddutile@dddsys1.bos.redhat.com>
>
> should be
> From:  Yinghai Lu<yinghai@kernel.org>
>
sorry. Tried to credit you for 1st two patches.

>>
>> Should make pci_creae_sysfs_dev_files simpler.
>> Also fix possible memleak in remove path.
>>
>> Authored-by: Yinghai Lu<yinghai@kernel.org>
>
> should be:
> Signed-off-by: Yinghai Lu<yinghai@kernel.org>
>
I didn't want to sign off for you b/c I reworded
the log.  I was trying to credit you for the patch
by stating you Authored it.  My apologies if that was
incorrect.

>> Signed-off-by: Donald Dutile<ddutile@redhat.com>
>
> patch 1 and 2 got acked-by from Greg KH
>
> https://patchwork.kernel.org/patch/1542941/
Yes, but didn't see it in Linus' git tree.
Should I have used one of Bjorn's tree/branch with
this patch in it as master of these patches ?

> --
> 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 2/4] PCI,sys: Use is_visible() with boot_vga attribute for pci_dev

[Yinghai Lu]

On Wed, Oct 31, 2012 at 3:08 PM, Don Dutile <ddutile@redhat.com> wrote:
> On 10/31/2012 05:53 PM, Yinghai Lu wrote:
>>
>> On Wed, Oct 31, 2012 at 2:19 PM, Donald Dutile<ddutile@redhat.com>  wrote:
>>>
>>> From: Don Dutile<ddutile@dddsys1.bos.redhat.com>
>>
>>
>> should be
>> From:  Yinghai Lu<yinghai@kernel.org>
>>
> sorry. Tried to credit you for 1st two patches.
>
>
>>>
>>> Should make pci_creae_sysfs_dev_files simpler.
>>> Also fix possible memleak in remove path.
>>>
>>> Authored-by: Yinghai Lu<yinghai@kernel.org>
>>
>>
>> should be:
>> Signed-off-by: Yinghai Lu<yinghai@kernel.org>
>>
> I didn't want to sign off for you b/c I reworded
> the log.  I was trying to credit you for the patch
> by stating you Authored it.  My apologies if that was
> incorrect.
>
>

if you change log, you still can keep my old Signed-off and add
[ changelog updated.... Donald]
>>> Signed-off-by: Donald Dutile<ddutile@redhat.com>

>>
>>
>> patch 1 and 2 got acked-by from Greg KH
>>
>> https://patchwork.kernel.org/patch/1542941/
>
> Yes, but didn't see it in Linus' git tree.
> Should I have used one of Bjorn's tree/branch with
> this patch in it as master of these patches ?

if bjorn could give Acked-by, those pci change patches go together
with ixgbe changes via Dave's Net-next tree.

or bjorn add one branch in pci next tree to have the first 4 patches,
and let Dave to pull it and use as base for ixgbe change.

or just first 4 patches go into 3.7 directly...

[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 */
> [...]
>

[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       | 27 ++++++++++++++++++++++++++-
 drivers/pci/pci-sysfs.c | 10 ++++++++--
 drivers/pci/pci.h       |  1 +
 include/linux/pci.h     |  5 +++++
 4 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index aeccc91..98c3d37 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -682,7 +682,6 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
 
 	if (!dev->is_physfn)
 		return -ENODEV;
-
 	return sriov_enable(dev, nr_virtfn);
 }
 EXPORT_SYMBOL_GPL(pci_enable_sriov);
@@ -735,3 +734,29 @@ 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, otherwise -EINVAL
+ */
+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 -EIO;
+	else
+		dev->sriov->drvttl = numvfs;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_sriov_set_totalvfs);
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 83be8ce..6333f82 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -412,7 +412,10 @@ static ssize_t sriov_totalvfs_show(struct device *dev,
 	u16 total;
 
 	pdev = to_pci_dev(dev);
-	total = pdev->sriov->total;
+	if (pdev->sriov->drvttl)
+		total = pdev->sriov->drvttl;
+	else
+		total = pdev->sriov->total;
 	return sprintf(buf, "%u\n", total);
 }
 
@@ -458,7 +461,10 @@ static ssize_t sriov_numvfs_store(struct device *dev,
 	}
 
 	/* if enabling vf's ... */
-	total = pdev->sriov->total;
+	if (pdev->sriov->drvttl)
+		total = pdev->sriov->drvttl;
+	else
+		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) {
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..1f58a03 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1611,6 +1611,7 @@ 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);
 #else
 static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
 {
@@ -1627,6 +1628,10 @@ 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;
+}
 #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

[Ben Hutchings]

On Wed, 2012-10-31 at 17:19 -0400, Donald Dutile 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>
> ---
>  drivers/pci/iov.c       | 27 ++++++++++++++++++++++++++-
>  drivers/pci/pci-sysfs.c | 10 ++++++++--
>  drivers/pci/pci.h       |  1 +
>  include/linux/pci.h     |  5 +++++
>  4 files changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index aeccc91..98c3d37 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -682,7 +682,6 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
>  
>  	if (!dev->is_physfn)
>  		return -ENODEV;
> -
>  	return sriov_enable(dev, nr_virtfn);
>  }
>  EXPORT_SYMBOL_GPL(pci_enable_sriov);

This hunk doesn't belong in this patch.

> @@ -735,3 +734,29 @@ 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, otherwise -EINVAL

This says the only possible error is -EINVAL, but it can also return
-EIO!  And the already-enabled case might be one that some drivers will
need to handle specially.

> + */
> +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 -EIO;
[...]

EBUSY seems a bit more appropriate.

Ben.

-- 
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 4/4] PCI,sriov: provide method to reduce the number of total VFs supported

[Don Dutile]

On 10/31/2012 07:53 PM, Ben Hutchings wrote:
> On Wed, 2012-10-31 at 17:19 -0400, Donald Dutile 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>
>> ---
>>   drivers/pci/iov.c       | 27 ++++++++++++++++++++++++++-
>>   drivers/pci/pci-sysfs.c | 10 ++++++++--
>>   drivers/pci/pci.h       |  1 +
>>   include/linux/pci.h     |  5 +++++
>>   4 files changed, 40 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index aeccc91..98c3d37 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -682,7 +682,6 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
>>
>>   	if (!dev->is_physfn)
>>   		return -ENODEV;
>> -
>>   	return sriov_enable(dev, nr_virtfn);
>>   }
>>   EXPORT_SYMBOL_GPL(pci_enable_sriov);
>
> This hunk doesn't belong in this patch.
>
gone in next rev.

>> @@ -735,3 +734,29 @@ 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, otherwise -EINVAL
>
> This says the only possible error is -EINVAL, but it can also return
> -EIO!  And the already-enabled case might be one that some drivers will
> need to handle specially.
>
>> + */
>> +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 -EIO;
> [...]
>
> EBUSY seems a bit more appropriate.
>
> Ben.
>
ok. changed to -EBUSY & comment on return values.

After looking at this area more, I created a pci_sriov_get_totalvfs()
as well, and now use that in the pci-sysfs.c file as well....
much cleaner; symmetric to set.  Makes backporting to
those crazy enterprise releases w/kabi rules easier too! ;-)

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

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

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

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

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] SRIOV device enable and disable via sysfs

[Don Dutile]

Sorry, $Subject should be [PATCH] not [RFC]  ... I don't know why
it prefixed by [RFC] .... I blame it on Hurricane Sandy ! ;-)

On 10/31/2012 05:19 PM, 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.
>
> 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>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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