[RFC] PCI: enable and disable sriov support via sysfs at per device level

[RFC] PCI: enable and disable sriov support via sysfs at per device level

[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_max_vfs -- cat-ing this file returns the maximum number
                 of VFs a PCIe device supports.
sriov_enable_vfs -- echo'ing a number to this file enables this
                    number of VFs for this given PCIe device.
                 -- cat-ing this file will return the number of VFs
                    currently enabled on this PCIe device.
sriov_disable_vfs -- echo-ing any number other than 0 disables all
                     VFs associated with this PCIe device.

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

Note: I haven't had a chance to refactor an SRIOV PF driver
      to test against; hoping to try ixgbe or igb in next couple days.
      To date, just tested that cat-ing sriov_max_vfs works, and
      cat-ing sriov_enable_vfs returns the correct number when
      a PF driver has been loaded with VFs enabled via per-driver param,
      e.g., modprobe igb max_vfs=4

Send comments and I'll integrate as needed, while modifying
a PF driver to use this interface.

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

---
 drivers/pci/pci-sysfs.c | 186 ++++++++++++++++++++++++++++++++++++++++++++----
 include/linux/pci.h     |   2 +
 2 files changed, 175 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 6869009..1b5eab7 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -404,6 +404,152 @@ static ssize_t d3cold_allowed_show(struct device *dev,
 }
 #endif
 
+
+#ifdef CONFIG_PCI_IOV
+static ssize_t sriov_max_vfs_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->total);
+}
+
+bool pci_dev_has_sriov(struct pci_dev *pdev)
+{
+	int ret = false;
+	int pos;
+
+        if (!pci_is_pcie(pdev))
+		goto out;
+
+	pos =  pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_SRIOV);
+	if (pos)
+		ret = true;
+out:
+	return ret;
+}
+
+static ssize_t sriov_enable_vfs_show(struct device *dev,
+				     struct device_attribute *attr,
+				     char *buf)
+{
+	struct pci_dev *pdev;
+	int nr_virtfn = 0;
+
+	pdev = to_pci_dev(dev);
+
+	if (pci_dev_has_sriov(pdev))
+		nr_virtfn = pdev->sriov->nr_virtfn;
+
+	return sprintf (buf, "%u\n", nr_virtfn);
+}
+
+static ssize_t sriov_enable_vfs_store(struct device *dev,
+				      struct device_attribute *attr,
+				      const char *buf, size_t count)
+{
+	struct pci_dev *pdev;
+	int num_vf_enabled = 0;
+	unsigned long num_vfs;
+	pdev = to_pci_dev(dev);
+	
+	if (!pci_dev_has_sriov(pdev))
+		goto out;
+
+	/* Requested VFs to enable < max_vfs 
+	 * and none enabled already
+	 */
+	if (strict_strtoul(buf, 0, &num_vfs) < 0)
+			return -EINVAL;
+
+	if ((num_vfs > pdev->sriov->total) ||
+	    (pdev->sriov->nr_virtfn != 0))
+		return -EINVAL;
+
+	/* Ready for lift-off! */
+	if (pdev->driver && pdev->driver->sriov_enable) {
+		num_vf_enabled = pdev->driver->sriov_enable(pdev, num_vfs);
+	}
+
+out:
+	if (num_vf_enabled != num_vfs)
+		printk(KERN_WARNING 
+			"%s: %04x:%02x:%02x.%d: Only %d VFs enabled \n",
+			pci_name(pdev), pci_domain_nr(pdev->bus),
+			pdev->bus->number, PCI_SLOT(pdev->devfn),
+			PCI_FUNC(pdev->devfn), num_vf_enabled);
+
+	return count;
+}
+
+static ssize_t sriov_disable_vfs_store(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buf, size_t count)
+{
+	struct pci_dev *pdev;
+
+	pdev = to_pci_dev(dev);
+
+	/* make sure sriov device & at least 1 vf enabled */
+	if (!pci_dev_has_sriov(pdev) ||
+	   (pdev->sriov->nr_virtfn == 0))
+		goto out;
+
+	/* Ready for landing! */
+	if (pdev->driver && pdev->driver->sriov_disable) {
+		pdev->driver->sriov_disable(pdev);
+	}
+out:
+	return count;
+}
+
+struct device_attribute pci_dev_sriov_attrs[] = {
+	__ATTR_RO(sriov_max_vfs),
+	__ATTR(sriov_enable_vfs, (S_IRUGO|S_IWUSR|S_IWGRP), 
+		sriov_enable_vfs_show, sriov_enable_vfs_store),
+	__ATTR(sriov_disable_vfs, (S_IWUSR|S_IWGRP), 
+		NULL, sriov_disable_vfs_store),
+	__ATTR_NULL,
+};
+
+static int pci_sriov_create_sysfs_dev_files(struct pci_dev *dev)
+{
+	int pos, i;
+	int retval=0;
+
+	if ((dev->is_physfn) && pci_is_pcie(dev)) {
+            pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_SRIOV);
+            if (pos) {
+		for (i = 0; attr_name(pci_dev_sriov_attrs[i]); i++) {
+		    retval = device_create_file(&dev->dev, &pci_dev_sriov_attrs[i]);
+		    if (retval) {
+			while (--i >= 0)
+			     device_remove_file(&dev->dev, &pci_dev_sriov_attrs[i]);
+			break;
+		    }
+		}
+	    }
+	}
+	return retval;
+}
+
+static void pci_sriov_remove_sysfs_dev_files(struct pci_dev *dev)
+{
+	int pos;
+
+	if ((dev->is_physfn) && pci_is_pcie(dev)) {
+            pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_SRIOV);
+            if (pos)
+		device_remove_file(&dev->dev, pci_dev_sriov_attrs);
+	}
+}
+#else
+static int pci_sriov_create_sysfs_dev_files(struct pci_dev *dev) { return 0; }
+static void pci_sriov_remove_sysfs_dev_files(struct pci_dev *dev) { return; }
+#endif /* CONFIG_PCI_IOV */
+
 struct device_attribute pci_dev_attrs[] = {
 	__ATTR_RO(resource),
 	__ATTR_RO(vendor),
@@ -1169,6 +1315,22 @@ static ssize_t reset_store(struct device *dev,
 
 static struct device_attribute reset_attr = __ATTR(reset, 0200, NULL, reset_store);
 
+static void pci_reset_remove_sysfs_dev_file(struct pci_dev *dev)
+{
+	if (dev->reset_fn) {
+		device_remove_file(&dev->dev, &reset_attr);
+		dev->reset_fn = 0;
+	}
+}
+
+static void pci_vpd_remove_sysfs_dev_file(struct pci_dev *dev)
+{
+	if (dev->vpd && dev->vpd->attr) {
+		sysfs_remove_bin_file(&dev->dev.kobj, dev->vpd->attr);
+		kfree(dev->vpd->attr);
+	}
+}
+
 static int pci_create_capabilities_sysfs(struct pci_dev *dev)
 {
 	int retval;
@@ -1203,14 +1365,18 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
 			goto error;
 		dev->reset_fn = 1;
 	}
+
+	/* SRIOV */
+	retval = pci_sriov_create_sysfs_dev_files(dev);
+	if (retval)
+		goto error;
+
 	return 0;
 
 error:
+	pci_reset_remove_sysfs_dev_file(dev);
 	pcie_aspm_remove_sysfs_dev_files(dev);
-	if (dev->vpd && dev->vpd->attr) {
-		sysfs_remove_bin_file(&dev->dev.kobj, dev->vpd->attr);
-		kfree(dev->vpd->attr);
-	}
+	pci_vpd_remove_sysfs_dev_file(dev);
 
 	return retval;
 }
@@ -1303,16 +1469,10 @@ err:
 
 static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
 {
-	if (dev->vpd && dev->vpd->attr) {
-		sysfs_remove_bin_file(&dev->dev.kobj, dev->vpd->attr);
-		kfree(dev->vpd->attr);
-	}
-
+	pci_sriov_remove_sysfs_dev_files(dev);
+	pci_reset_remove_sysfs_dev_file(dev);
 	pcie_aspm_remove_sysfs_dev_files(dev);
-	if (dev->reset_fn) {
-		device_remove_file(&dev->dev, &reset_attr);
-		dev->reset_fn = 0;
-	}
+	pci_vpd_remove_sysfs_dev_file(dev);
 }
 
 /**
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5faa831..29e10aa 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -596,6 +596,8 @@ 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_enable) (struct pci_dev *dev, int num_vfs);    /* PF pci dev */
+        void (*sriov_disable) (struct pci_dev *dev); 
 	struct pci_error_handlers *err_handler;
 	struct device_driver	driver;
 	struct pci_dynids dynids;
-- 
1.7.10.2.552.gaa3bb87

Re: [RFC] PCI: enable and disable sriov support via sysfs at per device level

[Yuval Mintz]

> +static void pci_sriov_remove_sysfs_dev_files(struct pci_dev *dev)
> +{
> +	int pos;
> +
> +	if ((dev->is_physfn) && pci_is_pcie(dev)) {
> +            pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_SRIOV);
> +            if (pos)
> +		device_remove_file(&dev->dev, pci_dev_sriov_attrs);

Shouldn't it iterate over the attributes when removing them?

Re: [RFC] PCI: enable and disable sriov support via sysfs at per device level

[Don Dutile]

On 10/02/2012 03:32 AM, Yuval Mintz wrote:
>> +static void pci_sriov_remove_sysfs_dev_files(struct pci_dev *dev)
>> +{
>> +	int pos;
>> +
>> +	if ((dev->is_physfn)&&  pci_is_pcie(dev)) {
>> +            pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_SRIOV);
>> +            if (pos)
>> +		device_remove_file(&dev->dev, pci_dev_sriov_attrs);
>
> Shouldn't it iterate over the attributes when removing them?
>
>
>
> --
> 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

good catch; that should be:
  +	int pos, i;
     ....
  +                for (i = 0; attr_name(pci_dev_sriov_attrs[i]); i++)
  +			device_remove_file(&dev->dev, &pci_dev_sriov_attrs);

it doesn't remove the files in reverse order, but neither does
the device-generic code for dev-attrs.


Another question for RFC:
-- above shows I didn't test with hot-plug
-- which makes me wonder/think if [get,put]_device([dev,parent]) needed
    during [create,remove]_file() calls.

Re: [RFC] PCI: enable and disable sriov support via sysfs at per device level

[Yinghai Lu]

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

On Mon, Oct 1, 2012 at 4:27 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_max_vfs -- cat-ing this file returns the maximum number
>                  of VFs a PCIe device supports.
> sriov_enable_vfs -- echo'ing a number to this file enables this
>                     number of VFs for this given PCIe device.
>                  -- cat-ing this file will return the number of VFs
>                     currently enabled on this PCIe device.
> sriov_disable_vfs -- echo-ing any number other than 0 disables all
>                      VFs associated with this PCIe device.
>
> VF enable and disablement is invoked much like other PCIe
> configuration functions -- via registered callbacks in the driver,
> i.e., probe, release, etc.
>
> Note: I haven't had a chance to refactor an SRIOV PF driver
>       to test against; hoping to try ixgbe or igb in next couple days.
>       To date, just tested that cat-ing sriov_max_vfs works, and
>       cat-ing sriov_enable_vfs returns the correct number when
>       a PF driver has been loaded with VFs enabled via per-driver param,
>       e.g., modprobe igb max_vfs=4
>
> Send comments and I'll integrate as needed, while modifying
> a PF driver to use this interface.
>
> Signed-off-by: Donald Dutile <ddutile@redhat.com>
>
> ---
>  drivers/pci/pci-sysfs.c | 186 ++++++++++++++++++++++++++++++++++++++++++++----
>  include/linux/pci.h     |   2 +
>  2 files changed, 175 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 6869009..1b5eab7 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -404,6 +404,152 @@ static ssize_t d3cold_allowed_show(struct device *dev,
>  }
>  #endif
>
> +
> +#ifdef CONFIG_PCI_IOV
> +static ssize_t sriov_max_vfs_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->total);
> +}
> +
> +bool pci_dev_has_sriov(struct pci_dev *pdev)
> +{
> +       int ret = false;
> +       int pos;
> +
> +        if (!pci_is_pcie(pdev))
> +               goto out;
> +
> +       pos =  pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_SRIOV);
> +       if (pos)
> +               ret = true;
> +out:
> +       return ret;
> +}
> +
> +static ssize_t sriov_enable_vfs_show(struct device *dev,
> +                                    struct device_attribute *attr,
> +                                    char *buf)
> +{
> +       struct pci_dev *pdev;
> +       int nr_virtfn = 0;
> +
> +       pdev = to_pci_dev(dev);
> +
> +       if (pci_dev_has_sriov(pdev))
> +               nr_virtfn = pdev->sriov->nr_virtfn;
> +
> +       return sprintf (buf, "%u\n", nr_virtfn);
> +}
> +
> +static ssize_t sriov_enable_vfs_store(struct device *dev,
> +                                     struct device_attribute *attr,
> +                                     const char *buf, size_t count)
> +{
> +       struct pci_dev *pdev;
> +       int num_vf_enabled = 0;
> +       unsigned long num_vfs;
> +       pdev = to_pci_dev(dev);
> +
> +       if (!pci_dev_has_sriov(pdev))
> +               goto out;
> +
> +       /* Requested VFs to enable < max_vfs
> +        * and none enabled already
> +        */
> +       if (strict_strtoul(buf, 0, &num_vfs) < 0)
> +                       return -EINVAL;
> +
> +       if ((num_vfs > pdev->sriov->total) ||
> +           (pdev->sriov->nr_virtfn != 0))
> +               return -EINVAL;
> +
> +       /* Ready for lift-off! */
> +       if (pdev->driver && pdev->driver->sriov_enable) {
> +               num_vf_enabled = pdev->driver->sriov_enable(pdev, num_vfs);
> +       }
> +
> +out:
> +       if (num_vf_enabled != num_vfs)
> +               printk(KERN_WARNING
> +                       "%s: %04x:%02x:%02x.%d: Only %d VFs enabled \n",
> +                       pci_name(pdev), pci_domain_nr(pdev->bus),
> +                       pdev->bus->number, PCI_SLOT(pdev->devfn),
> +                       PCI_FUNC(pdev->devfn), num_vf_enabled);
> +
> +       return count;
> +}
> +
> +static ssize_t sriov_disable_vfs_store(struct device *dev,
> +                                      struct device_attribute *attr,
> +                                      const char *buf, size_t count)
> +{
> +       struct pci_dev *pdev;
> +
> +       pdev = to_pci_dev(dev);
> +
> +       /* make sure sriov device & at least 1 vf enabled */
> +       if (!pci_dev_has_sriov(pdev) ||
> +          (pdev->sriov->nr_virtfn == 0))
> +               goto out;
> +
> +       /* Ready for landing! */
> +       if (pdev->driver && pdev->driver->sriov_disable) {
> +               pdev->driver->sriov_disable(pdev);
> +       }
> +out:
> +       return count;
> +}
> +
> +struct device_attribute pci_dev_sriov_attrs[] = {
> +       __ATTR_RO(sriov_max_vfs),
> +       __ATTR(sriov_enable_vfs, (S_IRUGO|S_IWUSR|S_IWGRP),
> +               sriov_enable_vfs_show, sriov_enable_vfs_store),
> +       __ATTR(sriov_disable_vfs, (S_IWUSR|S_IWGRP),
> +               NULL, sriov_disable_vfs_store),
> +       __ATTR_NULL,
> +};
> +
> +static int pci_sriov_create_sysfs_dev_files(struct pci_dev *dev)
> +{
> +       int pos, i;
> +       int retval=0;
> +
> +       if ((dev->is_physfn) && pci_is_pcie(dev)) {
> +            pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_SRIOV);
> +            if (pos) {
> +               for (i = 0; attr_name(pci_dev_sriov_attrs[i]); i++) {
> +                   retval = device_create_file(&dev->dev, &pci_dev_sriov_attrs[i]);
> +                   if (retval) {
> +                       while (--i >= 0)
> +                            device_remove_file(&dev->dev, &pci_dev_sriov_attrs[i]);
> +                       break;
> +                   }
> +               }
> +           }
> +       }
> +       return retval;

