| Message ID | 20180329182231.32151-1-jakub.kicinski@netronome.com |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: allow drivers to limit the number of VFs to 0 | expand |
On Thu, Mar 29, 2018 at 11:22:31AM -0700, Jakub Kicinski wrote: > Some user space depends on driver allowing sriov_totalvfs to be > enabled. I can't make sene of this sentence. Can you explain what user space code depends on what semantics? The sriov_totalvfs file should show up for any device supporting SR-IOV as far as I can tell. > > For devices which VF support depends on loaded FW we > have the pci_sriov_{g,s}et_totalvfs() API. However, this API > uses 0 as a special "unset" value, meaning drivers can't limit > sriov_totalvfs to 0. Change the special value to be U16_MAX. > Use simple min() to determine actual totalvfs. Please use a PCI_MAX_VFS or similar define instead of plain U16_MAX or ~0.
On Fri, Mar 30, 2018 at 4:49 AM, Christoph Hellwig <hch@infradead.org> wrote: > On Thu, Mar 29, 2018 at 11:22:31AM -0700, Jakub Kicinski wrote: >> Some user space depends on driver allowing sriov_totalvfs to be >> enabled. > > I can't make sene of this sentence. Can you explain what user space > code depends on what semantics? The sriov_totalvfs file should show > up for any device supporting SR-IOV as far as I can tell. > >> >> For devices which VF support depends on loaded FW we >> have the pci_sriov_{g,s}et_totalvfs() API. However, this API >> uses 0 as a special "unset" value, meaning drivers can't limit >> sriov_totalvfs to 0. Change the special value to be U16_MAX. >> Use simple min() to determine actual totalvfs. > > Please use a PCI_MAX_VFS or similar define instead of plain U16_MAX or ~0. Actually is there any reason why driver_max_VFs couldn't just be initialized to the same value as total_VFs? Also looking over the patch I don't see how writing ~0 would be accepted unless you also make changes to pci_sriov_set_totalvfs since it should fail the "numvfs > dev->sriov->total_VFs" check. You might just want to look at adding a new function that would reset the driver_max_VFs value instead of trying to write it to an arbitrary value from the driver. - Alex
On Fri, 30 Mar 2018 04:49:05 -0700, Christoph Hellwig wrote: > On Thu, Mar 29, 2018 at 11:22:31AM -0700, Jakub Kicinski wrote: > > Some user space depends on driver allowing sriov_totalvfs to be > > enabled. > > I can't make sene of this sentence. Can you explain what user space > code depends on what semantics? The sriov_totalvfs file should show > up for any device supporting SR-IOV as far as I can tell. Ugh, I took me a while to understand what I meant myself... So what I meant is that this should generally work: # cat .../sriov_totalvfs > .../sriov_numvfs I.e. one should be able to "enable" the number of VFs advertised in sriov_totalvfs. I will rephrase! > > For devices which VF support depends on loaded FW we > > have the pci_sriov_{g,s}et_totalvfs() API. However, this API > > uses 0 as a special "unset" value, meaning drivers can't limit > > sriov_totalvfs to 0. Change the special value to be U16_MAX. > > Use simple min() to determine actual totalvfs. > > Please use a PCI_MAX_VFS or similar define instead of plain U16_MAX or ~0. I think I may just go with what I think Alex is suggesting and "unset" the value to total_VFs.
On Fri, 30 Mar 2018 09:54:37 -0700, Alexander Duyck wrote: > On Fri, Mar 30, 2018 at 4:49 AM, Christoph Hellwig <hch@infradead.org> wrote: > > On Thu, Mar 29, 2018 at 11:22:31AM -0700, Jakub Kicinski wrote: > >> Some user space depends on driver allowing sriov_totalvfs to be > >> enabled. > > > > I can't make sene of this sentence. Can you explain what user space > > code depends on what semantics? The sriov_totalvfs file should show > > up for any device supporting SR-IOV as far as I can tell. > > > >> > >> For devices which VF support depends on loaded FW we > >> have the pci_sriov_{g,s}et_totalvfs() API. However, this API > >> uses 0 as a special "unset" value, meaning drivers can't limit > >> sriov_totalvfs to 0. Change the special value to be U16_MAX. > >> Use simple min() to determine actual totalvfs. > > > > Please use a PCI_MAX_VFS or similar define instead of plain U16_MAX or ~0. > > Actually is there any reason why driver_max_VFs couldn't just be > initialized to the same value as total_VFs? > > Also looking over the patch I don't see how writing ~0 would be > accepted unless you also make changes to pci_sriov_set_totalvfs since > it should fail the "numvfs > dev->sriov->total_VFs" check. You might > just want to look at adding a new function that would reset the > driver_max_VFs value instead of trying to write it to an arbitrary > value from the driver. Ack, the reset function plus using total_VFs as unset seems a lot cleaner, thanks!
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.c b/drivers/net/ethernet/netronome/nfp/nfp_main.c index c4b1f344b4da..dcd6e208a155 100644 --- a/drivers/net/ethernet/netronome/nfp/nfp_main.c +++ b/drivers/net/ethernet/netronome/nfp/nfp_main.c @@ -123,7 +123,7 @@ static int nfp_pcie_sriov_read_nfd_limit(struct nfp_pf *pf) return pci_sriov_set_totalvfs(pf->pdev, pf->limit_vfs); pf->limit_vfs = ~0; - pci_sriov_set_totalvfs(pf->pdev, 0); /* 0 is unset */ + pci_sriov_set_totalvfs(pf->pdev, ~0); /* Allow any setting for backwards compatibility if symbol not found */ if (err == -ENOENT) return 0; @@ -537,7 +537,7 @@ static int nfp_pci_probe(struct pci_dev *pdev, err_net_remove: nfp_net_pci_remove(pf); err_sriov_unlimit: - pci_sriov_set_totalvfs(pf->pdev, 0); + pci_sriov_set_totalvfs(pf->pdev, ~0); err_fw_unload: kfree(pf->rtbl); nfp_mip_close(pf->mip); @@ -570,7 +570,7 @@ static void nfp_pci_remove(struct pci_dev *pdev) nfp_hwmon_unregister(pf); nfp_pcie_sriov_disable(pdev); - pci_sriov_set_totalvfs(pf->pdev, 0); + pci_sriov_set_totalvfs(pf->pdev, ~0); nfp_net_pci_remove(pf); diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c index 677924ae0350..aa3dfe3ecd68 100644 --- a/drivers/pci/iov.c +++ b/drivers/pci/iov.c @@ -446,6 +446,7 @@ static int sriov_init(struct pci_dev *dev, int pos) pci_read_config_word(dev, pos + PCI_SRIOV_VF_DID, &iov->vf_device); iov->pgsz = pgsz; iov->self = dev; + iov->driver_max_VFs = U16_MAX; iov->drivers_autoprobe = true; pci_read_config_dword(dev, pos + PCI_SRIOV_CAP, &iov->cap); pci_read_config_byte(dev, pos + PCI_SRIOV_FUNC_LINK, &iov->link); @@ -801,9 +802,6 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev) if (!dev->is_physfn) return 0; - if (dev->sriov->driver_max_VFs) - return dev->sriov->driver_max_VFs; - - return dev->sriov->total_VFs; + return min(dev->sriov->total_VFs, dev->sriov->driver_max_VFs); } EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
Some user space depends on driver allowing sriov_totalvfs to be enabled. For devices which VF support depends on loaded FW we have the pci_sriov_{g,s}et_totalvfs() API. However, this API uses 0 as a special "unset" value, meaning drivers can't limit sriov_totalvfs to 0. Change the special value to be U16_MAX. Use simple min() to determine actual totalvfs. Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> --- drivers/net/ethernet/netronome/nfp/nfp_main.c | 6 +++--- drivers/pci/iov.c | 6 ++---- 2 files changed, 5 insertions(+), 7 deletions(-)