* [PATCH 1/8] Yinghai's patch 1 of 2
2012-10-25 18:38 [RFC] SRIOV device enable and disable via sysfs Donald Dutile
@ 2012-10-25 18:38 ` Donald Dutile
2012-10-25 18:38 ` [PATCH 2/8] Yinghai's second patch for vga attr Donald Dutile
` (6 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Donald Dutile @ 2012-10-25 18:38 UTC (permalink / raw)
To: linux-pci
Cc: bhelgaas, yuvalmin, bhutchings, gregory.v.rose, yinghai, davem,
ddutile
---
drivers/pci/pci-sysfs.c | 24 ++++++++++++++++++++++++
drivers/pci/pci.h | 1 +
drivers/pci/probe.c | 1 +
3 files changed, 26 insertions(+)
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 02d107b..3d160aa 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1411,3 +1411,27 @@ static int __init pci_sysfs_init(void)
}
late_initcall(pci_sysfs_init);
+
+static struct attribute *pci_dev_dev_attrs[] = {
+ NULL,
+};
+
+static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
+ struct attribute *a, int n)
+{
+ return a->mode;
+}
+
+static struct attribute_group pci_dev_attr_group = {
+ .attrs = pci_dev_dev_attrs,
+ .is_visible = pci_dev_attrs_are_visible,
+};
+
+static const struct attribute_group *pci_dev_attr_groups[] = {
+ &pci_dev_attr_group,
+ NULL,
+};
+
+struct device_type pci_dev_type = {
+ .groups = pci_dev_attr_groups,
+};
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index bacbcba..6f6cd14 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -157,6 +157,7 @@ static inline int pci_no_d1d2(struct pci_dev *dev)
}
extern struct device_attribute pci_dev_attrs[];
extern struct device_attribute pcibus_dev_attrs[];
+extern struct device_type pci_dev_type;
#ifdef CONFIG_HOTPLUG
extern struct bus_attribute pci_bus_attrs[];
#else
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ec909af..0312f1c48 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -975,6 +975,7 @@ int pci_setup_device(struct pci_dev *dev)
dev->sysdata = dev->bus->sysdata;
dev->dev.parent = dev->bus->bridge;
dev->dev.bus = &pci_bus_type;
+ dev->dev.type = &pci_dev_type;
dev->hdr_type = hdr_type & 0x7f;
dev->multifunction = !!(hdr_type & 0x80);
dev->error_state = pci_channel_io_normal;
--
1.7.10.2.552.gaa3bb87
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 2/8] Yinghai's second patch for vga attr
2012-10-25 18:38 [RFC] SRIOV device enable and disable via sysfs Donald Dutile
2012-10-25 18:38 ` [PATCH 1/8] Yinghai's patch 1 of 2 Donald Dutile
@ 2012-10-25 18:38 ` Donald Dutile
2012-10-25 18:38 ` [PATCH 3/8] PCI: sysfs per device SRIOV control and status Donald Dutile
` (5 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Donald Dutile @ 2012-10-25 18:38 UTC (permalink / raw)
To: linux-pci
Cc: bhelgaas, yuvalmin, bhutchings, gregory.v.rose, yinghai, davem,
ddutile
---
drivers/pci/pci-sysfs.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 3d160aa..fbbb97f 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1303,29 +1303,20 @@ int __must_check pci_create_sysfs_dev_files (struct pci_dev *pdev)
pdev->rom_attr = attr;
}
- if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) {
- retval = device_create_file(&pdev->dev, &vga_attr);
- if (retval)
- goto err_rom_file;
- }
-
/* add platform-specific attributes */
retval = pcibios_add_platform_entries(pdev);
if (retval)
- goto err_vga_file;
+ goto err_rom_file;
/* add sysfs entries for various capabilities */
retval = pci_create_capabilities_sysfs(pdev);
if (retval)
- goto err_vga_file;
+ goto err_rom_file;
pci_create_firmware_label_files(pdev);
return 0;
-err_vga_file:
- if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
- device_remove_file(&pdev->dev, &vga_attr);
err_rom_file:
if (rom_size) {
sysfs_remove_bin_file(&pdev->dev.kobj, pdev->rom_attr);
@@ -1413,12 +1404,20 @@ static int __init pci_sysfs_init(void)
late_initcall(pci_sysfs_init);
static struct attribute *pci_dev_dev_attrs[] = {
+ &vga_attr.attr,
NULL,
};
static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
struct attribute *a, int n)
{
+ struct device *dev = container_of(kobj, struct device, kobj);
+ struct pci_dev *pdev = to_pci_dev(dev);
+
+ if (a == &vga_attr.attr)
+ if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
+ return 0;
+
return a->mode;
}
--
1.7.10.2.552.gaa3bb87
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 3/8] PCI: sysfs per device SRIOV control and status
2012-10-25 18:38 [RFC] SRIOV device enable and disable via sysfs Donald Dutile
2012-10-25 18:38 ` [PATCH 1/8] Yinghai's patch 1 of 2 Donald Dutile
2012-10-25 18:38 ` [PATCH 2/8] Yinghai's second patch for vga attr Donald Dutile
@ 2012-10-25 18:38 ` Donald Dutile
2012-10-25 20:17 ` Ben Hutchings
2012-10-25 18:38 ` [PATCH 4/8] sriov: provide method to reduce the number of total VFs supported Donald Dutile
` (4 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Donald Dutile @ 2012-10-25 18:38 UTC (permalink / raw)
To: linux-pci
Cc: bhelgaas, yuvalmin, bhutchings, gregory.v.rose, yinghai, davem,
ddutile
Provide files under sysfs to determine the max number of vfs
an SRIOV-capable PCIe device supports, and methods to enable and
disable the vfs on a per device basis.
Currently, VF enablement by SRIOV-capable PCIe devices is done
in driver-specific module parameters. If not setup in modprobe files,
it requires admin to unload & reload PF drivers with number of desired
VFs to enable. Additionally, the enablement is system wide: all
devices controlled by the same driver have the same number of VFs
enabled. Although the latter is probably desired, there are PCI
configurations setup by system BIOS that may not enable that to occur.
Three files are created if a PCIe device has SRIOV support:
sriov_totalvfs -- cat-ing this file returns the maximum number
of VFs a PCIe device supports as reported by
the TotalVFs in the SRIOV ext cap structure.
sriov_numvfs -- echo'ing a positive number to this file enables this
number of VFs for this given PCIe device.
-- echo'ing a 0 to this file disables
any previously enabled VFs for this PCIe device.
-- cat-ing this file will return the number of VFs
currently enabled on this PCIe device, i.e.,
the NumVFs field in SRIOV ext. cap structure.
VF enable and disablement is invoked much like other PCIe
configuration functions -- via registered callbacks in the driver,
i.e., probe, release, etc.
Signed-off-by: Donald Dutile <ddutile@redhat.com>
---
drivers/pci/pci-sysfs.c | 155 ++++++++++++++++++++++++++++++++++++++++++++----
include/linux/pci.h | 1 +
2 files changed, 146 insertions(+), 10 deletions(-)
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index fbbb97f..c2894ca 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -14,6 +14,7 @@
*
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt /* has to precede printk.h */
#include <linux/kernel.h>
#include <linux/sched.h>
@@ -66,7 +67,7 @@ static ssize_t broken_parity_status_store(struct device *dev,
struct pci_dev *pdev = to_pci_dev(dev);
unsigned long val;
- if (strict_strtoul(buf, 0, &val) < 0)
+ if (kstrtoul(buf, 0, &val) < 0)
return -EINVAL;
pdev->broken_parity_status = !!val;
@@ -188,7 +189,7 @@ static ssize_t is_enabled_store(struct device *dev,
{
struct pci_dev *pdev = to_pci_dev(dev);
unsigned long val;
- ssize_t result = strict_strtoul(buf, 0, &val);
+ ssize_t result = kstrtoul(buf, 0, &val);
if (result < 0)
return result;
@@ -259,7 +260,7 @@ msi_bus_store(struct device *dev, struct device_attribute *attr,
struct pci_dev *pdev = to_pci_dev(dev);
unsigned long val;
- if (strict_strtoul(buf, 0, &val) < 0)
+ if (kstrtoul(buf, 0, &val) < 0)
return -EINVAL;
/* bad things may happen if the no_msi flag is changed
@@ -292,7 +293,7 @@ static ssize_t bus_rescan_store(struct bus_type *bus, const char *buf,
unsigned long val;
struct pci_bus *b = NULL;
- if (strict_strtoul(buf, 0, &val) < 0)
+ if (kstrtoul(buf, 0, &val) < 0)
return -EINVAL;
if (val) {
@@ -316,7 +317,7 @@ dev_rescan_store(struct device *dev, struct device_attribute *attr,
unsigned long val;
struct pci_dev *pdev = to_pci_dev(dev);
- if (strict_strtoul(buf, 0, &val) < 0)
+ if (kstrtoul(buf, 0, &val) < 0)
return -EINVAL;
if (val) {
@@ -343,7 +344,7 @@ remove_store(struct device *dev, struct device_attribute *dummy,
int ret = 0;
unsigned long val;
- if (strict_strtoul(buf, 0, &val) < 0)
+ if (kstrtoul(buf, 0, &val) < 0)
return -EINVAL;
/* An attribute cannot be unregistered by one of its own methods,
@@ -363,7 +364,7 @@ dev_bus_rescan_store(struct device *dev, struct device_attribute *attr,
unsigned long val;
struct pci_bus *bus = to_pci_bus(dev);
- if (strict_strtoul(buf, 0, &val) < 0)
+ if (kstrtoul(buf, 0, &val) < 0)
return -EINVAL;
if (val) {
@@ -387,7 +388,7 @@ static ssize_t d3cold_allowed_store(struct device *dev,
struct pci_dev *pdev = to_pci_dev(dev);
unsigned long val;
- if (strict_strtoul(buf, 0, &val) < 0)
+ if (kstrtoul(buf, 0, &val) < 0)
return -EINVAL;
pdev->d3cold_allowed = !!val;
@@ -404,6 +405,114 @@ static ssize_t d3cold_allowed_show(struct device *dev,
}
#endif
+#ifdef CONFIG_PCI_IOV
+static ssize_t sriov_totalvfs_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct pci_dev *pdev;
+ u16 total;
+
+ pdev = to_pci_dev(dev);
+ total = pdev->sriov->total;
+ return sprintf (buf, "%u\n", total);
+}
+
+
+static ssize_t sriov_numvfs_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct pci_dev *pdev;
+
+ pdev = to_pci_dev(dev);
+
+ return sprintf (buf, "%u\n", pdev->sriov->nr_virtfn);
+}
+
+/*
+ * num_vfs > 0; number of vfs to enable
+ * num_vfs = 0; disable all vfs
+ *
+ * Note: SRIOV spec doesn't allow partial VF
+ * disable, so its all or none.
+ */
+static ssize_t sriov_numvfs_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct pci_dev *pdev;
+ int num_vfs_enabled = 0;
+ int num_vfs;
+ int ret = 0;
+ u16 total;
+
+ pdev = to_pci_dev(dev);
+
+ /* Requested VFs to enable < totalvfs
+ * and none enabled already
+ */
+ if (kstrtoint(buf, 0, &num_vfs) < 0)
+ return -EINVAL;
+
+ /* is PF driver loaded w/callback */
+ if (!pdev->driver || !pdev->driver->sriov_configure) {
+ pr_info("Driver doesn't support sriov configuration via sysfs \n");
+ return -ENOSYS;
+ }
+
+ /* if enabling vf's ... */
+ total = pdev->sriov->total;
+ if ((num_vfs > 0) && (num_vfs <= total)) {
+ if (pdev->sriov->nr_virtfn == 0) { /* if not already enabled */
+ num_vfs_enabled = pdev->driver->sriov_configure(pdev, num_vfs);
+ if ((num_vfs_enabled >= 0) && (num_vfs_enabled != num_vfs))
+ pr_warn("%s: %04x:%02x:%02x.%d: Only %d VFs enabled \n",
+ pdev->dev.driver->name, pci_domain_nr(pdev->bus),
+ pdev->bus->number, PCI_SLOT(pdev->devfn),
+ PCI_FUNC(pdev->devfn), num_vfs_enabled);
+ return count;
+ } else {
+ pr_warn("VFs already enabled. Disable before re-enabling\n");
+ return -EINVAL;
+ }
+ }
+
+ /* disable vfs */
+ if (num_vfs == 0) {
+ if (pdev->sriov->nr_virtfn != 0) {
+ ret = pdev->driver->sriov_configure(pdev, 0);
+ return ret ? ret: count;
+ } else {
+ pr_err("Disable failed since no VFs enabled\n");
+ return -EINVAL;
+ }
+ }
+
+ pr_err("Invalid value for number of VFs to enable: %d\n", num_vfs);
+
+ return -EINVAL;
+}
+#else
+static ssize_t sriov_totalvfs_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{ return 0; }
+static ssize_t sriov_numvfs_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{ return 0; }
+static ssize_t sriov_numvfs_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{ return count; }
+#endif /* CONFIG_PCI_IOV */
+
+static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
+static struct device_attribute sriov_numvfs_attr =
+ __ATTR(sriov_numvfs, (S_IRUGO|S_IWUSR|S_IWGRP),
+ sriov_numvfs_show, sriov_numvfs_store);
+
struct device_attribute pci_dev_attrs[] = {
__ATTR_RO(resource),
__ATTR_RO(vendor),
@@ -1194,7 +1303,7 @@ static ssize_t reset_store(struct device *dev,
{
struct pci_dev *pdev = to_pci_dev(dev);
unsigned long val;
- ssize_t result = strict_strtoul(buf, 0, &val);
+ ssize_t result = kstrtoul(buf, 0, &val);
if (result < 0)
return result;
@@ -1408,8 +1517,14 @@ static struct attribute *pci_dev_dev_attrs[] = {
NULL,
};
+static struct attribute *sriov_dev_attrs[] = {
+ &sriov_totalvfs_attr.attr,
+ &sriov_numvfs_attr.attr,
+ NULL,
+};
+
static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
- struct attribute *a, int n)
+ struct attribute *a, int n)
{
struct device *dev = container_of(kobj, struct device, kobj);
struct pci_dev *pdev = to_pci_dev(dev);
@@ -1421,13 +1536,33 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
return a->mode;
}
+static umode_t sriov_attrs_are_visible(struct kobject *kobj,
+ struct attribute *a, int n)
+{
+ struct device *dev = container_of(kobj, struct device, kobj);
+
+ if ((a == &sriov_totalvfs_attr.attr) ||
+ (a == &sriov_numvfs_attr.attr)) {
+ if (!dev_is_pf(dev))
+ return 0;
+ }
+
+ return a->mode;
+}
+
static struct attribute_group pci_dev_attr_group = {
.attrs = pci_dev_dev_attrs,
.is_visible = pci_dev_attrs_are_visible,
};
+static struct attribute_group sriov_dev_attr_group = {
+ .attrs = sriov_dev_attrs,
+ .is_visible = sriov_attrs_are_visible,
+};
+
static const struct attribute_group *pci_dev_attr_groups[] = {
&pci_dev_attr_group,
+ &sriov_dev_attr_group,
NULL,
};
diff --git a/include/linux/pci.h b/include/linux/pci.h
index be1de01..1d60a23 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -595,6 +595,7 @@ struct pci_driver {
int (*resume_early) (struct pci_dev *dev);
int (*resume) (struct pci_dev *dev); /* Device woken up */
void (*shutdown) (struct pci_dev *dev);
+ int (*sriov_configure) (struct pci_dev *dev, int num_vfs); /* PF pci dev */
const struct pci_error_handlers *err_handler;
struct device_driver driver;
struct pci_dynids dynids;
--
1.7.10.2.552.gaa3bb87
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 3/8] PCI: sysfs per device SRIOV control and status
2012-10-25 18:38 ` [PATCH 3/8] PCI: sysfs per device SRIOV control and status Donald Dutile
@ 2012-10-25 20:17 ` Ben Hutchings
2012-10-26 15:07 ` Don Dutile
0 siblings, 1 reply; 19+ messages in thread
From: Ben Hutchings @ 2012-10-25 20:17 UTC (permalink / raw)
To: Donald Dutile
Cc: linux-pci, bhelgaas, yuvalmin, gregory.v.rose, yinghai, davem
On Thu, 2012-10-25 at 14:38 -0400, Donald Dutile wrote:
[...]
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -14,6 +14,7 @@
> *
> */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt /* has to precede printk.h */
>
> #include <linux/kernel.h>
> #include <linux/sched.h>
> @@ -66,7 +67,7 @@ static ssize_t broken_parity_status_store(struct device *dev,
> struct pci_dev *pdev = to_pci_dev(dev);
> unsigned long val;
>
> - if (strict_strtoul(buf, 0, &val) < 0)
> + if (kstrtoul(buf, 0, &val) < 0)
> return -EINVAL;
>
> pdev->broken_parity_status = !!val;
> @@ -188,7 +189,7 @@ static ssize_t is_enabled_store(struct device *dev,
> {
> struct pci_dev *pdev = to_pci_dev(dev);
> unsigned long val;
> - ssize_t result = strict_strtoul(buf, 0, &val);
> + ssize_t result = kstrtoul(buf, 0, &val);
>
> if (result < 0)
> return result;
> @@ -259,7 +260,7 @@ msi_bus_store(struct device *dev, struct device_attribute *attr,
> struct pci_dev *pdev = to_pci_dev(dev);
> unsigned long val;
>
> - if (strict_strtoul(buf, 0, &val) < 0)
> + if (kstrtoul(buf, 0, &val) < 0)
> return -EINVAL;
>
> /* bad things may happen if the no_msi flag is changed
> @@ -292,7 +293,7 @@ static ssize_t bus_rescan_store(struct bus_type *bus, const char *buf,
> unsigned long val;
> struct pci_bus *b = NULL;
>
> - if (strict_strtoul(buf, 0, &val) < 0)
> + if (kstrtoul(buf, 0, &val) < 0)
> return -EINVAL;
>
> if (val) {
> @@ -316,7 +317,7 @@ dev_rescan_store(struct device *dev, struct device_attribute *attr,
> unsigned long val;
> struct pci_dev *pdev = to_pci_dev(dev);
>
> - if (strict_strtoul(buf, 0, &val) < 0)
> + if (kstrtoul(buf, 0, &val) < 0)
> return -EINVAL;
>
> if (val) {
> @@ -343,7 +344,7 @@ remove_store(struct device *dev, struct device_attribute *dummy,
> int ret = 0;
> unsigned long val;
>
> - if (strict_strtoul(buf, 0, &val) < 0)
> + if (kstrtoul(buf, 0, &val) < 0)
> return -EINVAL;
>
> /* An attribute cannot be unregistered by one of its own methods,
> @@ -363,7 +364,7 @@ dev_bus_rescan_store(struct device *dev, struct device_attribute *attr,
> unsigned long val;
> struct pci_bus *bus = to_pci_bus(dev);
>
> - if (strict_strtoul(buf, 0, &val) < 0)
> + if (kstrtoul(buf, 0, &val) < 0)
> return -EINVAL;
>
> if (val) {
> @@ -387,7 +388,7 @@ static ssize_t d3cold_allowed_store(struct device *dev,
> struct pci_dev *pdev = to_pci_dev(dev);
> unsigned long val;
>
> - if (strict_strtoul(buf, 0, &val) < 0)
> + if (kstrtoul(buf, 0, &val) < 0)
> return -EINVAL;
>
> pdev->d3cold_allowed = !!val;
All this cleanup belongs in a separate patch.
> @@ -404,6 +405,114 @@ static ssize_t d3cold_allowed_show(struct device *dev,
> }
> #endif
>
> +#ifdef CONFIG_PCI_IOV
> +static ssize_t sriov_totalvfs_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct pci_dev *pdev;
> + u16 total;
> +
> + pdev = to_pci_dev(dev);
> + total = pdev->sriov->total;
> + return sprintf (buf, "%u\n", total);
No space after sprintf, please.
> +}
> +
> +
> +static ssize_t sriov_numvfs_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct pci_dev *pdev;
> +
> + pdev = to_pci_dev(dev);
> +
> + return sprintf (buf, "%u\n", pdev->sriov->nr_virtfn);
> +}
> +
> +/*
> + * num_vfs > 0; number of vfs to enable
> + * num_vfs = 0; disable all vfs
> + *
> + * Note: SRIOV spec doesn't allow partial VF
> + * disable, so its all or none.
> + */
> +static ssize_t sriov_numvfs_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct pci_dev *pdev;
> + int num_vfs_enabled = 0;
> + int num_vfs;
> + int ret = 0;
> + u16 total;
> +
> + pdev = to_pci_dev(dev);
> +
> + /* Requested VFs to enable < totalvfs
> + * and none enabled already
> + */
> + if (kstrtoint(buf, 0, &num_vfs) < 0)
> + return -EINVAL;
The above comment belongs with
'if ((num_vfs > 0) && (num_vfs <= total))'.
> + /* is PF driver loaded w/callback */
> + if (!pdev->driver || !pdev->driver->sriov_configure) {
> + pr_info("Driver doesn't support sriov configuration via sysfs \n");
> + return -ENOSYS;
Use dev_err() to set the proper severity and provide context. Not sure
whether this is the right error code.
> + }
> +
> + /* if enabling vf's ... */
> + total = pdev->sriov->total;
> + if ((num_vfs > 0) && (num_vfs <= total)) {
> + if (pdev->sriov->nr_virtfn == 0) { /* if not already enabled */
> + num_vfs_enabled = pdev->driver->sriov_configure(pdev, num_vfs);
> + if ((num_vfs_enabled >= 0) && (num_vfs_enabled != num_vfs))
> + pr_warn("%s: %04x:%02x:%02x.%d: Only %d VFs enabled \n",
> + pdev->dev.driver->name, pci_domain_nr(pdev->bus),
> + pdev->bus->number, PCI_SLOT(pdev->devfn),
> + PCI_FUNC(pdev->devfn), num_vfs_enabled);
Use dev_warn(), don't try to replicate it.
> + return count;
> + } else {
> + pr_warn("VFs already enabled. Disable before re-enabling\n");
> + return -EINVAL;
It would be helpful to make an 'enable' write succeed when
num_vfs == pdev->sriov->nr_virtfn.
> + }
> + }
> +
> + /* disable vfs */
> + if (num_vfs == 0) {
> + if (pdev->sriov->nr_virtfn != 0) {
> + ret = pdev->driver->sriov_configure(pdev, 0);
> + return ret ? ret: count;
> + } else {
> + pr_err("Disable failed since no VFs enabled\n");
> + return -EINVAL;
This definitely should be treated as successful, not an error.
> + }
> + }
> +
> + pr_err("Invalid value for number of VFs to enable: %d\n", num_vfs);
> +
> + return -EINVAL;
> +}
> +#else
> +static ssize_t sriov_totalvfs_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{ return 0; }
> +static ssize_t sriov_numvfs_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{ return 0; }
Shouldn't these print "0\n"?
> +static ssize_t sriov_numvfs_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{ return count; }
> +#endif /* CONFIG_PCI_IOV */
> +
> +static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
> +static struct device_attribute sriov_numvfs_attr =
> + __ATTR(sriov_numvfs, (S_IRUGO|S_IWUSR|S_IWGRP),
> + sriov_numvfs_show, sriov_numvfs_store);
sriov_numvfs should be read-only if !CONFIG_PCI_IOV, rather than
ignoring writes.
> struct device_attribute pci_dev_attrs[] = {
> __ATTR_RO(resource),
> __ATTR_RO(vendor),
> @@ -1194,7 +1303,7 @@ static ssize_t reset_store(struct device *dev,
> {
> struct pci_dev *pdev = to_pci_dev(dev);
> unsigned long val;
> - ssize_t result = strict_strtoul(buf, 0, &val);
> + ssize_t result = kstrtoul(buf, 0, &val);
>
> if (result < 0)
> return result;
> @@ -1408,8 +1517,14 @@ static struct attribute *pci_dev_dev_attrs[] = {
> NULL,
> };
>
> +static struct attribute *sriov_dev_attrs[] = {
> + &sriov_totalvfs_attr.attr,
> + &sriov_numvfs_attr.attr,
These are wrongly indented with spaces.
> + NULL,
> +};
> +
> static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
> - struct attribute *a, int n)
> + struct attribute *a, int n)
> {
> struct device *dev = container_of(kobj, struct device, kobj);
> struct pci_dev *pdev = to_pci_dev(dev);
> @@ -1421,13 +1536,33 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
> return a->mode;
> }
>
> +static umode_t sriov_attrs_are_visible(struct kobject *kobj,
> + struct attribute *a, int n)
> +{
> + struct device *dev = container_of(kobj, struct device, kobj);
> +
> + if ((a == &sriov_totalvfs_attr.attr) ||
> + (a == &sriov_numvfs_attr.attr)) {
> + if (!dev_is_pf(dev))
> + return 0;
[...]
An odd thing about dev_is_pf() is that if !CONFIG_PCI_IOV then it is
false for all devices. Which means that the dummy attribute reader
functions will actually be dead code. I think that either dev_is_pf()
should be fixed to be accurate when !CONFIG_PCI_IOV, or the attributes
should be completely removed if !CONFIG_PCI_IOV.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/8] PCI: sysfs per device SRIOV control and status
2012-10-25 20:17 ` Ben Hutchings
@ 2012-10-26 15:07 ` Don Dutile
2012-10-31 17:01 ` Rose, Gregory V
0 siblings, 1 reply; 19+ messages in thread
From: Don Dutile @ 2012-10-26 15:07 UTC (permalink / raw)
To: Ben Hutchings
Cc: linux-pci, bhelgaas, yuvalmin, gregory.v.rose, yinghai, davem
On 10/25/2012 04:17 PM, Ben Hutchings wrote:
> On Thu, 2012-10-25 at 14:38 -0400, Donald Dutile wrote:
> [...]
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
>> @@ -14,6 +14,7 @@
>> *
>> */
>>
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt /* has to precede printk.h */
>>
>> #include<linux/kernel.h>
>> #include<linux/sched.h>
>> @@ -66,7 +67,7 @@ static ssize_t broken_parity_status_store(struct device *dev,
>> struct pci_dev *pdev = to_pci_dev(dev);
>> unsigned long val;
>>
>> - if (strict_strtoul(buf, 0,&val)< 0)
>> + if (kstrtoul(buf, 0,&val)< 0)
>> return -EINVAL;
>>
>> pdev->broken_parity_status = !!val;
>> @@ -188,7 +189,7 @@ static ssize_t is_enabled_store(struct device *dev,
>> {
>> struct pci_dev *pdev = to_pci_dev(dev);
>> unsigned long val;
>> - ssize_t result = strict_strtoul(buf, 0,&val);
>> + ssize_t result = kstrtoul(buf, 0,&val);
>>
>> if (result< 0)
>> return result;
>> @@ -259,7 +260,7 @@ msi_bus_store(struct device *dev, struct device_attribute *attr,
>> struct pci_dev *pdev = to_pci_dev(dev);
>> unsigned long val;
>>
>> - if (strict_strtoul(buf, 0,&val)< 0)
>> + if (kstrtoul(buf, 0,&val)< 0)
>> return -EINVAL;
>>
>> /* bad things may happen if the no_msi flag is changed
>> @@ -292,7 +293,7 @@ static ssize_t bus_rescan_store(struct bus_type *bus, const char *buf,
>> unsigned long val;
>> struct pci_bus *b = NULL;
>>
>> - if (strict_strtoul(buf, 0,&val)< 0)
>> + if (kstrtoul(buf, 0,&val)< 0)
>> return -EINVAL;
>>
>> if (val) {
>> @@ -316,7 +317,7 @@ dev_rescan_store(struct device *dev, struct device_attribute *attr,
>> unsigned long val;
>> struct pci_dev *pdev = to_pci_dev(dev);
>>
>> - if (strict_strtoul(buf, 0,&val)< 0)
>> + if (kstrtoul(buf, 0,&val)< 0)
>> return -EINVAL;
>>
>> if (val) {
>> @@ -343,7 +344,7 @@ remove_store(struct device *dev, struct device_attribute *dummy,
>> int ret = 0;
>> unsigned long val;
>>
>> - if (strict_strtoul(buf, 0,&val)< 0)
>> + if (kstrtoul(buf, 0,&val)< 0)
>> return -EINVAL;
>>
>> /* An attribute cannot be unregistered by one of its own methods,
>> @@ -363,7 +364,7 @@ dev_bus_rescan_store(struct device *dev, struct device_attribute *attr,
>> unsigned long val;
>> struct pci_bus *bus = to_pci_bus(dev);
>>
>> - if (strict_strtoul(buf, 0,&val)< 0)
>> + if (kstrtoul(buf, 0,&val)< 0)
>> return -EINVAL;
>>
>> if (val) {
>> @@ -387,7 +388,7 @@ static ssize_t d3cold_allowed_store(struct device *dev,
>> struct pci_dev *pdev = to_pci_dev(dev);
>> unsigned long val;
>>
>> - if (strict_strtoul(buf, 0,&val)< 0)
>> + if (kstrtoul(buf, 0,&val)< 0)
>> return -EINVAL;
>>
>> pdev->d3cold_allowed = !!val;
>
> All this cleanup belongs in a separate patch.
Yes. Multiple people were getting anxious to see this V3, so I wanted
to get it out for review of its direction.
I did a checkpatch.pl run after I posted... ugh! what a mess!
you note a number of the issues below.
>
>> @@ -404,6 +405,114 @@ static ssize_t d3cold_allowed_show(struct device *dev,
>> }
>> #endif
>>
>> +#ifdef CONFIG_PCI_IOV
>> +static ssize_t sriov_totalvfs_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct pci_dev *pdev;
>> + u16 total;
>> +
>> + pdev = to_pci_dev(dev);
>> + total = pdev->sriov->total;
>> + return sprintf (buf, "%u\n", total);
>
> No space after sprintf, please.
>
yep, checkpatch...
>> +}
>> +
>> +
>> +static ssize_t sriov_numvfs_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct pci_dev *pdev;
>> +
>> + pdev = to_pci_dev(dev);
>> +
>> + return sprintf (buf, "%u\n", pdev->sriov->nr_virtfn);
>> +}
>> +
>> +/*
>> + * num_vfs> 0; number of vfs to enable
>> + * num_vfs = 0; disable all vfs
>> + *
>> + * Note: SRIOV spec doesn't allow partial VF
>> + * disable, so its all or none.
>> + */
>> +static ssize_t sriov_numvfs_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct pci_dev *pdev;
>> + int num_vfs_enabled = 0;
>> + int num_vfs;
>> + int ret = 0;
>> + u16 total;
>> +
>> + pdev = to_pci_dev(dev);
>> +
>> + /* Requested VFs to enable< totalvfs
>> + * and none enabled already
>> + */
>> + if (kstrtoint(buf, 0,&num_vfs)< 0)
>> + return -EINVAL;
>
> The above comment belongs with
> 'if ((num_vfs> 0)&& (num_vfs<= total))'.
>
yes, forgot to move it. thanks!
>> + /* is PF driver loaded w/callback */
>> + if (!pdev->driver || !pdev->driver->sriov_configure) {
>> + pr_info("Driver doesn't support sriov configuration via sysfs \n");
>> + return -ENOSYS;
>
> Use dev_err() to set the proper severity and provide context. Not sure
> whether this is the right error code.
>
well, i used pr_info() b/c that was what I was requested to do
when I did similar cleanup in the dmar.c file recently. I
can change to dev_*() formatting.
>> + }
>> +
>> + /* if enabling vf's ... */
>> + total = pdev->sriov->total;
>> + if ((num_vfs> 0)&& (num_vfs<= total)) {
>> + if (pdev->sriov->nr_virtfn == 0) { /* if not already enabled */
>> + num_vfs_enabled = pdev->driver->sriov_configure(pdev, num_vfs);
>> + if ((num_vfs_enabled>= 0)&& (num_vfs_enabled != num_vfs))
>> + pr_warn("%s: %04x:%02x:%02x.%d: Only %d VFs enabled \n",
>> + pdev->dev.driver->name, pci_domain_nr(pdev->bus),
>> + pdev->bus->number, PCI_SLOT(pdev->devfn),
>> + PCI_FUNC(pdev->devfn), num_vfs_enabled);
>
> Use dev_warn(), don't try to replicate it.
>
ditto.
>> + return count;
>> + } else {
>> + pr_warn("VFs already enabled. Disable before re-enabling\n");
>> + return -EINVAL;
>
> It would be helpful to make an 'enable' write succeed when
> num_vfs == pdev->sriov->nr_virtfn.
>
ah, functional feedback!
yes, setting numvfs == existing value could reply success,
but I had it this way since it could show a programming error
with a mgmt tool that is trying to do the enable when it
should not, or already had, and a successful return may be interpreted
as a correctness in the mgmt app's logic. .... I could go either way.
I'll wait for other feedback on this to see which way the
group wants to have it.
>> + }
>> + }
>> +
>> + /* disable vfs */
>> + if (num_vfs == 0) {
>> + if (pdev->sriov->nr_virtfn != 0) {
>> + ret = pdev->driver->sriov_configure(pdev, 0);
>> + return ret ? ret: count;
>> + } else {
>> + pr_err("Disable failed since no VFs enabled\n");
>> + return -EINVAL;
>
> This definitely should be treated as successful, not an error.
>
Same comment as above. it could be considered successful,
or it could be considered a rtn for programming logic error
of a mgmt app. again, I could go either way. ditto above.
>> + }
>> + }
>> +
>> + pr_err("Invalid value for number of VFs to enable: %d\n", num_vfs);
>> +
>> + return -EINVAL;
>> +}
>> +#else
>> +static ssize_t sriov_totalvfs_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{ return 0; }
>> +static ssize_t sriov_numvfs_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{ return 0; }
>
> Shouldn't these print "0\n"?
>
agree.
>> +static ssize_t sriov_numvfs_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{ return count; }
>> +#endif /* CONFIG_PCI_IOV */
>> +
>> +static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
>> +static struct device_attribute sriov_numvfs_attr =
>> + __ATTR(sriov_numvfs, (S_IRUGO|S_IWUSR|S_IWGRP),
>> + sriov_numvfs_show, sriov_numvfs_store);
>
> sriov_numvfs should be read-only if !CONFIG_PCI_IOV, rather than
> ignoring writes.
>
If !CONFIG_PCI_IOV, none of these attributes are visible,
so they won't exist.
>> struct device_attribute pci_dev_attrs[] = {
>> __ATTR_RO(resource),
>> __ATTR_RO(vendor),
>> @@ -1194,7 +1303,7 @@ static ssize_t reset_store(struct device *dev,
>> {
>> struct pci_dev *pdev = to_pci_dev(dev);
>> unsigned long val;
>> - ssize_t result = strict_strtoul(buf, 0,&val);
>> + ssize_t result = kstrtoul(buf, 0,&val);
>>
>> if (result< 0)
>> return result;
>> @@ -1408,8 +1517,14 @@ static struct attribute *pci_dev_dev_attrs[] = {
>> NULL,
>> };
>>
>> +static struct attribute *sriov_dev_attrs[] = {
>> +&sriov_totalvfs_attr.attr,
>> +&sriov_numvfs_attr.attr,
>
> These are wrongly indented with spaces.
>
checkpatch!
>> + NULL,
>> +};
>> +
>> static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
>> - struct attribute *a, int n)
>> + struct attribute *a, int n)
>> {
>> struct device *dev = container_of(kobj, struct device, kobj);
>> struct pci_dev *pdev = to_pci_dev(dev);
>> @@ -1421,13 +1536,33 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
>> return a->mode;
>> }
>>
>> +static umode_t sriov_attrs_are_visible(struct kobject *kobj,
>> + struct attribute *a, int n)
>> +{
>> + struct device *dev = container_of(kobj, struct device, kobj);
>> +
>> + if ((a ==&sriov_totalvfs_attr.attr) ||
>> + (a ==&sriov_numvfs_attr.attr)) {
>> + if (!dev_is_pf(dev))
>> + return 0;
> [...]
>
> An odd thing about dev_is_pf() is that if !CONFIG_PCI_IOV then it is
> false for all devices. Which means that the dummy attribute reader
> functions will actually be dead code. I think that either dev_is_pf()
> should be fixed to be accurate when !CONFIG_PCI_IOV, or the attributes
> should be completely removed if !CONFIG_PCI_IOV.
>
> Ben.
>
Since it's all in the same file, I could wrap the following code with
CONFIG_PCI_IOV:
static const struct attribute_group *pci_dev_attr_groups[] = {
&pci_dev_attr_group,
#ifdef CONFIG_PCI_IOV
&sriov_dev_attr_group,
#endif
NULL,
};
and then the dummy attribute function could be removed.
another good cleanup, thanks!
Thanks for the thorough review of the above!
Ok, my turn:
Any feedback on having the sysfs configure call pci_sriov_[enable/disable](),
as well as do the don't-disable if VFs are assigned to guests?
^ permalink raw reply [flat|nested] 19+ messages in thread* RE: [PATCH 3/8] PCI: sysfs per device SRIOV control and status
2012-10-26 15:07 ` Don Dutile
@ 2012-10-31 17:01 ` Rose, Gregory V
2012-10-31 17:36 ` Ben Hutchings
0 siblings, 1 reply; 19+ messages in thread
From: Rose, Gregory V @ 2012-10-31 17:01 UTC (permalink / raw)
To: Don Dutile, Ben Hutchings
Cc: linux-pci@vger.kernel.org, bhelgaas@google.com,
yuvalmin@broadcom.com, yinghai@kernel.org, davem@davemloft.net
PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBEb24gRHV0aWxlIFttYWlsdG86
ZGR1dGlsZUByZWRoYXQuY29tXQ0KPiBTZW50OiBGcmlkYXksIE9jdG9iZXIgMjYsIDIwMTIgODow
NyBBTQ0KPiBUbzogQmVuIEh1dGNoaW5ncw0KPiBDYzogbGludXgtcGNpQHZnZXIua2VybmVsLm9y
ZzsgYmhlbGdhYXNAZ29vZ2xlLmNvbTsgeXV2YWxtaW5AYnJvYWRjb20uY29tOw0KPiBSb3NlLCBH
cmVnb3J5IFY7IHlpbmdoYWlAa2VybmVsLm9yZzsgZGF2ZW1AZGF2ZW1sb2Z0Lm5ldA0KPiBTdWJq
ZWN0OiBSZTogW1BBVENIIDMvOF0gUENJOiBzeXNmcyBwZXIgZGV2aWNlIFNSSU9WIGNvbnRyb2wg
YW5kIHN0YXR1cw0KPiANCiANCj4gDQo+IE9rLCBteSB0dXJuOg0KPiBBbnkgZmVlZGJhY2sgb24g
aGF2aW5nIHRoZSBzeXNmcyBjb25maWd1cmUgY2FsbA0KPiBwY2lfc3Jpb3ZfW2VuYWJsZS9kaXNh
YmxlXSgpLCBhcyB3ZWxsIGFzIGRvIHRoZSBkb24ndC1kaXNhYmxlIGlmIFZGcyBhcmUNCj4gYXNz
aWduZWQgdG8gZ3Vlc3RzPw0KPiANCg0KRG9uLA0KDQpBcyBJJ3ZlIG1lbnRpb25lZCBiZWZvcmUg
SSBzdGlsbCBwcmVmZXIgdG8gaGF2ZSB0aGUgc3lzZnMgaW50ZXJmYWNlIHlvdSd2ZSB3cml0dGVu
IHVwIG1ha2UgdGhlIGNhbGxzIHRvIHBjaV9zcmlvdl9lbmFibGUvZGlzYWJsZSgpIGFuZCBoYXZl
IHRoZSBjaGVja2luZyBmb3Igd2hldGhlciB0aGUgVkZzIGFyZSBhc3NpZ25lZCB0byBndWVzdHMg
ZG9uZSB0aGVyZSBhbHNvLCBidXQgcmVhbGx5IGl0IGlzbid0IGFueXRoaW5nIHdvcnRoIGdvaW5n
IHRvIHRoZSBtYXRzIGFib3V0LiAgQXMgaXQgc3RhbmRzIEkgdGhpbmsgaWYgeW91IGFkZHJlc3Mg
dGhlIGlzc3VlcyBicm91Z2h0IHVwIGJ5IEJlbiB0aGVuIEknbSBmaW5lIHdpdGggd2hhdCB5b3Un
dmUgd29ya2VkIHVwIHNvIGZhci4gIFNpbmNlIG5vIG9uZSBlbHNlIHNlZW1zIHRvIGhhdmUgYW4g
b3BpbmlvbiBhYm91dCBpdCAoYXMgZGVtb25zdHJhdGVkIGJ5IGEgbGFjayBvZiByZXNwb25zZSBv
dmVyIHRoZSBsYXN0IDUgZGF5cykgdGhlbiBJJ2Qgc3VnZ2VzdCB3ZSBnbyBmb3J3YXJkIHdpdGgg
dGhlIGN1cnJlbnQgaW1wbGVtZW50YXRpb24uICBJJ2QgcmVhbGx5IGxpa2UgdG8gc2VlIHRoaXMg
aW4gMy44IGlmIHBvc3NpYmxlLg0KDQpUaGFua3MgZm9yIGFsbCB5b3VyIHdvcmsuDQoNCi0gR3Jl
Zw0K
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH 3/8] PCI: sysfs per device SRIOV control and status
2012-10-31 17:01 ` Rose, Gregory V
@ 2012-10-31 17:36 ` Ben Hutchings
2012-10-31 18:18 ` Don Dutile
0 siblings, 1 reply; 19+ messages in thread
From: Ben Hutchings @ 2012-10-31 17:36 UTC (permalink / raw)
To: Rose, Gregory V
Cc: Don Dutile, linux-pci@vger.kernel.org, bhelgaas@google.com,
yuvalmin@broadcom.com, yinghai@kernel.org, davem@davemloft.net
On Wed, 2012-10-31 at 17:01 +0000, Rose, Gregory V wrote:
> > -----Original Message-----
> > From: Don Dutile [mailto:ddutile@redhat.com]
> > Sent: Friday, October 26, 2012 8:07 AM
> > To: Ben Hutchings
> > Cc: linux-pci@vger.kernel.org; bhelgaas@google.com; yuvalmin@broadcom.com;
> > Rose, Gregory V; yinghai@kernel.org; davem@davemloft.net
> > Subject: Re: [PATCH 3/8] PCI: sysfs per device SRIOV control and status
> >
>
> >
> > Ok, my turn:
> > Any feedback on having the sysfs configure call
> > pci_sriov_[enable/disable](), as well as do the don't-disable if VFs are
> > assigned to guests?
> >
>
> Don,
>
> As I've mentioned before I still prefer to have the sysfs interface
> you've written up make the calls to pci_sriov_enable/disable()
I think that would work for sfc, assuming the driver function is to be
called before either of those. I don't know whether it would work for
any of the other drivers with SR-IOV back-ends, though.
> and have the checking for whether the VFs are assigned to guests done
> there also,
I agree that this is should be centralised, though I think that could be
done as a later step without too much pain.
> but really it isn't anything worth going to the mats about. As it
> stands I think if you address the issues brought up by Ben then I'm
> fine with what you've worked up so far. Since no one else seems to
> have an opinion about it (as demonstrated by a lack of response over
> the last 5 days) then I'd suggest we go forward with the current
> implementation. I'd really like to see this in 3.8 if possible.
>
> Thanks for all your work.
Agreed, thanks Don.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/8] PCI: sysfs per device SRIOV control and status
2012-10-31 17:36 ` Ben Hutchings
@ 2012-10-31 18:18 ` Don Dutile
2012-10-31 18:25 ` Rose, Gregory V
0 siblings, 1 reply; 19+ messages in thread
From: Don Dutile @ 2012-10-31 18:18 UTC (permalink / raw)
To: Ben Hutchings
Cc: Rose, Gregory V, linux-pci@vger.kernel.org, bhelgaas@google.com,
yuvalmin@broadcom.com, yinghai@kernel.org, davem@davemloft.net
On 10/31/2012 01:36 PM, Ben Hutchings wrote:
> On Wed, 2012-10-31 at 17:01 +0000, Rose, Gregory V wrote:
>>> -----Original Message-----
>>> From: Don Dutile [mailto:ddutile@redhat.com]
>>> Sent: Friday, October 26, 2012 8:07 AM
>>> To: Ben Hutchings
>>> Cc: linux-pci@vger.kernel.org; bhelgaas@google.com; yuvalmin@broadcom.com;
>>> Rose, Gregory V; yinghai@kernel.org; davem@davemloft.net
>>> Subject: Re: [PATCH 3/8] PCI: sysfs per device SRIOV control and status
>>>
>>
>>>
>>> Ok, my turn:
>>> Any feedback on having the sysfs configure call
>>> pci_sriov_[enable/disable](), as well as do the don't-disable if VFs are
>>> assigned to guests?
>>>
>>
>> Don,
>>
>> As I've mentioned before I still prefer to have the sysfs interface
>> you've written up make the calls to pci_sriov_enable/disable()
>
> I think that would work for sfc, assuming the driver function is to be
> called before either of those. I don't know whether it would work for
> any of the other drivers with SR-IOV back-ends, though.
>
>> and have the checking for whether the VFs are assigned to guests done
>> there also,
>
> I agree that this is should be centralised, though I think that could be
> done as a later step without too much pain.
>
>> but really it isn't anything worth going to the mats about. As it
>> stands I think if you address the issues brought up by Ben then I'm
>> fine with what you've worked up so far. Since no one else seems to
>> have an opinion about it (as demonstrated by a lack of response over
>> the last 5 days) then I'd suggest we go forward with the current
>> implementation. I'd really like to see this in 3.8 if possible.
>>
>> Thanks for all your work.
>
> Agreed, thanks Don.
>
> Ben.
>
Greg & Ben,
Thanks for your feedback on the core doing the sriov_enable/disable.
I agree that let's get existing the core stuff into 3.8 & do the enable/disable as a follow up.
I'm just polishing the patch set now based on Ben's feedback....
I was delayed when my test machine was (improperly) taken from me (bad IT, bad! :-( ).
Just got the machine back today to test the various (error) cases,
and make checkpatch.pl happy. I expect to post a PATCH (non-RFC) later today.
- Don
ps -- of course, if Greg wants to give me a set of igb patches like the ones he did
for ixgbe, it'd get me out of the test machine battles -- I have private
test machine w/igb's. :) hint, hint...ok, more like 'beg, beg' ... again... ;)
^ permalink raw reply [flat|nested] 19+ messages in thread* RE: [PATCH 3/8] PCI: sysfs per device SRIOV control and status
2012-10-31 18:18 ` Don Dutile
@ 2012-10-31 18:25 ` Rose, Gregory V
0 siblings, 0 replies; 19+ messages in thread
From: Rose, Gregory V @ 2012-10-31 18:25 UTC (permalink / raw)
To: Don Dutile, Ben Hutchings
Cc: linux-pci@vger.kernel.org, bhelgaas@google.com,
yuvalmin@broadcom.com, yinghai@kernel.org, davem@davemloft.net
PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBEb24gRHV0aWxlIFttYWlsdG86
ZGR1dGlsZUByZWRoYXQuY29tXQ0KPiBTZW50OiBXZWRuZXNkYXksIE9jdG9iZXIgMzEsIDIwMTIg
MTE6MTkgQU0NCj4gVG86IEJlbiBIdXRjaGluZ3MNCj4gQ2M6IFJvc2UsIEdyZWdvcnkgVjsgbGlu
dXgtcGNpQHZnZXIua2VybmVsLm9yZzsgYmhlbGdhYXNAZ29vZ2xlLmNvbTsNCj4geXV2YWxtaW5A
YnJvYWRjb20uY29tOyB5aW5naGFpQGtlcm5lbC5vcmc7IGRhdmVtQGRhdmVtbG9mdC5uZXQNCj4g
U3ViamVjdDogUmU6IFtQQVRDSCAzLzhdIFBDSTogc3lzZnMgcGVyIGRldmljZSBTUklPViBjb250
cm9sIGFuZCBzdGF0dXMNCj4gDQo+IE9uIDEwLzMxLzIwMTIgMDE6MzYgUE0sIEJlbiBIdXRjaGlu
Z3Mgd3JvdGU6DQo+ID4gT24gV2VkLCAyMDEyLTEwLTMxIGF0IDE3OjAxICswMDAwLCBSb3NlLCBH
cmVnb3J5IFYgd3JvdGU6DQo+ID4+PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiA+Pj4g
RnJvbTogRG9uIER1dGlsZSBbbWFpbHRvOmRkdXRpbGVAcmVkaGF0LmNvbV0NCj4gPj4+IFNlbnQ6
IEZyaWRheSwgT2N0b2JlciAyNiwgMjAxMiA4OjA3IEFNDQo+ID4+PiBUbzogQmVuIEh1dGNoaW5n
cw0KPiA+Pj4gQ2M6IGxpbnV4LXBjaUB2Z2VyLmtlcm5lbC5vcmc7IGJoZWxnYWFzQGdvb2dsZS5j
b207DQo+ID4+PiB5dXZhbG1pbkBicm9hZGNvbS5jb207IFJvc2UsIEdyZWdvcnkgVjsgeWluZ2hh
aUBrZXJuZWwub3JnOw0KPiA+Pj4gZGF2ZW1AZGF2ZW1sb2Z0Lm5ldA0KPiA+Pj4gU3ViamVjdDog
UmU6IFtQQVRDSCAzLzhdIFBDSTogc3lzZnMgcGVyIGRldmljZSBTUklPViBjb250cm9sIGFuZA0K
PiA+Pj4gc3RhdHVzDQo+ID4+Pg0KPiA+Pg0KPiA+Pj4NCj4gPj4+IE9rLCBteSB0dXJuOg0KPiA+
Pj4gQW55IGZlZWRiYWNrIG9uIGhhdmluZyB0aGUgc3lzZnMgY29uZmlndXJlIGNhbGwNCj4gPj4+
IHBjaV9zcmlvdl9bZW5hYmxlL2Rpc2FibGVdKCksIGFzIHdlbGwgYXMgZG8gdGhlIGRvbid0LWRp
c2FibGUgaWYgVkZzDQo+ID4+PiBhcmUgYXNzaWduZWQgdG8gZ3Vlc3RzPw0KPiA+Pj4NCj4gPj4N
Cj4gPj4gRG9uLA0KPiA+Pg0KPiA+PiBBcyBJJ3ZlIG1lbnRpb25lZCBiZWZvcmUgSSBzdGlsbCBw
cmVmZXIgdG8gaGF2ZSB0aGUgc3lzZnMgaW50ZXJmYWNlDQo+ID4+IHlvdSd2ZSB3cml0dGVuIHVw
IG1ha2UgdGhlIGNhbGxzIHRvIHBjaV9zcmlvdl9lbmFibGUvZGlzYWJsZSgpDQo+ID4NCj4gPiBJ
IHRoaW5rIHRoYXQgd291bGQgd29yayBmb3Igc2ZjLCBhc3N1bWluZyB0aGUgZHJpdmVyIGZ1bmN0
aW9uIGlzIHRvIGJlDQo+ID4gY2FsbGVkIGJlZm9yZSBlaXRoZXIgb2YgdGhvc2UuICBJIGRvbid0
IGtub3cgd2hldGhlciBpdCB3b3VsZCB3b3JrIGZvcg0KPiA+IGFueSBvZiB0aGUgb3RoZXIgZHJp
dmVycyB3aXRoIFNSLUlPViBiYWNrLWVuZHMsIHRob3VnaC4NCj4gPg0KPiA+PiBhbmQgaGF2ZSB0
aGUgY2hlY2tpbmcgZm9yIHdoZXRoZXIgdGhlIFZGcyBhcmUgYXNzaWduZWQgdG8gZ3Vlc3RzIGRv
bmUNCj4gPj4gdGhlcmUgYWxzbywNCj4gPg0KPiA+IEkgYWdyZWUgdGhhdCB0aGlzIGlzIHNob3Vs
ZCBiZSBjZW50cmFsaXNlZCwgdGhvdWdoIEkgdGhpbmsgdGhhdCBjb3VsZA0KPiA+IGJlIGRvbmUg
YXMgYSBsYXRlciBzdGVwIHdpdGhvdXQgdG9vIG11Y2ggcGFpbi4NCj4gPg0KPiA+PiBidXQgcmVh
bGx5IGl0IGlzbid0IGFueXRoaW5nIHdvcnRoIGdvaW5nIHRvIHRoZSBtYXRzIGFib3V0LiAgQXMg
aXQNCj4gPj4gc3RhbmRzIEkgdGhpbmsgaWYgeW91IGFkZHJlc3MgdGhlIGlzc3VlcyBicm91Z2h0
IHVwIGJ5IEJlbiB0aGVuIEknbQ0KPiA+PiBmaW5lIHdpdGggd2hhdCB5b3UndmUgd29ya2VkIHVw
IHNvIGZhci4gIFNpbmNlIG5vIG9uZSBlbHNlIHNlZW1zIHRvDQo+ID4+IGhhdmUgYW4gb3Bpbmlv
biBhYm91dCBpdCAoYXMgZGVtb25zdHJhdGVkIGJ5IGEgbGFjayBvZiByZXNwb25zZSBvdmVyDQo+
ID4+IHRoZSBsYXN0IDUgZGF5cykgdGhlbiBJJ2Qgc3VnZ2VzdCB3ZSBnbyBmb3J3YXJkIHdpdGgg
dGhlIGN1cnJlbnQNCj4gPj4gaW1wbGVtZW50YXRpb24uICBJJ2QgcmVhbGx5IGxpa2UgdG8gc2Vl
IHRoaXMgaW4gMy44IGlmIHBvc3NpYmxlLg0KPiA+Pg0KPiA+PiBUaGFua3MgZm9yIGFsbCB5b3Vy
IHdvcmsuDQo+ID4NCj4gPiBBZ3JlZWQsIHRoYW5rcyBEb24uDQo+ID4NCj4gPiBCZW4uDQo+ID4N
Cj4gR3JlZyAmIEJlbiwNCj4gVGhhbmtzIGZvciB5b3VyIGZlZWRiYWNrIG9uIHRoZSBjb3JlIGRv
aW5nIHRoZSBzcmlvdl9lbmFibGUvZGlzYWJsZS4NCj4gSSBhZ3JlZSB0aGF0IGxldCdzIGdldCBl
eGlzdGluZyB0aGUgY29yZSBzdHVmZiBpbnRvIDMuOCAmIGRvIHRoZQ0KPiBlbmFibGUvZGlzYWJs
ZSBhcyBhIGZvbGxvdyB1cC4NCj4gSSdtIGp1c3QgcG9saXNoaW5nIHRoZSBwYXRjaCBzZXQgbm93
IGJhc2VkIG9uIEJlbidzIGZlZWRiYWNrLi4uLg0KPiBJIHdhcyBkZWxheWVkIHdoZW4gbXkgdGVz
dCBtYWNoaW5lIHdhcyAoaW1wcm9wZXJseSkgdGFrZW4gZnJvbSBtZSAoYmFkIElULA0KPiBiYWQh
IDotKCApLg0KPiBKdXN0IGdvdCB0aGUgbWFjaGluZSBiYWNrIHRvZGF5IHRvIHRlc3QgdGhlIHZh
cmlvdXMgKGVycm9yKSBjYXNlcywgYW5kDQo+IG1ha2UgY2hlY2twYXRjaC5wbCBoYXBweS4gIEkg
ZXhwZWN0IHRvIHBvc3QgYSBQQVRDSCAobm9uLVJGQykgbGF0ZXIgdG9kYXkuDQo+IC0gRG9uDQo+
IHBzIC0tIG9mIGNvdXJzZSwgaWYgR3JlZyB3YW50cyB0byBnaXZlIG1lIGEgc2V0IG9mIGlnYiBw
YXRjaGVzIGxpa2UgdGhlDQo+IG9uZXMgaGUgZGlkDQo+ICAgICAgICBmb3IgaXhnYmUsIGl0J2Qg
Z2V0IG1lIG91dCBvZiB0aGUgdGVzdCBtYWNoaW5lIGJhdHRsZXMgLS0gSSBoYXZlDQo+IHByaXZh
dGUNCj4gICAgICAgIHRlc3QgbWFjaGluZSB3L2lnYidzLiA6KSAgaGludCwgaGludC4uLm9rLCBt
b3JlIGxpa2UgJ2JlZywgYmVnJyAuLi4NCj4gYWdhaW4uLi4gOykNCg0KSSdsbCBzZWUgd2hhdCBJ
IGNhbiBkby4NCg0KOikNCg0KLSBHcmVnDQo=
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/8] sriov: provide method to reduce the number of total VFs supported
2012-10-25 18:38 [RFC] SRIOV device enable and disable via sysfs Donald Dutile
` (2 preceding siblings ...)
2012-10-25 18:38 ` [PATCH 3/8] PCI: sysfs per device SRIOV control and status Donald Dutile
@ 2012-10-25 18:38 ` Donald Dutile
2012-10-25 20:24 ` Ben Hutchings
2012-10-25 18:38 ` [PATCH 5/8] ixgbe: refactor mailbox ops init Donald Dutile
` (3 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Donald Dutile @ 2012-10-25 18:38 UTC (permalink / raw)
To: linux-pci
Cc: bhelgaas, yuvalmin, bhutchings, gregory.v.rose, yinghai, davem,
ddutile
Some implementations of SRIOV provide a capability structure
value of TotalVFs that is greater than what the software can support.
Provide a method to reduce the capability structure reported value
to the value the driver can support.
This ensures sysfs reports the current capability of the system,
hardware and software.
Example for its use: igb & ixgbe -- report 8 & 64 as TotalVFs,
but drivers only support 7 & 63 maximum.
Signed-off-by: Donald Dutile <ddutile@redhat.com>
---
drivers/pci/iov.c | 24 +++++++++++++++++++++++-
drivers/pci/pci-sysfs.c | 10 ++++++++--
drivers/pci/pci.h | 1 +
include/linux/pci.h | 5 +++++
4 files changed, 37 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index aeccc91..f1357b0 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -682,7 +682,6 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
if (!dev->is_physfn)
return -ENODEV;
-
return sriov_enable(dev, nr_virtfn);
}
EXPORT_SYMBOL_GPL(pci_enable_sriov);
@@ -735,3 +734,26 @@ int pci_num_vf(struct pci_dev *dev)
return dev->sriov->nr_virtfn;
}
EXPORT_SYMBOL_GPL(pci_num_vf);
+
+/**
+ * pci_sriov_set_totalvfs -- reduce the TotalVFs available
+ * @dev: the PCI PF device
+ * numvfs: number that should be used for TotalVFs supported
+ *
+ * Returns 0 if PF is an SRIOV-capable device and
+ * value of numvfs valid, otherwise -EINVAL
+ */
+int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
+{
+ if (!dev || !dev->is_physfn || (numvfs > dev->sriov->total))
+ return -EINVAL;
+
+ /* Shouldn't change if VFs already enabled */
+ if (!dev->sriov->ctrl & PCI_SRIOV_CTRL_VFE)
+ return -EIO;
+
+ dev->sriov->drvttl = numvfs;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(pci_sriov_set_totalvfs);
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index c2894ca..1cf6c15 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -414,7 +414,10 @@ static ssize_t sriov_totalvfs_show(struct device *dev,
u16 total;
pdev = to_pci_dev(dev);
- total = pdev->sriov->total;
+ if (pdev->sriov->drvttl)
+ total = pdev->sriov->drvttl;
+ else
+ total = pdev->sriov->total;
return sprintf (buf, "%u\n", total);
}
@@ -462,7 +465,10 @@ static ssize_t sriov_numvfs_store(struct device *dev,
}
/* if enabling vf's ... */
- total = pdev->sriov->total;
+ if (pdev->sriov->drvttl)
+ total = pdev->sriov->drvttl;
+ else
+ total = pdev->sriov->total;
if ((num_vfs > 0) && (num_vfs <= total)) {
if (pdev->sriov->nr_virtfn == 0) { /* if not already enabled */
num_vfs_enabled = pdev->driver->sriov_configure(pdev, num_vfs);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6f6cd14..553bbba 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -240,6 +240,7 @@ struct pci_sriov {
u16 stride; /* following VF stride */
u32 pgsz; /* page size for BAR alignment */
u8 link; /* Function Dependency Link */
+ u16 drvttl; /* max num VFs driver supports */
struct pci_dev *dev; /* lowest numbered PF */
struct pci_dev *self; /* this PF */
struct mutex lock; /* lock for VF bus */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1d60a23..a5e08f2 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1633,6 +1633,7 @@ extern int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
extern void pci_disable_sriov(struct pci_dev *dev);
extern irqreturn_t pci_sriov_migration(struct pci_dev *dev);
extern int pci_num_vf(struct pci_dev *dev);
+extern int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
#else
static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
{
@@ -1649,6 +1650,10 @@ static inline int pci_num_vf(struct pci_dev *dev)
{
return 0;
}
+static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
+{
+ return -EINVAL;
+}
#endif
#if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE)
--
1.7.10.2.552.gaa3bb87
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 4/8] sriov: provide method to reduce the number of total VFs supported
2012-10-25 18:38 ` [PATCH 4/8] sriov: provide method to reduce the number of total VFs supported Donald Dutile
@ 2012-10-25 20:24 ` Ben Hutchings
2012-10-26 15:11 ` Don Dutile
0 siblings, 1 reply; 19+ messages in thread
From: Ben Hutchings @ 2012-10-25 20:24 UTC (permalink / raw)
To: Donald Dutile
Cc: linux-pci, bhelgaas, yuvalmin, gregory.v.rose, yinghai, davem
On Thu, 2012-10-25 at 14:38 -0400, Donald Dutile wrote:
> Some implementations of SRIOV provide a capability structure
> value of TotalVFs that is greater than what the software can support.
> Provide a method to reduce the capability structure reported value
> to the value the driver can support.
> This ensures sysfs reports the current capability of the system,
> hardware and software.
> Example for its use: igb & ixgbe -- report 8 & 64 as TotalVFs,
> but drivers only support 7 & 63 maximum.
>
> Signed-off-by: Donald Dutile <ddutile@redhat.com>
> ---
> drivers/pci/iov.c | 24 +++++++++++++++++++++++-
> drivers/pci/pci-sysfs.c | 10 ++++++++--
> drivers/pci/pci.h | 1 +
> include/linux/pci.h | 5 +++++
> 4 files changed, 37 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index aeccc91..f1357b0 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -682,7 +682,6 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
>
> if (!dev->is_physfn)
> return -ENODEV;
> -
> return sriov_enable(dev, nr_virtfn);
> }
> EXPORT_SYMBOL_GPL(pci_enable_sriov);
> @@ -735,3 +734,26 @@ int pci_num_vf(struct pci_dev *dev)
> return dev->sriov->nr_virtfn;
> }
> EXPORT_SYMBOL_GPL(pci_num_vf);
> +
> +/**
> + * pci_sriov_set_totalvfs -- reduce the TotalVFs available
> + * @dev: the PCI PF device
> + * numvfs: number that should be used for TotalVFs supported
> + *
> + * Returns 0 if PF is an SRIOV-capable device and
> + * value of numvfs valid, otherwise -EINVAL
What are the locking requirements? Presumably this is expected to be
called from the probe function, with the device's mutex held?
> + */
> +int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
> +{
> + if (!dev || !dev->is_physfn || (numvfs > dev->sriov->total))
> + return -EINVAL;
> +
> + /* Shouldn't change if VFs already enabled */
> + if (!dev->sriov->ctrl & PCI_SRIOV_CTRL_VFE)
Missing parentheses.
> + return -EIO;
> +
> + dev->sriov->drvttl = numvfs;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_sriov_set_totalvfs);
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index c2894ca..1cf6c15 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -414,7 +414,10 @@ static ssize_t sriov_totalvfs_show(struct device *dev,
> u16 total;
>
> pdev = to_pci_dev(dev);
> - total = pdev->sriov->total;
> + if (pdev->sriov->drvttl)
> + total = pdev->sriov->drvttl;
> + else
> + total = pdev->sriov->total;
> return sprintf (buf, "%u\n", total);
> }
>
> @@ -462,7 +465,10 @@ static ssize_t sriov_numvfs_store(struct device *dev,
> }
>
> /* if enabling vf's ... */
> - total = pdev->sriov->total;
> + if (pdev->sriov->drvttl)
> + total = pdev->sriov->drvttl;
> + else
> + total = pdev->sriov->total;
> if ((num_vfs > 0) && (num_vfs <= total)) {
> if (pdev->sriov->nr_virtfn == 0) { /* if not already enabled */
> num_vfs_enabled = pdev->driver->sriov_configure(pdev, num_vfs);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 6f6cd14..553bbba 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -240,6 +240,7 @@ struct pci_sriov {
> u16 stride; /* following VF stride */
> u32 pgsz; /* page size for BAR alignment */
> u8 link; /* Function Dependency Link */
> + u16 drvttl; /* max num VFs driver supports */
It's a rather obscure possibility, but the device could be switched
between two different versions of a driver where one has a lower limit
and the other doesn't. So this should be reset to 0 when the driver is
removed.
> struct pci_dev *dev; /* lowest numbered PF */
> struct pci_dev *self; /* this PF */
> struct mutex lock; /* lock for VF bus */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 1d60a23..a5e08f2 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1633,6 +1633,7 @@ extern int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
> extern void pci_disable_sriov(struct pci_dev *dev);
> extern irqreturn_t pci_sriov_migration(struct pci_dev *dev);
> extern int pci_num_vf(struct pci_dev *dev);
> +extern int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
> #else
> static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
> {
> @@ -1649,6 +1650,10 @@ static inline int pci_num_vf(struct pci_dev *dev)
> {
> return 0;
> }
> +static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
> +{
> + return -EINVAL;
> +}
I think this should return 0, as the number of VFs certainly will be
limited to <= numvfs.
Ben.
> #endif
>
> #if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE)
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 4/8] sriov: provide method to reduce the number of total VFs supported
2012-10-25 20:24 ` Ben Hutchings
@ 2012-10-26 15:11 ` Don Dutile
0 siblings, 0 replies; 19+ messages in thread
From: Don Dutile @ 2012-10-26 15:11 UTC (permalink / raw)
To: Ben Hutchings
Cc: linux-pci, bhelgaas, yuvalmin, gregory.v.rose, yinghai, davem
On 10/25/2012 04:24 PM, Ben Hutchings wrote:
> On Thu, 2012-10-25 at 14:38 -0400, Donald Dutile wrote:
>> Some implementations of SRIOV provide a capability structure
>> value of TotalVFs that is greater than what the software can support.
>> Provide a method to reduce the capability structure reported value
>> to the value the driver can support.
>> This ensures sysfs reports the current capability of the system,
>> hardware and software.
>> Example for its use: igb& ixgbe -- report 8& 64 as TotalVFs,
>> but drivers only support 7& 63 maximum.
>>
>> Signed-off-by: Donald Dutile<ddutile@redhat.com>
>> ---
>> drivers/pci/iov.c | 24 +++++++++++++++++++++++-
>> drivers/pci/pci-sysfs.c | 10 ++++++++--
>> drivers/pci/pci.h | 1 +
>> include/linux/pci.h | 5 +++++
>> 4 files changed, 37 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index aeccc91..f1357b0 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -682,7 +682,6 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
>>
>> if (!dev->is_physfn)
>> return -ENODEV;
>> -
>> return sriov_enable(dev, nr_virtfn);
>> }
>> EXPORT_SYMBOL_GPL(pci_enable_sriov);
>> @@ -735,3 +734,26 @@ int pci_num_vf(struct pci_dev *dev)
>> return dev->sriov->nr_virtfn;
>> }
>> EXPORT_SYMBOL_GPL(pci_num_vf);
>> +
>> +/**
>> + * pci_sriov_set_totalvfs -- reduce the TotalVFs available
>> + * @dev: the PCI PF device
>> + * numvfs: number that should be used for TotalVFs supported
>> + *
>> + * Returns 0 if PF is an SRIOV-capable device and
>> + * value of numvfs valid, otherwise -EINVAL
>
> What are the locking requirements? Presumably this is expected to be
> called from the probe function, with the device's mutex held?
>
agreed. I'll add the above assumptions to the comments.
>> + */
>> +int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
>> +{
>> + if (!dev || !dev->is_physfn || (numvfs> dev->sriov->total))
>> + return -EINVAL;
>> +
>> + /* Shouldn't change if VFs already enabled */
>> + if (!dev->sriov->ctrl& PCI_SRIOV_CTRL_VFE)
>
> Missing parentheses.
>
good point. needed a bad caller of set_totalvfs().
i'll let 'my qa team' know they botched the testing! ;-)
>> + return -EIO;
>> +
>> + dev->sriov->drvttl = numvfs;
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(pci_sriov_set_totalvfs);
>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> index c2894ca..1cf6c15 100644
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
>> @@ -414,7 +414,10 @@ static ssize_t sriov_totalvfs_show(struct device *dev,
>> u16 total;
>>
>> pdev = to_pci_dev(dev);
>> - total = pdev->sriov->total;
>> + if (pdev->sriov->drvttl)
>> + total = pdev->sriov->drvttl;
>> + else
>> + total = pdev->sriov->total;
>> return sprintf (buf, "%u\n", total);
>> }
>>
>> @@ -462,7 +465,10 @@ static ssize_t sriov_numvfs_store(struct device *dev,
>> }
>>
>> /* if enabling vf's ... */
>> - total = pdev->sriov->total;
>> + if (pdev->sriov->drvttl)
>> + total = pdev->sriov->drvttl;
>> + else
>> + total = pdev->sriov->total;
>> if ((num_vfs> 0)&& (num_vfs<= total)) {
>> if (pdev->sriov->nr_virtfn == 0) { /* if not already enabled */
>> num_vfs_enabled = pdev->driver->sriov_configure(pdev, num_vfs);
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 6f6cd14..553bbba 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -240,6 +240,7 @@ struct pci_sriov {
>> u16 stride; /* following VF stride */
>> u32 pgsz; /* page size for BAR alignment */
>> u8 link; /* Function Dependency Link */
>> + u16 drvttl; /* max num VFs driver supports */
>
> It's a rather obscure possibility, but the device could be switched
> between two different versions of a driver where one has a lower limit
> and the other doesn't. So this should be reset to 0 when the driver is
> removed.
>
I was wondering the same, but I thought I followed the code flow
that when a driver was released, the sriov structure was freed...
I'll recheck...
>> struct pci_dev *dev; /* lowest numbered PF */
>> struct pci_dev *self; /* this PF */
>> struct mutex lock; /* lock for VF bus */
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 1d60a23..a5e08f2 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1633,6 +1633,7 @@ extern int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
>> extern void pci_disable_sriov(struct pci_dev *dev);
>> extern irqreturn_t pci_sriov_migration(struct pci_dev *dev);
>> extern int pci_num_vf(struct pci_dev *dev);
>> +extern int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>> #else
>> static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
>> {
>> @@ -1649,6 +1650,10 @@ static inline int pci_num_vf(struct pci_dev *dev)
>> {
>> return 0;
>> }
>> +static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
>> +{
>> + return -EINVAL;
>> +}
>
> I think this should return 0, as the number of VFs certainly will be
> limited to<= numvfs.
>
> Ben.
agreed.
>
>> #endif
>>
>> #if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE)
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 5/8] ixgbe: refactor mailbox ops init
2012-10-25 18:38 [RFC] SRIOV device enable and disable via sysfs Donald Dutile
` (3 preceding siblings ...)
2012-10-25 18:38 ` [PATCH 4/8] sriov: provide method to reduce the number of total VFs supported Donald Dutile
@ 2012-10-25 18:38 ` Donald Dutile
2012-10-25 18:38 ` [PATCH 6/8] ixgbe: refactor SRIOV enable and disable for sysfs interface Donald Dutile
` (2 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Donald Dutile @ 2012-10-25 18:38 UTC (permalink / raw)
To: linux-pci
Cc: bhelgaas, yuvalmin, bhutchings, gregory.v.rose, yinghai, davem,
ddutile
There is no actual dependency on initialization of the mailbox ops on
whether SR-IOV is enabled or not and it doesn't hurt to go ahead and
initialize ops unconditionally. Move the initialization into the device
probe so that the mailbox ops are initialized at the time we have the
board info necessary to do it.
Authored-by: Greg Rose <gregory.v.rose@intel.com>
Signed-off-by: Donald Dutile <ddutile@redhat.com>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 ++++-
drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 9 +--------
drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h | 3 +--
3 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 868af69..8d2a507 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7249,7 +7249,10 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
}
#ifdef CONFIG_PCI_IOV
- ixgbe_enable_sriov(adapter, ii);
+ /* Mailbox */
+ ixgbe_init_mbx_params_pf(hw);
+ memcpy(&hw->mbx.ops, ii->mbx_ops, sizeof(hw->mbx.ops));
+ ixgbe_enable_sriov(adapter);
#endif
netdev->features = NETIF_F_SG |
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
index dce48bf..9aeb929 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -44,8 +44,7 @@
#include "ixgbe_sriov.h"
#ifdef CONFIG_PCI_IOV
-void ixgbe_enable_sriov(struct ixgbe_adapter *adapter,
- const struct ixgbe_info *ii)
+void ixgbe_enable_sriov(struct ixgbe_adapter *adapter);
{
struct ixgbe_hw *hw = &adapter->hw;
int num_vf_macvlans, i;
@@ -124,12 +123,6 @@ void ixgbe_enable_sriov(struct ixgbe_adapter *adapter,
kcalloc(adapter->num_vfs,
sizeof(struct vf_data_storage), GFP_KERNEL);
if (adapter->vfinfo) {
- /* Now that we're sure SR-IOV is enabled
- * and memory allocated set up the mailbox parameters
- */
- ixgbe_init_mbx_params_pf(hw);
- memcpy(&hw->mbx.ops, ii->mbx_ops, sizeof(hw->mbx.ops));
-
/* limit trafffic classes based on VFs enabled */
if ((adapter->hw.mac.type == ixgbe_mac_82599EB) &&
(adapter->num_vfs < 16)) {
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
index 1be1d30..d96ead2 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
@@ -43,8 +43,7 @@ int ixgbe_ndo_get_vf_config(struct net_device *netdev,
void ixgbe_check_vf_rate_limit(struct ixgbe_adapter *adapter);
void ixgbe_disable_sriov(struct ixgbe_adapter *adapter);
#ifdef CONFIG_PCI_IOV
-void ixgbe_enable_sriov(struct ixgbe_adapter *adapter,
- const struct ixgbe_info *ii);
+void ixgbe_enable_sriov(struct ixgbe_adapter *adapter);
#endif
--
1.7.10.2.552.gaa3bb87
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 6/8] ixgbe: refactor SRIOV enable and disable for sysfs interface
2012-10-25 18:38 [RFC] SRIOV device enable and disable via sysfs Donald Dutile
` (4 preceding siblings ...)
2012-10-25 18:38 ` [PATCH 5/8] ixgbe: refactor mailbox ops init Donald Dutile
@ 2012-10-25 18:38 ` Donald Dutile
2012-10-25 18:38 ` [PATCH 7/8] ixgbe: sysfs sriov configuration callback support Donald Dutile
2012-10-25 18:38 ` [PATCH 8/8] ixgbe: change totalvfs to match support in driver Donald Dutile
7 siblings, 0 replies; 19+ messages in thread
From: Donald Dutile @ 2012-10-25 18:38 UTC (permalink / raw)
To: linux-pci
Cc: bhelgaas, yuvalmin, bhutchings, gregory.v.rose, yinghai, davem,
ddutile
In preparation for enable/disable of SR-IOV via the pci sysfs interface
move some core SR-IOV enablement code that would be common to module
parameter usage or callback from the pci bus driver to a separate
function so that it can be used by either method.
Authored-by: Greg Rose <gregory.v.rose@intel.com>
Signed-off-by: Donald Dutile <ddutile@redhat.com>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 98 +++++++++++++++-----------
1 file changed, 58 insertions(+), 40 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
index 9aeb929..7c068b4 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -44,49 +44,11 @@
#include "ixgbe_sriov.h"
#ifdef CONFIG_PCI_IOV
-void ixgbe_enable_sriov(struct ixgbe_adapter *adapter);
+static int __ixgbe_enable_sriov(struct ixgbe_adapter *adapter)
{
struct ixgbe_hw *hw = &adapter->hw;
int num_vf_macvlans, i;
struct vf_macvlans *mv_list;
- int pre_existing_vfs = 0;
-
- pre_existing_vfs = pci_num_vf(adapter->pdev);
- if (!pre_existing_vfs && !adapter->num_vfs)
- return;
-
- /* If there are pre-existing VFs then we have to force
- * use of that many because they were not deleted the last
- * time someone removed the PF driver. That would have
- * been because they were allocated to guest VMs and can't
- * be removed. Go ahead and just re-enable the old amount.
- * If the user wants to change the number of VFs they can
- * use ethtool while making sure no VFs are allocated to
- * guest VMs... i.e. the right way.
- */
- if (pre_existing_vfs) {
- adapter->num_vfs = pre_existing_vfs;
- dev_warn(&adapter->pdev->dev, "Virtual Functions already "
- "enabled for this device - Please reload all "
- "VF drivers to avoid spoofed packet errors\n");
- } else {
- int err;
- /*
- * The 82599 supports up to 64 VFs per physical function
- * but this implementation limits allocation to 63 so that
- * basic networking resources are still available to the
- * physical function. If the user requests greater thn
- * 63 VFs then it is an error - reset to default of zero.
- */
- adapter->num_vfs = min_t(unsigned int, adapter->num_vfs, 63);
-
- err = pci_enable_sriov(adapter->pdev, adapter->num_vfs);
- if (err) {
- e_err(probe, "Failed to enable PCI sriov: %d\n", err);
- adapter->num_vfs = 0;
- return;
- }
- }
adapter->flags |= IXGBE_FLAG_SRIOV_ENABLED;
e_info(probe, "SR-IOV enabled with %d VFs\n", adapter->num_vfs);
@@ -156,10 +118,66 @@ void ixgbe_enable_sriov(struct ixgbe_adapter *adapter);
/* enable spoof checking for all VFs */
for (i = 0; i < adapter->num_vfs; i++)
adapter->vfinfo[i].spoofchk_enabled = true;
+ return 0;
+ }
+
+ return -ENOMEM;
+}
+
+/*
+ * Note this function is called when the user wants to enable SR-IOV
+ * VFs using the now deprecated module parameter
+ */
+void ixgbe_enable_sriov(struct ixgbe_adapter *adapter)
+{
+ int pre_existing_vfs = 0;
+
+ pre_existing_vfs = pci_num_vf(adapter->pdev);
+ if (!pre_existing_vfs && !adapter->num_vfs)
return;
+
+ if (!pre_existing_vfs)
+ dev_warn(&adapter->pdev->dev,
+ "Enabling SR-IOV VFs using the module parameter is deprecated - please use the pci sysfs interface.\n");
+
+ /*
+ * If there are pre-existing VFs then we have to force
+ * use of that many - over ride any module parameter value.
+ * This may result from the user unloading the PF driver
+ * while VFs were assigned to guest VMs or because the VFs
+ * have been created via the new PCI SR-IOV sysfs interface.
+ */
+ if (pre_existing_vfs) {
+ adapter->num_vfs = pre_existing_vfs;
+ dev_warn(&adapter->pdev->dev, "Virtual Functions already "
+ "enabled for this device - Please reload all "
+ "VF drivers to avoid spoofed packet errors\n");
+ } else {
+ int err;
+ /*
+ * The 82599 supports up to 64 VFs per physical function
+ * but this implementation limits allocation to 63 so that
+ * basic networking resources are still available to the
+ * physical function. If the user requests greater thn
+ * 63 VFs then it is an error - reset to default of zero.
+ */
+ adapter->num_vfs = min_t(unsigned int, adapter->num_vfs, 63);
+
+ err = pci_enable_sriov(adapter->pdev, adapter->num_vfs);
+ if (err) {
+ e_err(probe, "Failed to enable PCI sriov: %d\n", err);
+ adapter->num_vfs = 0;
+ return;
+ }
}
- /* Oh oh */
+ if (!__ixgbe_enable_sriov(adapter))
+ return;
+
+ /*
+ * If we have gotten to this point then there is no memory available
+ * to manage the VF devices - print message and bail.
+ */
e_err(probe, "Unable to allocate memory for VF Data Storage - "
"SRIOV disabled\n");
ixgbe_disable_sriov(adapter);
--
1.7.10.2.552.gaa3bb87
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 7/8] ixgbe: sysfs sriov configuration callback support
2012-10-25 18:38 [RFC] SRIOV device enable and disable via sysfs Donald Dutile
` (5 preceding siblings ...)
2012-10-25 18:38 ` [PATCH 6/8] ixgbe: refactor SRIOV enable and disable for sysfs interface Donald Dutile
@ 2012-10-25 18:38 ` Donald Dutile
2012-10-25 18:38 ` [PATCH 8/8] ixgbe: change totalvfs to match support in driver Donald Dutile
7 siblings, 0 replies; 19+ messages in thread
From: Donald Dutile @ 2012-10-25 18:38 UTC (permalink / raw)
To: linux-pci
Cc: bhelgaas, yuvalmin, bhutchings, gregory.v.rose, yinghai, davem,
ddutile
Implement callbacks in the driver for the new PCI bus driver
interface that allows the user to enable/disable SR-IOV VFs
in a device via the sysfs interface.
Authored-by: Greg Rose <gregory.v.rose@intel.com>
Signed-off-by: Donald Dutile <ddutile@redhat.com>
---
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 3 +
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 28 +++++++-
drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 89 +++++++++++++++++++++++++-
drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h | 6 +-
4 files changed, 120 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 5bd2676..89fa3f8 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -753,5 +753,8 @@ extern int ixgbe_ptp_hwtstamp_ioctl(struct ixgbe_adapter *adapter,
extern void ixgbe_ptp_start_cyclecounter(struct ixgbe_adapter *adapter);
extern void ixgbe_ptp_check_pps_event(struct ixgbe_adapter *adapter, u32 eicr);
#endif /* CONFIG_IXGBE_PTP */
+#ifdef CONFIG_PCI_IOV
+void ixgbe_sriov_reinit(struct ixgbe_adapter *adapter);
+#endif
#endif /* _IXGBE_H_ */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 8d2a507..5d868e4 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -6784,6 +6784,26 @@ int ixgbe_setup_tc(struct net_device *dev, u8 tc)
}
#endif /* CONFIG_IXGBE_DCB */
+#ifdef CONFIG_PCI_IOV
+void ixgbe_sriov_reinit(struct ixgbe_adapter *adapter)
+{
+ struct net_device *netdev = adapter->netdev;
+
+ rtnl_lock();
+#ifdef CONFIG_IXGBE_DCB
+ ixgbe_setup_tc(netdev, netdev_get_num_tc(netdev));
+#else
+ if (netif_running(netdev))
+ ixgbe_close(netdev);
+ ixgbe_clear_interrupt_scheme(adapter);
+ ixgbe_init_interrupt_scheme(adapter);
+ if (netif_running(netdev))
+ ixgbe_open(netdev);
+#endif
+ rtnl_unlock();
+}
+
+#endif
void ixgbe_do_reset(struct net_device *netdev)
{
struct ixgbe_adapter *adapter = netdev_priv(netdev);
@@ -7519,7 +7539,12 @@ static void __devexit ixgbe_remove(struct pci_dev *pdev)
if (netdev->reg_state == NETREG_REGISTERED)
unregister_netdev(netdev);
- ixgbe_disable_sriov(adapter);
+ /*
+ * Only disable SR-IOV on unload if the user specified the now
+ * deprecated max_vfs module parameter.
+ */
+ if (max_vfs)
+ ixgbe_disable_sriov(adapter);
ixgbe_clear_interrupt_scheme(adapter);
@@ -7734,6 +7759,7 @@ static struct pci_driver ixgbe_driver = {
.resume = ixgbe_resume,
#endif
.shutdown = ixgbe_shutdown,
+ .sriov_configure = ixgbe_pci_sriov_configure,
.err_handler = &ixgbe_err_handler
};
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
index 7c068b4..60a2f4f 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -217,11 +217,12 @@ static bool ixgbe_vfs_are_assigned(struct ixgbe_adapter *adapter)
}
#endif /* #ifdef CONFIG_PCI_IOV */
-void ixgbe_disable_sriov(struct ixgbe_adapter *adapter)
+int ixgbe_disable_sriov(struct ixgbe_adapter *adapter)
{
struct ixgbe_hw *hw = &adapter->hw;
u32 gpie;
u32 vmdctl;
+ int rss;
/* set num VFs to 0 to prevent access to vfinfo */
adapter->num_vfs = 0;
@@ -236,7 +237,7 @@ void ixgbe_disable_sriov(struct ixgbe_adapter *adapter)
/* if SR-IOV is already disabled then there is nothing to do */
if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
- return;
+ return 0;
#ifdef CONFIG_PCI_IOV
/*
@@ -246,7 +247,7 @@ void ixgbe_disable_sriov(struct ixgbe_adapter *adapter)
*/
if (ixgbe_vfs_are_assigned(adapter)) {
e_dev_warn("Unloading driver while VFs are assigned - VFs will not be deallocated\n");
- return;
+ return -EPERM;
}
/* disable iov and allow time for transactions to clear */
pci_disable_sriov(adapter->pdev);
@@ -269,10 +270,92 @@ void ixgbe_disable_sriov(struct ixgbe_adapter *adapter)
adapter->flags &= ~IXGBE_FLAG_VMDQ_ENABLED;
adapter->ring_feature[RING_F_VMDQ].offset = 0;
+ rss = min_t(int, IXGBE_MAX_RSS_INDICES, num_online_cpus());
+ adapter->ring_feature[RING_F_RSS].limit = rss;
+
/* take a breather then clean up driver data */
msleep(100);
adapter->flags &= ~IXGBE_FLAG_SRIOV_ENABLED;
+ return 0;
+}
+
+int ixgbe_pci_sriov_enable(struct pci_dev *dev, int num_vfs)
+{
+#ifdef CONFIG_PCI_IOV
+ struct ixgbe_adapter *adapter = pci_get_drvdata(dev);
+ int err = 0;
+ int i;
+ int pre_existing_vfs = pci_num_vf(dev);
+
+ if (pre_existing_vfs && pre_existing_vfs != num_vfs)
+ err = ixgbe_disable_sriov(adapter);
+ else if (pre_existing_vfs && pre_existing_vfs == num_vfs)
+ goto out;
+
+ if (err)
+ goto err_out;
+
+ /*
+ * While the SR-IOV capability structure reports total VFs to be
+ * 64 we limit the actual number that can be allocated to 63 so
+ * that some transmit/receive resources can be reserved to the
+ * PF. The PCI bus driver already checks for other values out of
+ * range.
+ */
+ if (num_vfs > 63) {
+ err = -EPERM;
+ goto err_out;
+ }
+
+ adapter->num_vfs = num_vfs;
+
+ if ((err = __ixgbe_enable_sriov(adapter)))
+ goto err_out;
+
+ for (i = 0; i < adapter->num_vfs; i++)
+ ixgbe_vf_configuration(dev, (i | 0x10000000));
+
+ err = pci_enable_sriov(dev, num_vfs);
+ if (err) {
+ e_dev_warn("Failed to enable PCI sriov: %d\n", err);
+ goto err_out;
+ }
+ ixgbe_sriov_reinit(adapter);
+
+out:
+ return num_vfs;
+
+err_out:
+ return err;
+#endif
+ return 0;
+}
+
+int ixgbe_pci_sriov_disable(struct pci_dev *dev)
+{
+ struct ixgbe_adapter *adapter = pci_get_drvdata(dev);
+ int err;
+ u32 current_flags = adapter->flags;
+
+ err = ixgbe_disable_sriov(adapter);
+
+ /* Only reinit if no error and state changed */
+ if (!err && current_flags != adapter->flags) {
+ /* ixgbe_disable_sriov() doesn't clear VMDQ flag */
+ adapter->flags &= ~IXGBE_FLAG_VMDQ_ENABLED;
+ ixgbe_sriov_reinit(adapter);
+ }
+
+ return err;
+}
+
+int ixgbe_pci_sriov_configure(struct pci_dev *dev, int num_vfs)
+{
+ if (num_vfs == 0)
+ return ixgbe_pci_sriov_disable(dev);
+ else
+ return ixgbe_pci_sriov_enable(dev, num_vfs);
}
static int ixgbe_set_vf_multicasts(struct ixgbe_adapter *adapter,
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
index d96ead2..3d0fb07 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
@@ -41,11 +41,13 @@ int ixgbe_ndo_set_vf_spoofchk(struct net_device *netdev, int vf, bool setting);
int ixgbe_ndo_get_vf_config(struct net_device *netdev,
int vf, struct ifla_vf_info *ivi);
void ixgbe_check_vf_rate_limit(struct ixgbe_adapter *adapter);
-void ixgbe_disable_sriov(struct ixgbe_adapter *adapter);
+int ixgbe_disable_sriov(struct ixgbe_adapter *adapter);
#ifdef CONFIG_PCI_IOV
void ixgbe_enable_sriov(struct ixgbe_adapter *adapter);
#endif
-
+int ixgbe_pci_sriov_enable(struct pci_dev *dev, int num_vfs);
+int ixgbe_pci_sriov_disable(struct pci_dev *dev);
+int ixgbe_pci_sriov_configure(struct pci_dev *dev, int num_vfs);
#endif /* _IXGBE_SRIOV_H_ */
--
1.7.10.2.552.gaa3bb87
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 8/8] ixgbe: change totalvfs to match support in driver
2012-10-25 18:38 [RFC] SRIOV device enable and disable via sysfs Donald Dutile
` (6 preceding siblings ...)
2012-10-25 18:38 ` [PATCH 7/8] ixgbe: sysfs sriov configuration callback support Donald Dutile
@ 2012-10-25 18:38 ` Donald Dutile
7 siblings, 0 replies; 19+ messages in thread
From: Donald Dutile @ 2012-10-25 18:38 UTC (permalink / raw)
To: linux-pci
Cc: bhelgaas, yuvalmin, bhutchings, gregory.v.rose, yinghai, davem,
ddutile
ixgbe claims it supports 64 VFs in its SRIOV capability
structure, but the driver only supports 63. Adjust it
so sysfs sriov configuration checking will check with
the proper totalvf value.
Signed-off-by: Donald Dutile <ddutile@redhat.com>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 5d868e4..69e0cff 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7273,6 +7273,7 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
ixgbe_init_mbx_params_pf(hw);
memcpy(&hw->mbx.ops, ii->mbx_ops, sizeof(hw->mbx.ops));
ixgbe_enable_sriov(adapter);
+ pci_sriov_set_totalvfs(pdev, 63);
#endif
netdev->features = NETIF_F_SG |
--
1.7.10.2.552.gaa3bb87
^ permalink raw reply related [flat|nested] 19+ messages in thread