> +}
> +
> +static void pci_sriov_remove_sysfs_dev_files(struct pci_dev *dev)
> +{
> +       int pos;
> +
> +       if ((dev->is_physfn) && pci_is_pcie(dev)) {
> +            pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_SRIOV);
> +            if (pos)
> +               device_remove_file(&dev->dev, pci_dev_sriov_attrs);
> +       }
> +}
...
> @@ -1203,14 +1365,18 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
>                         goto error;
>                 dev->reset_fn = 1;
>         }
> +
> +       /* SRIOV */
> +       retval = pci_sriov_create_sysfs_dev_files(dev);
> +       if (retval)
> +               goto error;
> +

No, You should not use function for that.

According to Greg, you should use group visible to do that.

Please check the patches  that i send out before.

here I attached them again.

Yinghai

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

Subject: [PATCH] PCI: Add pci_dev_type

later use it for visiable attribute control.

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(+)

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
@@ -1409,3 +1409,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,
+};
Index: linux-2.6/drivers/pci/pci.h
===================================================================
--- linux-2.6.orig/drivers/pci/pci.h
+++ linux-2.6/drivers/pci/pci.h
@@ -158,6 +158,7 @@ static inline int pci_no_d1d2(struct pci
 }
 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
Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c
+++ linux-2.6/drivers/pci/probe.c
@@ -1274,6 +1274,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;

[-- Attachment #3: pci_010_pci_dev_boot_vga_x.patch --]
[-- Type: application/octet-stream, Size: 1868 bytes --]

Subject: [PATCH 10/10] PCI, sys: Use is_visable() with boot_vga attribute for pci_dev

That could let pci_create_sysfs_dev_files more simple.

also fix possible fix memleak during removing path.

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

---
 drivers/pci/pci-sysfs.c |   21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

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
@@ -1301,29 +1301,20 @@ int __must_check pci_create_sysfs_dev_fi
 		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);
@@ -1411,12 +1402,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;
 }
 

