| Message ID | 1351190310-5043-5-git-send-email-ddutile@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
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)
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) > -- 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
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)
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(-)