[-- Attachment #4: pci_drv_set_max_vfs.patch --]
[-- Type: application/octet-stream, Size: 830 bytes --]

---
 include/linux/pci.h |    1 +
 1 file changed, 1 insertion(+)

Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -665,6 +665,7 @@ struct pci_driver {
 	const struct pci_device_id *id_table;	/* must be non-NULL for probe to be called */
 	int  (*probe)  (struct pci_dev *dev, const struct pci_device_id *id);	/* New device inserted */
 	void (*remove) (struct pci_dev *dev);	/* Device removed (NULL if not a hot-plug capable driver) */
+	void (*set_max_vfs) (struct pci_dev *dev); /* enable sriov */
 	int  (*suspend) (struct pci_dev *dev, pm_message_t state);	/* Device suspended */
 	int  (*suspend_late) (struct pci_dev *dev, pm_message_t state);
 	int  (*resume_early) (struct pci_dev *dev);

[-- Attachment #5: sriov_xxx.patch --]
[-- Type: application/octet-stream, Size: 2625 bytes --]

Subject: [PATCH] PCI: Add max_vfs in sysfs per pci device where supports

driver later need to check dev->max_vfs in /sys

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

---
 drivers/pci/pci-sysfs.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h     |    1 
 2 files changed, 52 insertions(+)

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
@@ -456,6 +456,52 @@ boot_vga_show(struct device *dev, struct
 }
 struct device_attribute vga_attr = __ATTR_RO(boot_vga);
 
+static ssize_t
+max_vfs_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pdev->max_vfs);
+}
+
+static void max_vfs_callback(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	mutex_lock(&pci_remove_rescan_mutex);
+	if (pdev->is_added && dev->driver){
+		struct pci_driver *pdrv;
+
+		pdrv = to_pci_driver(dev->driver);
+		if (pdrv->set_max_vfs)
+			pdrv->set_max_vfs(pdev);
+
+	}
+	mutex_unlock(&pci_remove_rescan_mutex);
+}
+static ssize_t
+max_vfs_store(struct device *dev, struct device_attribute *attr,
+		 const char *buf, size_t count)
+{
+	unsigned long val;
+	int err;
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (strict_strtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	pdev->max_vfs = val;
+
+	err = device_schedule_callback(dev, max_vfs_callback);
+
+	if (err)
+		return err;
+
+	return count;
+}
+struct device_attribute max_vfs_attr =
+	__ATTR(max_vfs, 0644, max_vfs_show, max_vfs_store);
+
 static void
 pci_config_pm_runtime_get(struct pci_dev *pdev)
 {
@@ -1403,6 +1449,7 @@ late_initcall(pci_sysfs_init);
 
 static struct attribute *pci_dev_dev_attrs[] = {
 	&vga_attr.attr,
+	&max_vfs_attr.attr,
 	NULL,
 };
 
@@ -1416,6 +1463,10 @@ static umode_t pci_dev_attrs_are_visible
 		if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
 			return 0;
 
+	if (a == &max_vfs_attr.attr)
+		if (!pdev->is_physfn)
+			return 0;
+
 	return a->mode;
 }
 
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -359,6 +359,7 @@ struct pci_dev {
 	unsigned int	broken_intx_masking:1;
 	unsigned int	io_window_1k:1;	/* Intel P2P bridge 1K I/O windows */
 	pci_dev_flags_t dev_flags;
+	unsigned int	max_vfs;
 	atomic_t	enable_cnt;	/* pci_enable_device has been called */
 
 	u32		saved_config_space[16]; /* config space saved at suspend time */

[-- Attachment #6: ixgbe_max_vfs.patch --]
[-- Type: application/octet-stream, Size: 3056 bytes --]

---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |    2 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   44 ++++++++++++++++++++------
 2 files changed, 37 insertions(+), 9 deletions(-)

Index: linux-2.6/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
===================================================================
--- linux-2.6.orig/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ linux-2.6/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -129,13 +129,6 @@ static struct notifier_block dca_notifie
 };
 #endif
 
-#ifdef CONFIG_PCI_IOV
-static unsigned int max_vfs;
-module_param(max_vfs, uint, 0);
-MODULE_PARM_DESC(max_vfs,
-		 "Maximum number of virtual functions to allocate per physical function - default is zero and maximum value is 63");
-#endif /* CONFIG_PCI_IOV */
-
 static unsigned int allow_unsupported_sfp;
 module_param(allow_unsupported_sfp, uint, 0);
 MODULE_PARM_DESC(allow_unsupported_sfp,
@@ -4496,7 +4489,7 @@ static int __devinit ixgbe_sw_init(struc
 #ifdef CONFIG_PCI_IOV
 	/* assign number of SR-IOV VFs */
 	if (hw->mac.type != ixgbe_mac_82598EB)
-		adapter->num_vfs = (max_vfs > 63) ? 0 : max_vfs;
+		adapter->num_vfs = min_t(int, pdev->max_vfs, 63);
 
 #endif
 	/* enable itr by default in dynamic mode */
@@ -7220,8 +7213,9 @@ static int __devinit ixgbe_probe(struct
 
 #ifdef CONFIG_PCI_IOV
 	ixgbe_enable_sriov(adapter, ii);
-
 #endif
+	adapter->ixgbe_info = ii;
+
 	netdev->features = NETIF_F_SG |
 			   NETIF_F_IP_CSUM |
 			   NETIF_F_IPV6_CSUM |
@@ -7683,11 +7677,43 @@ static const struct pci_error_handlers i
 	.resume = ixgbe_io_resume,
 };
 
+static void ixgbe_set_max_vfs(struct pci_dev *pdev)
+{
+#ifdef CONFIG_PCI_IOV
+	struct ixgbe_adapter *adapter = pci_get_drvdata(pdev);
+	struct ixgbe_hw *hw = &adapter->hw;
+	int num_vfs = 0;
+
+	/* assign number of SR-IOV VFs */
+	if (hw->mac.type != ixgbe_mac_82598EB)
+		num_vfs = min_t(int, pdev->max_vfs, 63);
+
+	/* no change */
+	if (adapter->num_vfs == num_vfs)
+		return;
+
+	if (!num_vfs) {
+		/* disable sriov */
+		ixgbe_disable_sriov(adapter);
+		adapter->num_vfs = 0;
+	} else if (!adapter->num_vfs && num_vfs) {
+		/* enable sriov */
+		adapter->num_vfs = num_vfs;
+		ixgbe_enable_sriov(adapter, adapter->ixgbe_info);
+	} else {
+		/* increase or decrease */
+	}
+
+	pdev->max_vfs = adapter->num_vfs;
+#endif
+}
+
 static struct pci_driver ixgbe_driver = {
 	.name     = ixgbe_driver_name,
 	.id_table = ixgbe_pci_tbl,
 	.probe    = ixgbe_probe,
 	.remove   = __devexit_p(ixgbe_remove),
+	.set_max_vfs = ixgbe_set_max_vfs,
 #ifdef CONFIG_PM
 	.suspend  = ixgbe_suspend,
 	.resume   = ixgbe_resume,
Index: linux-2.6/drivers/net/ethernet/intel/ixgbe/ixgbe.h
===================================================================
--- linux-2.6.orig/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ linux-2.6/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -558,6 +558,8 @@ struct ixgbe_adapter {
 	u32 interrupt_event;
 	u32 led_reg;
 
+	struct ixgbe_info *ixgbe_info;
+
 #ifdef CONFIG_IXGBE_PTP
 	struct ptp_clock *ptp_clock;
 	struct ptp_clock_info ptp_caps;

Re: [RFC] PCI: enable and disable sriov support via sysfs at per device level

[Don Dutile]

On 10/02/2012 04:01 PM, Yinghai Lu wrote:
> On Mon, Oct 1, 2012 at 4:27 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_max_vfs -- cat-ing this file returns the maximum number
>>                   of VFs a PCIe device supports.
>> sriov_enable_vfs -- echo'ing a number to this file enables this
>>                      number of VFs for this given PCIe device.
>>                   -- cat-ing this file will return the number of VFs
>>                      currently enabled on this PCIe device.
>> sriov_disable_vfs -- echo-ing any number other than 0 disables all
>>                       VFs associated with this PCIe device.
>>
>> VF enable and disablement is invoked much like other PCIe
>> configuration functions -- via registered callbacks in the driver,
>> i.e., probe, release, etc.
>>
>> Note: I haven't had a chance to refactor an SRIOV PF driver
>>        to test against; hoping to try ixgbe or igb in next couple days.
>>        To date, just tested that cat-ing sriov_max_vfs works, and
>>        cat-ing sriov_enable_vfs returns the correct number when
>>        a PF driver has been loaded with VFs enabled via per-driver param,
>>        e.g., modprobe igb max_vfs=4
>>
>> Send comments and I'll integrate as needed, while modifying
>> a PF driver to use this interface.
>>
>> Signed-off-by: Donald Dutile<ddutile@redhat.com>
>>
>> ---
>>   drivers/pci/pci-sysfs.c | 186 ++++++++++++++++++++++++++++++++++++++++++++----
>>   include/linux/pci.h     |   2 +
>>   2 files changed, 175 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> index 6869009..1b5eab7 100644
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
>> @@ -404,6 +404,152 @@ static ssize_t d3cold_allowed_show(struct device *dev,
>>   }
>>   #endif
>>
>> +
>> +#ifdef CONFIG_PCI_IOV
>> +static ssize_t sriov_max_vfs_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->total);
>> +}
>> +
>> +bool pci_dev_has_sriov(struct pci_dev *pdev)
>> +{
>> +       int ret = false;
>> +       int pos;
>> +
>> +        if (!pci_is_pcie(pdev))
>> +               goto out;
>> +
>> +       pos =  pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_SRIOV);
>> +       if (pos)
>> +               ret = true;
>> +out:
>> +       return ret;
>> +}
>> +
>> +static ssize_t sriov_enable_vfs_show(struct device *dev,
>> +                                    struct device_attribute *attr,
>> +                                    char *buf)
>> +{
>> +       struct pci_dev *pdev;
>> +       int nr_virtfn = 0;
>> +
>> +       pdev = to_pci_dev(dev);
>> +
>> +       if (pci_dev_has_sriov(pdev))
>> +               nr_virtfn = pdev->sriov->nr_virtfn;
>> +
>> +       return sprintf (buf, "%u\n", nr_virtfn);
>> +}
>> +
>> +static ssize_t sriov_enable_vfs_store(struct device *dev,
>> +                                     struct device_attribute *attr,
>> +                                     const char *buf, size_t count)
>> +{
>> +       struct pci_dev *pdev;
>> +       int num_vf_enabled = 0;
>> +       unsigned long num_vfs;
>> +       pdev = to_pci_dev(dev);
>> +
>> +       if (!pci_dev_has_sriov(pdev))
>> +               goto out;
>> +
>> +       /* Requested VFs to enable<  max_vfs
>> +        * and none enabled already
>> +        */
>> +       if (strict_strtoul(buf, 0,&num_vfs)<  0)
>> +                       return -EINVAL;
>> +
>> +       if ((num_vfs>  pdev->sriov->total) ||
>> +           (pdev->sriov->nr_virtfn != 0))
>> +               return -EINVAL;
>> +
>> +       /* Ready for lift-off! */
>> +       if (pdev->driver&&  pdev->driver->sriov_enable) {
>> +               num_vf_enabled = pdev->driver->sriov_enable(pdev, num_vfs);
>> +       }
>> +
>> +out:
>> +       if (num_vf_enabled != num_vfs)
>> +               printk(KERN_WARNING
>> +                       "%s: %04x:%02x:%02x.%d: Only %d VFs enabled \n",
>> +                       pci_name(pdev), pci_domain_nr(pdev->bus),
>> +                       pdev->bus->number, PCI_SLOT(pdev->devfn),
>> +                       PCI_FUNC(pdev->devfn), num_vf_enabled);
>> +
>> +       return count;
>> +}
>> +
>> +static ssize_t sriov_disable_vfs_store(struct device *dev,
>> +                                      struct device_attribute *attr,
>> +                                      const char *buf, size_t count)
>> +{
>> +       struct pci_dev *pdev;
>> +
>> +       pdev = to_pci_dev(dev);
>> +
>> +       /* make sure sriov device&  at least 1 vf enabled */
>> +       if (!pci_dev_has_sriov(pdev) ||
>> +          (pdev->sriov->nr_virtfn == 0))
>> +               goto out;
>> +
>> +       /* Ready for landing! */
>> +       if (pdev->driver&&  pdev->driver->sriov_disable) {
>> +               pdev->driver->sriov_disable(pdev);
>> +       }
>> +out:
>> +       return count;
>> +}
>> +
>> +struct device_attribute pci_dev_sriov_attrs[] = {
>> +       __ATTR_RO(sriov_max_vfs),
>> +       __ATTR(sriov_enable_vfs, (S_IRUGO|S_IWUSR|S_IWGRP),
>> +               sriov_enable_vfs_show, sriov_enable_vfs_store),
>> +       __ATTR(sriov_disable_vfs, (S_IWUSR|S_IWGRP),
>> +               NULL, sriov_disable_vfs_store),
>> +       __ATTR_NULL,
>> +};
>> +
>> +static int pci_sriov_create_sysfs_dev_files(struct pci_dev *dev)
>> +{
>> +       int pos, i;
>> +       int retval=0;
>> +
>> +       if ((dev->is_physfn)&&  pci_is_pcie(dev)) {
>> +            pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_SRIOV);
>> +            if (pos) {
>> +               for (i = 0; attr_name(pci_dev_sriov_attrs[i]); i++) {
>> +                   retval = device_create_file(&dev->dev,&pci_dev_sriov_attrs[i]);
>> +                   if (retval) {
>> +                       while (--i>= 0)
>> +                            device_remove_file(&dev->dev,&pci_dev_sriov_attrs[i]);
>> +                       break;
>> +                   }
>> +               }
>> +           }
>> +       }
>> +       return retval;
>
>> +}
>> +
>> +static void pci_sriov_remove_sysfs_dev_files(struct pci_dev *dev)
>> +{
>> +       int pos;
>> +
>> +       if ((dev->is_physfn)&&  pci_is_pcie(dev)) {
>> +            pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_SRIOV);
>> +            if (pos)
>> +               device_remove_file(&dev->dev, pci_dev_sriov_attrs);
>> +       }
>> +}
> ...
>> @@ -1203,14 +1365,18 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
>>                          goto error;
>>                  dev->reset_fn = 1;
>>          }
>> +
>> +       /* SRIOV */
>> +       retval = pci_sriov_create_sysfs_dev_files(dev);
>> +       if (retval)
>> +               goto error;
>> +
>
> No, You should not use function for that.
>
> According to Greg, you should use group visible to do that.
>
> Please check the patches  that i send out before.
>
> here I attached them again.
>
> Yinghai
Greg: Why not use the above functions?
       and what are 'visible groups' ??
I tried to follow how other optional files were added,
which didn't involve groups or visibility. ... well, ok,
I followed optional symlinks in iov.c... which didn't use
group attributes, and these file seemed no different than other
pcie attributes which is handled in the pci-sysfs.c module
in this fashion.

Yinghai: As for looking at your patches, I did that.... and
they made no sense, i.e., I don't see the value/use/need of
'is_visible'.    maybe if there was some decent
documentation/comments on this stuff, I could
confidently use them.... if they exist, please point me
in the that direction.  if not, then point me to better examples.
Looking in Documentation, I didn't find anything to help.
Doing a grep on is_visible, all I see are flags being
added to subsystem-specific struct's, and the use of device_[create,remove]_files().

Re: [RFC] PCI: enable and disable sriov support via sysfs at per device level

[Yinghai Lu]

On Tue, Oct 2, 2012 at 1:23 PM, Don Dutile <ddutile@redhat.com> wrote:

>
> Yinghai: As for looking at your patches, I did that.... and
> they made no sense, i.e., I don't see the value/use/need of
> 'is_visible'.

sysfs frame work use it during create sysfs files for that group.

> maybe if there was some decent
> documentation/comments on this stuff, I could
> confidently use them.... if they exist, please point me
> in the that direction.  if not, then point me to better examples.
> Looking in Documentation,

sriov_xxx.patch should create one file, you could just expand them to
make three.

Yinghai

Re: [RFC] PCI: enable and disable sriov support via sysfs at per device level

[Greg Kroah-Hartman]

On Tue, Oct 02, 2012 at 04:23:40PM -0400, Don Dutile wrote:
> Greg: Why not use the above functions?

What "above functions"?  You make a lot of calls in the code :)

You are creating an attribute group, and then manually walking through
it to create/destroy the file?  That's not good, use the in-kernel
functions to do that automatically for you.

Also, you need to do this _before_ the device shows up to userspace,
otherwise you have a race that you just lost with udev and the like.

thanks,

greg k-h

Re: [RFC] PCI: enable and disable sriov support via sysfs at per device level

[Don Dutile]

On 10/02/2012 04:39 PM, Greg Kroah-Hartman wrote:
> On Tue, Oct 02, 2012 at 04:23:40PM -0400, Don Dutile wrote:
>> Greg: Why not use the above functions?
>
> What "above functions"?  You make a lot of calls in the code :)
>
> You are creating an attribute group, and then manually walking through
> it to create/destroy the file?  That's not good, use the in-kernel
> functions to do that automatically for you.
>
> Also, you need to do this _before_ the device shows up to userspace,
> otherwise you have a race that you just lost with udev and the like.
>
> thanks,
>
> greg k-h

So, why is it ok to create the 'reset' file after the device shows up?
Like reset, SRIOV is optional.  The sysfs files are designed to only
be used by userspace utils to enable/disable VFs after the system is up.

Or should the design allow a udev rule to be created that if one
of these new sriov files exists, then perform a set of read-max-vf &
enable n-vf ops ?

Re: [RFC] PCI: enable and disable sriov support via sysfs at per device level

[Greg Kroah-Hartman]

On Tue, Oct 02, 2012 at 05:06:34PM -0400, Don Dutile wrote:
> On 10/02/2012 04:39 PM, Greg Kroah-Hartman wrote:
> >On Tue, Oct 02, 2012 at 04:23:40PM -0400, Don Dutile wrote:
> >>Greg: Why not use the above functions?
> >
> >What "above functions"?  You make a lot of calls in the code :)
> >
> >You are creating an attribute group, and then manually walking through
> >it to create/destroy the file?  That's not good, use the in-kernel
> >functions to do that automatically for you.
> >
> >Also, you need to do this _before_ the device shows up to userspace,
> >otherwise you have a race that you just lost with udev and the like.
> >
> >thanks,
> >
> >greg k-h
> 
> So, why is it ok to create the 'reset' file after the device shows up?

It's not, that should be fixed, patches are always welcome.

> Like reset, SRIOV is optional.  The sysfs files are designed to only
> be used by userspace utils to enable/disable VFs after the system is up.
> 
> Or should the design allow a udev rule to be created that if one
> of these new sriov files exists, then perform a set of read-max-vf &
> enable n-vf ops ?

Yes it should.  Never create sysfs files after the device is present in
the system, otherwise userspace never knows about it.

thanks,

greg k-h

Re: [RFC] PCI: enable and disable sriov support via sysfs at per device level

[Yinghai Lu]

On Tue, Oct 2, 2012 at 8:10 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Oct 02, 2012 at 05:06:34PM -0400, Don Dutile wrote:
>>
>> So, why is it ok to create the 'reset' file after the device shows up?
>
> It's not, that should be fixed, patches are always welcome.
>

I tried to move "reset" to device type, but it did not work.

Anyway will send out two patches for dev_type and vga again.

Yinghai

[PATCH 1/2] PCI: Add pci_dev_type

[Yinghai Lu]

need to use it for visiable 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(+)

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
@@ -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,
+};
Index: linux-2.6/drivers/pci/pci.h
===================================================================
--- linux-2.6.orig/drivers/pci/pci.h
+++ linux-2.6/drivers/pci/pci.h
@@ -157,6 +157,7 @@ static inline int pci_no_d1d2(struct pci
 }
 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
Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c
+++ linux-2.6/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;

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

[Yinghai Lu]

That could let pci_create_sysfs_dev_files more simple.

also fix possible fix memleak during removing path.

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

---
 drivers/pci/pci-sysfs.c |   21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

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
@@ -1303,29 +1303,20 @@ int __must_check pci_create_sysfs_dev_fi
 		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;
 }
 

Re: [RFC] PCI: enable and disable sriov support via sysfs at per device level

[Don Dutile]

On 10/02/2012 11:10 PM, Greg Kroah-Hartman wrote:
> On Tue, Oct 02, 2012 at 05:06:34PM -0400, Don Dutile wrote:
>> On 10/02/2012 04:39 PM, Greg Kroah-Hartman wrote:
>>> On Tue, Oct 02, 2012 at 04:23:40PM -0400, Don Dutile wrote:
>>>> Greg: Why not use the above functions?
>>>
>>> What "above functions"?  You make a lot of calls in the code :)
>>>
>>> You are creating an attribute group, and then manually walking through
>>> it to create/destroy the file?  That's not good, use the in-kernel
>>> functions to do that automatically for you.
>>>
>>> Also, you need to do this _before_ the device shows up to userspace,
>>> otherwise you have a race that you just lost with udev and the like.
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>> So, why is it ok to create the 'reset' file after the device shows up?
>
> It's not, that should be fixed, patches are always welcome.
>
>> Like reset, SRIOV is optional.  The sysfs files are designed to only
>> be used by userspace utils to enable/disable VFs after the system is up.
>>
>> Or should the design allow a udev rule to be created that if one
>> of these new sriov files exists, then perform a set of read-max-vf&
>> enable n-vf ops ?
>
> Yes it should.  Never create sysfs files after the device is present in
> the system, otherwise userspace never knows about it.
>
> thanks,
>
> greg k-h

ok, thanks for feedback and bit of education ....
still looking for more on this 'visible' tagging though.

so, I guess Bjorn ought to add 'cleaning up pci-sysfs' to
his PCI to-do list...

maybe once i figure out the proper sriov sysfs code, I
can duplicate that effort in the reset,vga & pm-capabilities
section.

- Don

[PATCH 0/5] PCI: per pci device sysfs set_max_vfs support

[Yinghai Lu]

First two patches are old patches in my queue.
They introduce pci_dev_type and use it with vga in sysfs.

other two will add set_max_vfs in sysfs for device that support siov.

last one is draft version for ixgbe, still need ixgbe guys to sort it
out.

Thanks

Yinghai


Yinghai Lu (5):
  PCI: Add pci_dev_type
  PCI, sys: Use is_visable() with boot_vga attribute for pci_dev
  PCI: add set_max_vfs in pci_driver ops
  PCI: Add max_vfs in sysfs per pci device where supports
  ixgbe: add driver set_max_vfs support

 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |    2 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   44 +++++++++---
 drivers/pci/pci-sysfs.c                       |   96 ++++++++++++++++++++++---
 drivers/pci/pci.h                             |    1 +
 drivers/pci/probe.c                           |    1 +
 include/linux/pci.h                           |    2 +
 6 files changed, 126 insertions(+), 20 deletions(-)

-- 
1.7.7

[PATCH 1/5] PCI: Add pci_dev_type

[Yinghai Lu]

need to use it for visiable 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(+), 0 deletions(-)

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

Re: [PATCH 1/5] PCI: Add pci_dev_type

[Konrad Rzeszutek Wilk]

On Wed, Oct 03, 2012 at 10:51:31AM -0700, Yinghai Lu wrote:
> need to use it for visiable attribute control in syfsfs for pci_dev.

Please use 'ispell' before sending your patches.


Also please explain why do you want this? If I do
'git annotate' on that file (say six months from now when I've forgotten
a lot) and try to lookup what 'is_visible' attribute is, I get
this one line explanation. Worst, the explanation does not
say *why* we need it - or the *purpose* behind it. Or if there
are patches that are going to utilize it.

The Documentation/SubmittingPatches says:

	2) Describe your changes.

	Describe the technical detail of the change(s) your patch includes.

	Be as specific as possible.  The WORST descriptions possible include
	things like "update driver X", "bug fix for driver X", or "this patch
	includes updates for subsystem X.  Please apply."

	The maintainer will thank you if you write your patch description in a
	form which can be easily pulled into Linux's source code management
	system, git, as a "commit log".  See #15, below.

	If your description starts to get long, that's a sign that you probably
	need to split up your patch.  See #3, next.

	When you submit or resubmit a patch or patch series, include the
	complete patch description and justification for it.  Don't just
	say that this is version N of the patch (series).  Don't expect the
	patch merger to refer back to earlier patch versions or referenced
	URLs to find the patch description and put that into the patch.
	I.e., the patch (series) and its description should be self-contained.
	This benefits both the patch merger(s) and reviewers.  Some reviewers
	probably didn't even receive earlier versions of the patch.

	If the patch fixes a logged bug entry, refer to that bug entry by
	number and URL.

> 
> 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(+), 0 deletions(-)
> 
> 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.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

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

[Yinghai Lu]

That could let pci_create_sysfs_dev_files more simple.

also fix possible fix memleak during removing path.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/pci-sysfs.c |   21 ++++++++++-----------
 1 files 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.7

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

[Greg Kroah-Hartman]

On Wed, Oct 03, 2012 at 10:51:32AM -0700, Yinghai Lu wrote:
> That could let pci_create_sysfs_dev_files more simple.
> 
> also fix possible fix memleak during removing path.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>

This, combined with the 1/5 patch, looks great, thanks for doing this.

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH 3/5] PCI: add set_max_vfs in pci_driver ops

[Yinghai Lu]

Will use it enable sriov for pci devices.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 include/linux/pci.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index be1de01..7d70a5e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -590,6 +590,7 @@ struct pci_driver {
 	const struct pci_device_id *id_table;	/* must be non-NULL for probe to be called */
 	int  (*probe)  (struct pci_dev *dev, const struct pci_device_id *id);	/* New device inserted */
 	void (*remove) (struct pci_dev *dev);	/* Device removed (NULL if not a hot-plug capable driver) */
+	void (*set_max_vfs) (struct pci_dev *dev); /* enable sriov */
 	int  (*suspend) (struct pci_dev *dev, pm_message_t state);	/* Device suspended */
 	int  (*suspend_late) (struct pci_dev *dev, pm_message_t state);
 	int  (*resume_early) (struct pci_dev *dev);
-- 
1.7.7

Re: [PATCH 3/5] PCI: add set_max_vfs in pci_driver ops

[Don Dutile]

On 10/03/2012 01:51 PM, Yinghai Lu wrote:
> Will use it enable sriov for pci devices.
>
> Signed-off-by: Yinghai Lu<yinghai@kernel.org>
> ---
>   include/linux/pci.h |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index be1de01..7d70a5e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -590,6 +590,7 @@ struct pci_driver {
>   	const struct pci_device_id *id_table;	/* must be non-NULL for probe to be called */
>   	int  (*probe)  (struct pci_dev *dev, const struct pci_device_id *id);	/* New device inserted */
>   	void (*remove) (struct pci_dev *dev);	/* Device removed (NULL if not a hot-plug capable driver) */
> +	void (*set_max_vfs) (struct pci_dev *dev); /* enable sriov */
>   	int  (*suspend) (struct pci_dev *dev, pm_message_t state);	/* Device suspended */
>   	int  (*suspend_late) (struct pci_dev *dev, pm_message_t state);
>   	int  (*resume_early) (struct pci_dev *dev);

I thought I stated the following in your earlier patch set....

(a) don't use 'set_max_vfs' ;  it is not changing the max; the max
     is whatever the device supports. This kind of terminology confuses
     what is being done, and not descripting what is being done.
(b) this is equiv to the sriov_enable_vfs in the RFC set I sent.
     -- in this set, it prevents the user trying to do more than one enable,
        and that check should be done, and reject the request, which solves one
        of the complaints Alexander had.

I'll try to mind-meld your sysfs attr creation patches to mind later today
and post a new series tonight or tomorrow.  Sorry, stuck in mtgs today (and right now!),
thus the delay.

- Don

Re: [PATCH 3/5] PCI: add set_max_vfs in pci_driver ops

[Yinghai Lu]

On Wed, Oct 3, 2012 at 11:55 AM, Don Dutile <ddutile@redhat.com> wrote:
> On 10/03/2012 01:51 PM, Yinghai Lu wrote:
>>
>> Will use it enable sriov for pci devices.
>>
>> Signed-off-by: Yinghai Lu<yinghai@kernel.org>
>> ---
>>   include/linux/pci.h |    1 +
>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index be1de01..7d70a5e 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -590,6 +590,7 @@ struct pci_driver {
>>         const struct pci_device_id *id_table;   /* must be non-NULL for
>> probe to be called */
>>         int  (*probe)  (struct pci_dev *dev, const struct pci_device_id
>> *id);   /* New device inserted */
>>         void (*remove) (struct pci_dev *dev);   /* Device removed (NULL if
>> not a hot-plug capable driver) */
>> +       void (*set_max_vfs) (struct pci_dev *dev); /* enable sriov */
>>         int  (*suspend) (struct pci_dev *dev, pm_message_t state);      /*
>> Device suspended */
>>         int  (*suspend_late) (struct pci_dev *dev, pm_message_t state);
>>         int  (*resume_early) (struct pci_dev *dev);
>
>
> I thought I stated the following in your earlier patch set....
>
> (a) don't use 'set_max_vfs' ;  it is not changing the max; the max
>     is whatever the device supports. This kind of terminology confuses
>     what is being done, and not descripting what is being done.
> (b) this is equiv to the sriov_enable_vfs in the RFC set I sent.
>     -- in this set, it prevents the user trying to do more than one enable,
>        and that check should be done, and reject the request, which solves
> one
>        of the complaints Alexander had.
>
> I'll try to mind-meld your sysfs attr creation patches to mind later today
> and post a new series tonight or tomorrow.  Sorry, stuck in mtgs today (and
> right now!),
> thus the delay.

Sure. please update 3, 4 as your like, and ask greg.rose work on patch 5.

Thanks

Yinghai

Re: [PATCH 3/5] PCI: add set_max_vfs in pci_driver ops

[Don Dutile]

On 10/03/2012 04:41 PM, Yinghai Lu wrote:
> On Wed, Oct 3, 2012 at 11:55 AM, Don Dutile<ddutile@redhat.com>  wrote:
>> On 10/03/2012 01:51 PM, Yinghai Lu wrote:
>>>
>>> Will use it enable sriov for pci devices.
>>>
>>> Signed-off-by: Yinghai Lu<yinghai@kernel.org>
>>> ---
>>>    include/linux/pci.h |    1 +
>>>    1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>> index be1de01..7d70a5e 100644
>>> --- a/include/linux/pci.h
>>> +++ b/include/linux/pci.h
>>> @@ -590,6 +590,7 @@ struct pci_driver {
>>>          const struct pci_device_id *id_table;   /* must be non-NULL for
>>> probe to be called */
>>>          int  (*probe)  (struct pci_dev *dev, const struct pci_device_id
>>> *id);   /* New device inserted */
>>>          void (*remove) (struct pci_dev *dev);   /* Device removed (NULL if
>>> not a hot-plug capable driver) */
>>> +       void (*set_max_vfs) (struct pci_dev *dev); /* enable sriov */
>>>          int  (*suspend) (struct pci_dev *dev, pm_message_t state);      /*
>>> Device suspended */
>>>          int  (*suspend_late) (struct pci_dev *dev, pm_message_t state);
>>>          int  (*resume_early) (struct pci_dev *dev);
>>
>>
>> I thought I stated the following in your earlier patch set....
>>
>> (a) don't use 'set_max_vfs' ;  it is not changing the max; the max
>>      is whatever the device supports. This kind of terminology confuses
>>      what is being done, and not descripting what is being done.
>> (b) this is equiv to the sriov_enable_vfs in the RFC set I sent.
>>      -- in this set, it prevents the user trying to do more than one enable,
>>         and that check should be done, and reject the request, which solves
>> one
>>         of the complaints Alexander had.
>>
>> I'll try to mind-meld your sysfs attr creation patches to mind later today
>> and post a new series tonight or tomorrow.  Sorry, stuck in mtgs today (and
>> right now!),
>> thus the delay.
>
> Sure. please update 3, 4 as your like, and ask greg.rose work on patch 5.
>
Exactly what I'm doing now.
Thanks for the initial patch split of 1 & 2.
I *think* I understand how the generic framework works for
visible/invisible attributes now works! :)

> Thanks
>
> Yinghai
> --
> 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 4/5] PCI: Add max_vfs in sysfs per pci device where supports

[Yinghai Lu]

only pci device that support sriov will have max_vfs show up in /sys

when user set value in /sys, driver ops set_max_vfs will be called to enable
VF there.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/pci-sysfs.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h     |    1 +
 2 files changed, 52 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index fbbb97f..9b6f409 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -458,6 +458,52 @@ boot_vga_show(struct device *dev, struct device_attribute *attr, char *buf)
 }
 struct device_attribute vga_attr = __ATTR_RO(boot_vga);
 
+static ssize_t
+max_vfs_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pdev->max_vfs);
+}
+
+static void max_vfs_callback(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	mutex_lock(&pci_remove_rescan_mutex);
+	if (pdev->is_added && dev->driver) {
+		struct pci_driver *pdrv;
+
+		pdrv = to_pci_driver(dev->driver);
+		if (pdrv->set_max_vfs)
+			pdrv->set_max_vfs(pdev);
+
+	}
+	mutex_unlock(&pci_remove_rescan_mutex);
+}
+static ssize_t
+max_vfs_store(struct device *dev, struct device_attribute *attr,
+		 const char *buf, size_t count)
+{
+	unsigned long val;
+	int err;
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (kstrtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	pdev->max_vfs = val;
+
+	err = device_schedule_callback(dev, max_vfs_callback);
+
+	if (err)
+		return err;
+
+	return count;
+}
+struct device_attribute max_vfs_attr =
+	__ATTR(max_vfs, 0644, max_vfs_show, max_vfs_store);
+
 static void
 pci_config_pm_runtime_get(struct pci_dev *pdev)
 {
@@ -1405,6 +1451,7 @@ late_initcall(pci_sysfs_init);
 
 static struct attribute *pci_dev_dev_attrs[] = {
 	&vga_attr.attr,
+	&max_vfs_attr.attr,
 	NULL,
 };
 
@@ -1418,6 +1465,10 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
 		if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
 			return 0;
 
+	if (a == &max_vfs_attr.attr)
+		if (!pdev->is_physfn)
+			return 0;
+
 	return a->mode;
 }
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 7d70a5e..f7423d4 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -335,6 +335,7 @@ struct pci_dev {
 	unsigned int	broken_intx_masking:1;
 	unsigned int	io_window_1k:1;	/* Intel P2P bridge 1K I/O windows */
 	pci_dev_flags_t dev_flags;
+	unsigned int	max_vfs;
 	atomic_t	enable_cnt;	/* pci_enable_device has been called */
 
 	u32		saved_config_space[16]; /* config space saved at suspend time */
-- 
1.7.7

Re: [PATCH 4/5] PCI: Add max_vfs in sysfs per pci device where supports

[Konrad Rzeszutek Wilk]

On Wed, Oct 03, 2012 at 10:51:34AM -0700, Yinghai Lu wrote:
> only pci device that support sriov will have max_vfs show up in /sys
  ^-Only   ^-devices           ^-SRIOV

> 
> when user set value in /sys, driver ops set_max_vfs will be called to enable
> VF there.

Huh? What value? What are they enabling? Your comment makes it sound as
if setting any value (say '0xdeadbeef') will be called to enable a VF.

I don't think that is what the code does. Can you explain what the
proper values are to be submitted to the SysFS value 'max_vfs' and
what the kernel ought to be doing? Perhaps include a little snippet
from the kernel log so if somebody has a bug they can look at see
what you got and can compare?

Thank you.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/pci-sysfs.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h     |    1 +
>  2 files changed, 52 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index fbbb97f..9b6f409 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -458,6 +458,52 @@ boot_vga_show(struct device *dev, struct device_attribute *attr, char *buf)
>  }
>  struct device_attribute vga_attr = __ATTR_RO(boot_vga);
>  
> +static ssize_t
> +max_vfs_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	return sprintf(buf, "%u\n", pdev->max_vfs);
> +}
> +
> +static void max_vfs_callback(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	mutex_lock(&pci_remove_rescan_mutex);
> +	if (pdev->is_added && dev->driver) {
> +		struct pci_driver *pdrv;
> +
> +		pdrv = to_pci_driver(dev->driver);
> +		if (pdrv->set_max_vfs)
> +			pdrv->set_max_vfs(pdev);
> +
> +	}
> +	mutex_unlock(&pci_remove_rescan_mutex);
> +}
> +static ssize_t
> +max_vfs_store(struct device *dev, struct device_attribute *attr,
> +		 const char *buf, size_t count)
> +{
> +	unsigned long val;
> +	int err;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	if (kstrtoul(buf, 0, &val) < 0)
> +		return -EINVAL;
> +
> +	pdev->max_vfs = val;
> +
> +	err = device_schedule_callback(dev, max_vfs_callback);
> +
> +	if (err)
> +		return err;
> +
> +	return count;
> +}
> +struct device_attribute max_vfs_attr =
> +	__ATTR(max_vfs, 0644, max_vfs_show, max_vfs_store);
> +
>  static void
>  pci_config_pm_runtime_get(struct pci_dev *pdev)
>  {
> @@ -1405,6 +1451,7 @@ late_initcall(pci_sysfs_init);
>  
>  static struct attribute *pci_dev_dev_attrs[] = {
>  	&vga_attr.attr,
> +	&max_vfs_attr.attr,
>  	NULL,
>  };
>  
> @@ -1418,6 +1465,10 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
>  		if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
>  			return 0;
>  
> +	if (a == &max_vfs_attr.attr)
> +		if (!pdev->is_physfn)
> +			return 0;
> +
>  	return a->mode;
>  }
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 7d70a5e..f7423d4 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -335,6 +335,7 @@ struct pci_dev {
>  	unsigned int	broken_intx_masking:1;
>  	unsigned int	io_window_1k:1;	/* Intel P2P bridge 1K I/O windows */
>  	pci_dev_flags_t dev_flags;
> +	unsigned int	max_vfs;
>  	atomic_t	enable_cnt;	/* pci_enable_device has been called */
>  
>  	u32		saved_config_space[16]; /* config space saved at suspend time */
> -- 
> 1.7.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH 4/5] PCI: Add max_vfs in sysfs per pci device where supports

[Yinghai Lu]

On Thu, Oct 4, 2012 at 7:15 AM, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> On Wed, Oct 03, 2012 at 10:51:34AM -0700, Yinghai Lu wrote:
>> only pci device that support sriov will have max_vfs show up in /sys
>   ^-Only   ^-devices           ^-SRIOV
>
>>
>> when user set value in /sys, driver ops set_max_vfs will be called to enable
>> VF there.
>
> Huh? What value? What are they enabling? Your comment makes it sound as
> if setting any value (say '0xdeadbeef') will be called to enable a VF.
>
> I don't think that is what the code does. Can you explain what the
> proper values are to be submitted to the SysFS value 'max_vfs' and
> what the kernel ought to be doing? Perhaps include a little snippet
> from the kernel log so if somebody has a bug they can look at see
> what you got and can compare?

I sent out this patches  as reference or base for Don and greg.rose
and other sriov guys.

Anyway, thank you for review them.

Yinghai

[PATCH 5/5] ixgbe: add driver set_max_vfs support

[Yinghai Lu]

Need ixgbe guys to close the loop to use set_max_vfs instead
kernel parameters.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
Cc: Greg Rose <gregory.v.rose@intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: John Fastabend <john.r.fastabend@intel.com>
Cc: e1000-devel@lists.sourceforge.net
Cc: netdev@vger.kernel.org
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |    2 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   44 +++++++++++++++++++-----
 2 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index b9623e9..d39d975 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -558,6 +558,8 @@ struct ixgbe_adapter {
 	u32 interrupt_event;
 	u32 led_reg;
 
+	struct ixgbe_info *ixgbe_info;
+
 #ifdef CONFIG_IXGBE_PTP
 	struct ptp_clock *ptp_clock;
 	struct ptp_clock_info ptp_caps;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index ee61819..1c097c7 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -129,13 +129,6 @@ static struct notifier_block dca_notifier = {
 };
 #endif
 
-#ifdef CONFIG_PCI_IOV
-static unsigned int max_vfs;
-module_param(max_vfs, uint, 0);
-MODULE_PARM_DESC(max_vfs,
-		 "Maximum number of virtual functions to allocate per physical function - default is zero and maximum value is 63");
-#endif /* CONFIG_PCI_IOV */
-
 static unsigned int allow_unsupported_sfp;
 module_param(allow_unsupported_sfp, uint, 0);
 MODULE_PARM_DESC(allow_unsupported_sfp,
@@ -4496,7 +4489,7 @@ static int __devinit ixgbe_sw_init(struct ixgbe_adapter *adapter)
 #ifdef CONFIG_PCI_IOV
 	/* assign number of SR-IOV VFs */
 	if (hw->mac.type != ixgbe_mac_82598EB)
-		adapter->num_vfs = (max_vfs > 63) ? 0 : max_vfs;
+		adapter->num_vfs = min_t(int, pdev->max_vfs, 63);
 
 #endif
 	/* enable itr by default in dynamic mode */
@@ -7220,8 +7213,9 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
 
 #ifdef CONFIG_PCI_IOV
 	ixgbe_enable_sriov(adapter, ii);
-
 #endif
+	adapter->ixgbe_info = ii;
+
 	netdev->features = NETIF_F_SG |
 			   NETIF_F_IP_CSUM |
 			   NETIF_F_IPV6_CSUM |
@@ -7683,11 +7677,43 @@ static const struct pci_error_handlers ixgbe_err_handler = {
 	.resume = ixgbe_io_resume,
 };
 
+static void ixgbe_set_max_vfs(struct pci_dev *pdev)
+{
+#ifdef CONFIG_PCI_IOV
+	struct ixgbe_adapter *adapter = pci_get_drvdata(pdev);
+	struct ixgbe_hw *hw = &adapter->hw;
+	int num_vfs = 0;
+
+	/* assign number of SR-IOV VFs */
+	if (hw->mac.type != ixgbe_mac_82598EB)
+		num_vfs = min_t(int, pdev->max_vfs, 63);
+
+	/* no change */
+	if (adapter->num_vfs == num_vfs)
+		return;
+
+	if (!num_vfs) {
+		/* disable sriov */
+		ixgbe_disable_sriov(adapter);
+		adapter->num_vfs = 0;
+	} else if (!adapter->num_vfs && num_vfs) {
+		/* enable sriov */
+		adapter->num_vfs = num_vfs;
+		ixgbe_enable_sriov(adapter, adapter->ixgbe_info);
+	} else {
+		/* increase or decrease */
+	}
+
+	pdev->max_vfs = adapter->num_vfs;
+#endif
+}
+
 static struct pci_driver ixgbe_driver = {
 	.name     = ixgbe_driver_name,
 	.id_table = ixgbe_pci_tbl,
 	.probe    = ixgbe_probe,
 	.remove   = __devexit_p(ixgbe_remove),
+	.set_max_vfs = ixgbe_set_max_vfs,
 #ifdef CONFIG_PM
 	.suspend  = ixgbe_suspend,
 	.resume   = ixgbe_resume,
-- 
1.7.7

Re: [PATCH 5/5] ixgbe: add driver set_max_vfs support

[Yinghai Lu]

On Wed, Oct 3, 2012 at 10:51 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> Need ixgbe guys to close the loop to use set_max_vfs instead
> kernel parameters.

Sorry, I should put RFC in the subject line for this one.

Thanks

Yinghai

Re: [PATCH 5/5] ixgbe: add driver set_max_vfs support

[Dan Carpenter]

On Wed, Oct 03, 2012 at 10:51:35AM -0700, Yinghai Lu wrote:
> Need ixgbe guys to close the loop to use set_max_vfs instead
> kernel parameters.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Cc: Greg Rose <gregory.v.rose@intel.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: John Fastabend <john.r.fastabend@intel.com>
> Cc: e1000-devel@lists.sourceforge.net
> Cc: netdev@vger.kernel.org
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe.h      |    2 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   44 +++++++++++++++++++-----
>  2 files changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> index b9623e9..d39d975 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> @@ -558,6 +558,8 @@ struct ixgbe_adapter {
>  	u32 interrupt_event;
>  	u32 led_reg;
>  
> +	struct ixgbe_info *ixgbe_info;
> +
>  #ifdef CONFIG_IXGBE_PTP
>  	struct ptp_clock *ptp_clock;
>  	struct ptp_clock_info ptp_caps;
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index ee61819..1c097c7 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -129,13 +129,6 @@ static struct notifier_block dca_notifier = {
>  };
>  #endif
>  
> -#ifdef CONFIG_PCI_IOV
> -static unsigned int max_vfs;
> -module_param(max_vfs, uint, 0);
> -MODULE_PARM_DESC(max_vfs,
> -		 "Maximum number of virtual functions to allocate per physical function - default is zero and maximum value is 63");
> -#endif /* CONFIG_PCI_IOV */
> -
>  static unsigned int allow_unsupported_sfp;
>  module_param(allow_unsupported_sfp, uint, 0);
>  MODULE_PARM_DESC(allow_unsupported_sfp,
> @@ -4496,7 +4489,7 @@ static int __devinit ixgbe_sw_init(struct ixgbe_adapter *adapter)
>  #ifdef CONFIG_PCI_IOV
>  	/* assign number of SR-IOV VFs */
>  	if (hw->mac.type != ixgbe_mac_82598EB)
> -		adapter->num_vfs = (max_vfs > 63) ? 0 : max_vfs;
> +		adapter->num_vfs = min_t(int, pdev->max_vfs, 63);

Could we make this min_t(uint, ...);

->max_vfs is type unsigned int. We take an unsigned long from sysfs.
We silently truncate it to an unsigned int.  Then we cast it to a
negative number and compare against 63 and take the minimum...

It's root only so it's not a problem but it's a hassle to audit.

regards,
dan carpenter

Re: [PATCH 5/5] ixgbe: add driver set_max_vfs support

[Alexander Duyck]

On 10/03/2012 10:51 AM, Yinghai Lu wrote:
> Need ixgbe guys to close the loop to use set_max_vfs instead
> kernel parameters.
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Cc: Greg Rose <gregory.v.rose@intel.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: John Fastabend <john.r.fastabend@intel.com>
> Cc: e1000-devel@lists.sourceforge.net
> Cc: netdev@vger.kernel.org
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe.h      |    2 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   44 +++++++++++++++++++-----
>  2 files changed, 37 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> index b9623e9..d39d975 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> @@ -558,6 +558,8 @@ struct ixgbe_adapter {
>  	u32 interrupt_event;
>  	u32 led_reg;
>  
> +	struct ixgbe_info *ixgbe_info;
> +
>  #ifdef CONFIG_IXGBE_PTP
>  	struct ptp_clock *ptp_clock;
>  	struct ptp_clock_info ptp_caps;
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index ee61819..1c097c7 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -129,13 +129,6 @@ static struct notifier_block dca_notifier = {
>  };
>  #endif
>  
> -#ifdef CONFIG_PCI_IOV
> -static unsigned int max_vfs;
> -module_param(max_vfs, uint, 0);
> -MODULE_PARM_DESC(max_vfs,
> -		 "Maximum number of virtual functions to allocate per physical function - default is zero and maximum value is 63");
> -#endif /* CONFIG_PCI_IOV */
> -
>  static unsigned int allow_unsupported_sfp;
>  module_param(allow_unsupported_sfp, uint, 0);
>  MODULE_PARM_DESC(allow_unsupported_sfp,
> @@ -4496,7 +4489,7 @@ static int __devinit ixgbe_sw_init(struct ixgbe_adapter *adapter)
>  #ifdef CONFIG_PCI_IOV
>  	/* assign number of SR-IOV VFs */
>  	if (hw->mac.type != ixgbe_mac_82598EB)
> -		adapter->num_vfs = (max_vfs > 63) ? 0 : max_vfs;
> +		adapter->num_vfs = min_t(int, pdev->max_vfs, 63);
>  
>  #endif
>  	/* enable itr by default in dynamic mode */
> @@ -7220,8 +7213,9 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
>  
>  #ifdef CONFIG_PCI_IOV
>  	ixgbe_enable_sriov(adapter, ii);
> -
>  #endif
> +	adapter->ixgbe_info = ii;
> +
>  	netdev->features = NETIF_F_SG |
>  			   NETIF_F_IP_CSUM |
>  			   NETIF_F_IPV6_CSUM |
> @@ -7683,11 +7677,43 @@ static const struct pci_error_handlers ixgbe_err_handler = {
>  	.resume = ixgbe_io_resume,
>  };
>  
> +static void ixgbe_set_max_vfs(struct pci_dev *pdev)
> +{
> +#ifdef CONFIG_PCI_IOV
> +	struct ixgbe_adapter *adapter = pci_get_drvdata(pdev);
> +	struct ixgbe_hw *hw = &adapter->hw;
> +	int num_vfs = 0;
> +
> +	/* assign number of SR-IOV VFs */
> +	if (hw->mac.type != ixgbe_mac_82598EB)
> +		num_vfs = min_t(int, pdev->max_vfs, 63);
> +
> +	/* no change */
> +	if (adapter->num_vfs == num_vfs)
> +		return;
> +
> +	if (!num_vfs) {
> +		/* disable sriov */
> +		ixgbe_disable_sriov(adapter);
> +		adapter->num_vfs = 0;
> +	} else if (!adapter->num_vfs && num_vfs) {
> +		/* enable sriov */
> +		adapter->num_vfs = num_vfs;
> +		ixgbe_enable_sriov(adapter, adapter->ixgbe_info);
> +	} else {
> +		/* increase or decrease */
> +	}
> +
> +	pdev->max_vfs = adapter->num_vfs;
> +#endif
> +}
> +
>  static struct pci_driver ixgbe_driver = {
>  	.name     = ixgbe_driver_name,
>  	.id_table = ixgbe_pci_tbl,
>  	.probe    = ixgbe_probe,
>  	.remove   = __devexit_p(ixgbe_remove),
> +	.set_max_vfs = ixgbe_set_max_vfs,
>  #ifdef CONFIG_PM
>  	.suspend  = ixgbe_suspend,
>  	.resume   = ixgbe_resume,

The ixgbe_set_max_vfs function has several issues.  The two big ones are
that this function assumes it can just enable/disable SR-IOV without any
other changes being necessary which is not the case.  I would recommend
looking at ixgbe_setup_tc for how to do this properly.  Secondly is the
fact that this code will change the PF network device and as such
sections of the code should be called with the RTNL lock held.  In
addition I believe you have to disable SR-IOV before enabling it again
with a different number of VFs.

Below is a link to one of the early patches for igb when we were first
introducing SR-IOV, and the in-driver sysfs value had been rejected.  I
figure it might be useful as it was also using sysfs to enable/disable
VFs.  It however doesn't have the correct locking on changing the queues
and as such will likely throw an error if you were to implement it the
same way now:
http://lists.openwall.net/netdev/2009/04/08/34

Thanks,

Alex

Re: [PATCH 5/5] ixgbe: add driver set_max_vfs support

[Don Dutile]

On 10/03/2012 02:47 PM, Alexander Duyck wrote:
> On 10/03/2012 10:51 AM, Yinghai Lu wrote:
>> Need ixgbe guys to close the loop to use set_max_vfs instead
>> kernel parameters.
>>
>> Signed-off-by: Yinghai Lu<yinghai@kernel.org>
>> Cc: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
>> Cc: Jesse Brandeburg<jesse.brandeburg@intel.com>
>> Cc: Greg Rose<gregory.v.rose@intel.com>
>> Cc: "David S. Miller"<davem@davemloft.net>
>> Cc: John Fastabend<john.r.fastabend@intel.com>
>> Cc: e1000-devel@lists.sourceforge.net
>> Cc: netdev@vger.kernel.org
>> ---
>>   drivers/net/ethernet/intel/ixgbe/ixgbe.h      |    2 +
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   44 +++++++++++++++++++-----
>>   2 files changed, 37 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> index b9623e9..d39d975 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> @@ -558,6 +558,8 @@ struct ixgbe_adapter {
>>   	u32 interrupt_event;
>>   	u32 led_reg;
>>
>> +	struct ixgbe_info *ixgbe_info;
>> +
>>   #ifdef CONFIG_IXGBE_PTP
>>   	struct ptp_clock *ptp_clock;
>>   	struct ptp_clock_info ptp_caps;
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index ee61819..1c097c7 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -129,13 +129,6 @@ static struct notifier_block dca_notifier = {
>>   };
>>   #endif
>>
>> -#ifdef CONFIG_PCI_IOV
>> -static unsigned int max_vfs;
>> -module_param(max_vfs, uint, 0);
>> -MODULE_PARM_DESC(max_vfs,
>> -		 "Maximum number of virtual functions to allocate per physical function - default is zero and maximum value is 63");
>> -#endif /* CONFIG_PCI_IOV */
>> -
>>   static unsigned int allow_unsupported_sfp;
>>   module_param(allow_unsupported_sfp, uint, 0);
>>   MODULE_PARM_DESC(allow_unsupported_sfp,
>> @@ -4496,7 +4489,7 @@ static int __devinit ixgbe_sw_init(struct ixgbe_adapter *adapter)
>>   #ifdef CONFIG_PCI_IOV
>>   	/* assign number of SR-IOV VFs */
>>   	if (hw->mac.type != ixgbe_mac_82598EB)
>> -		adapter->num_vfs = (max_vfs>  63) ? 0 : max_vfs;
>> +		adapter->num_vfs = min_t(int, pdev->max_vfs, 63);
>>
>>   #endif
>>   	/* enable itr by default in dynamic mode */
>> @@ -7220,8 +7213,9 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
>>
>>   #ifdef CONFIG_PCI_IOV
>>   	ixgbe_enable_sriov(adapter, ii);
>> -
>>   #endif
>> +	adapter->ixgbe_info = ii;
>> +
>>   	netdev->features = NETIF_F_SG |
>>   			   NETIF_F_IP_CSUM |
>>   			   NETIF_F_IPV6_CSUM |
>> @@ -7683,11 +7677,43 @@ static const struct pci_error_handlers ixgbe_err_handler = {
>>   	.resume = ixgbe_io_resume,
>>   };
>>
>> +static void ixgbe_set_max_vfs(struct pci_dev *pdev)
>> +{
>> +#ifdef CONFIG_PCI_IOV
>> +	struct ixgbe_adapter *adapter = pci_get_drvdata(pdev);
>> +	struct ixgbe_hw *hw =&adapter->hw;
>> +	int num_vfs = 0;
>> +
>> +	/* assign number of SR-IOV VFs */
>> +	if (hw->mac.type != ixgbe_mac_82598EB)
>> +		num_vfs = min_t(int, pdev->max_vfs, 63);
>> +
>> +	/* no change */
>> +	if (adapter->num_vfs == num_vfs)
>> +		return;
>> +
>> +	if (!num_vfs) {
>> +		/* disable sriov */
>> +		ixgbe_disable_sriov(adapter);
>> +		adapter->num_vfs = 0;
>> +	} else if (!adapter->num_vfs&&  num_vfs) {
>> +		/* enable sriov */
>> +		adapter->num_vfs = num_vfs;
>> +		ixgbe_enable_sriov(adapter, adapter->ixgbe_info);
>> +	} else {
>> +		/* increase or decrease */
>> +	}
>> +
>> +	pdev->max_vfs = adapter->num_vfs;
>> +#endif
>> +}
>> +
>>   static struct pci_driver ixgbe_driver = {
>>   	.name     = ixgbe_driver_name,
>>   	.id_table = ixgbe_pci_tbl,
>>   	.probe    = ixgbe_probe,
>>   	.remove   = __devexit_p(ixgbe_remove),
>> +	.set_max_vfs = ixgbe_set_max_vfs,
>>   #ifdef CONFIG_PM
>>   	.suspend  = ixgbe_suspend,
>>   	.resume   = ixgbe_resume,
>
> The ixgbe_set_max_vfs function has several issues.  The two big ones are
> that this function assumes it can just enable/disable SR-IOV without any
> other changes being necessary which is not the case.  I would recommend
> looking at ixgbe_setup_tc for how to do this properly.  Secondly is the
> fact that this code will change the PF network device and as such
> sections of the code should be called with the RTNL lock held.  In
> addition I believe you have to disable SR-IOV before enabling it again
> with a different number of VFs.
>
> Below is a link to one of the early patches for igb when we were first
> introducing SR-IOV, and the in-driver sysfs value had been rejected.  I
> figure it might be useful as it was also using sysfs to enable/disable
> VFs.  It however doesn't have the correct locking on changing the queues
> and as such will likely throw an error if you were to implement it the
> same way now:
> http://lists.openwall.net/netdev/2009/04/08/34
>
> Thanks,
>
> Alex

Alex,
Thanks for patch set pointer.
When I started to work on the ixgbe example use based on the RFC set I posted,
I ran into the problem you outlined -- the PF uses/consumes all the queues &
MSI intrs when sriov not enabled at driver load time, which required
more network shutdown logic that I'm not familiar with... So, I was going
to defer to Greg to work that magic. :)
Greg: assume the 2 callback function interface in the RFC patch set I sent,
       (primarily, just the include/linux/pci.h changes), and you can make the
       necessary drivers mods from there.  In the meantime, I'll make the changes
       to my original/v1 RFC to reflect the changes that GKH & Yinghai recommended/implemented
       for sysfs attribute creation & removal in a v2 posting.
       The end result is that the current module parameter setting for max_vfs should
       continue to work, and the sysfs interface will work when those pieces are provided.

-Don

RE: [PATCH 5/5] ixgbe: add driver set_max_vfs support

[Rose, Gregory V]

> -----Original Message-----
> From: Don Dutile [mailto:ddutile@redhat.com]
> Sent: Wednesday, October 03, 2012 12:03 PM
> To: Duyck, Alexander H
> Cc: Yinghai Lu; Bjorn Helgaas; Greg Kroah-Hartman; linux-
> pci@vger.kernel.org; linux-kernel@vger.kernel.org; yuvalmin@broadcom.com;
> bhutchings@solarflare.com; Rose, Gregory V; davem@davemloft.net--no-chain-
> reply-to; Kirsher, Jeffrey T; Brandeburg, Jesse; David S. Miller;
> Fastabend, John R; e1000-devel@lists.sourceforge.net;
> netdev@vger.kernel.org
> Subject: Re: [PATCH 5/5] ixgbe: add driver set_max_vfs support
> 
> On 10/03/2012 02:47 PM, Alexander Duyck wrote:

[snip]

> >
> > The ixgbe_set_max_vfs function has several issues.  The two big ones
> > are that this function assumes it can just enable/disable SR-IOV
> > without any other changes being necessary which is not the case.  I
> > would recommend looking at ixgbe_setup_tc for how to do this properly.
> > Secondly is the fact that this code will change the PF network device
> > and as such sections of the code should be called with the RTNL lock
> > held.  In addition I believe you have to disable SR-IOV before
> > enabling it again with a different number of VFs.
> >
> > Below is a link to one of the early patches for igb when we were first
> > introducing SR-IOV, and the in-driver sysfs value had been rejected.
> > I figure it might be useful as it was also using sysfs to
> > enable/disable VFs.  It however doesn't have the correct locking on
> > changing the queues and as such will likely throw an error if you were
> > to implement it the same way now:
> > http://lists.openwall.net/netdev/2009/04/08/34
> >
> > Thanks,
> >
> > Alex
> 
> Alex,
> Thanks for patch set pointer.
> When I started to work on the ixgbe example use based on the RFC set I
> posted, I ran into the problem you outlined -- the PF uses/consumes all
> the queues & MSI intrs when sriov not enabled at driver load time, which
> required more network shutdown logic that I'm not familiar with... So, I
> was going to defer to Greg to work that magic. :)
> Greg: assume the 2 callback function interface in the RFC patch set I
> sent,
>        (primarily, just the include/linux/pci.h changes), and you can make
> the
>        necessary drivers mods from there.  In the meantime, I'll make the
> changes
>        to my original/v1 RFC to reflect the changes that GKH & Yinghai
> recommended/implemented
>        for sysfs attribute creation & removal in a v2 posting.
>        The end result is that the current module parameter setting for
> max_vfs should
>        continue to work, and the sysfs interface will work when those
> pieces are provided.

OK, I'll start work on it.

Thanks Don,

- Greg

> 
> -Don

Re: [PATCH 5/5] ixgbe: add driver set_max_vfs support

[Yinghai Lu]

On Wed, Oct 3, 2012 at 11:47 AM, Alexander Duyck
<alexander.h.duyck@intel.com> wrote:
> The ixgbe_set_max_vfs function has several issues.  The two big ones are
> that this function assumes it can just enable/disable SR-IOV without any
> other changes being necessary which is not the case.  I would recommend
> looking at ixgbe_setup_tc for how to do this properly.  Secondly is the
> fact that this code will change the PF network device and as such
> sections of the code should be called with the RTNL lock held.  In
> addition I believe you have to disable SR-IOV before enabling it again
> with a different number of VFs.

yes, agreed.

>
> Below is a link to one of the early patches for igb when we were first
> introducing SR-IOV, and the in-driver sysfs value had been rejected.  I
> figure it might be useful as it was also using sysfs to enable/disable
> VFs.  It however doesn't have the correct locking on changing the queues
> and as such will likely throw an error if you were to implement it the
> same way now:
> http://lists.openwall.net/netdev/2009/04/08/34

yes, that is almost there if put that in-driver value to per device
value and ops.

Thanks

Yinghai

Re: [RFC] PCI: enable and disable sriov support via sysfs at per device level

[Yinghai Lu]

On Wed, Oct 3, 2012 at 6:18 AM, Don Dutile <ddutile@redhat.com> wrote:
> maybe once i figure out the proper sriov sysfs code, I
> can duplicate that effort in the reset,vga & pm-capabilities
> section.

I resent my patcheset in plain mail instead of attached form.

I am thinking we should ixgbe guys to sort out ixgbe change to use
set_max_vfs ops.

Thanks

Yinghai

RE: [RFC] PCI: enable and disable sriov support via sysfs at per device level

[Rose, Gregory V]

 -----Original Message-----
> From: yhlu.kernel@gmail.com [mailto:yhlu.kernel@gmail.com] On Behalf Of
> Yinghai Lu
> Sent: Wednesday, October 03, 2012 10:56 AM
> To: Don Dutile
> Cc: Greg Kroah-Hartman; linux-pci@vger.kernel.org; bhelgaas@google.com;
> yuvalmin@broadcom.com; bhutchings@solarflare.com; Rose, Gregory V;
> davem@davemloft.net
> Subject: Re: [RFC] PCI: enable and disable sriov support via sysfs at per
> device level
> 
> On Wed, Oct 3, 2012 at 6:18 AM, Don Dutile <ddutile@redhat.com> wrote:
> > maybe once i figure out the proper sriov sysfs code, I can duplicate
> > that effort in the reset,vga & pm-capabilities section.
> 
> I resent my patcheset in plain mail instead of attached form.
> 
> I am thinking we should ixgbe guys to sort out ixgbe change to use
> set_max_vfs ops.

I'm willing to do that but I'm sort of waiting for things to settle out a bit and see which set of patches everyone settles on before diving into it.

- Greg

> 
> Thanks
> 
> Yinghai

Re: [RFC] PCI: enable and disable sriov support via sysfs at per device level

[Yinghai Lu]

On Wed, Oct 3, 2012 at 11:16 AM, Rose, Gregory V
<gregory.v.rose@intel.com> wrote:
>>
>> I resent my patcheset in plain mail instead of attached form.
>>
>> I am thinking we should ixgbe guys to sort out ixgbe change to use
>> set_max_vfs ops.
>
> I'm willing to do that but I'm sort of waiting for things to settle out a bit and see which set of patches everyone settles on before diving into it.

That would be great !

Actually your changes to ixgbe should be critical changes.

others change to drivers/pci/pci-sysfs.c or adding 3 or 1 methods in driver ops
is not that really matter.

Thanks

Yinghai

Re: [RFC] PCI: enable and disable sriov support via sysfs at per device level

[Konrad Rzeszutek Wilk]

>> Also, you need to do this _before_ the device shows up to userspace,
>> otherwise you have a race that you just lost with udev and the like.
>>
>> thanks,
>>
>> greg k-h
>
>
> So, why is it ok to create the 'reset' file after the device shows up?
> Like reset, SRIOV is optional.  The sysfs files are designed to only
> be used by userspace utils to enable/disable VFs after the system is up.

It should (the attribute) be there when the device is created. Otherwise you end
up with the case when udev could call 'reset' attribute the moment the
SR-IOV device shows up. Or the userspace utility could.

> Or should the design allow a udev rule to be created that if one
> of these new sriov files exists, then perform a set of read-max-vf &
> enable n-vf ops ?

Well, that depends on the ingenuity of the udev rules writers.  But we should
also make sure that the concept is race-free. You don't want the udev
rules to have 'sleep 1' to work-around the kernel code.