| Message ID | 1463521199-16604-2-git-send-email-keith.busch@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Doesn't look fine (because the API is a mess), but the best we can do
for now:
Reviewed-by: Christoph Hellwig <hch@lst.de>
--
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
On Tue, May 17, 2016 at 03:39:59PM -0600, Keith Busch wrote: > Registers a standard boiler-plate SR-IOV configure callback for NVMe. > > Signed-off-by: Keith Busch <keith.busch@intel.com> > --- > drivers/nvme/host/pci.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index fbe8cc2..8379b9a 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -2033,6 +2033,18 @@ static void nvme_remove(struct pci_dev *pdev) > nvme_put_ctrl(&dev->ctrl); > } > > +static int nvme_sriov_configure(struct pci_dev *pdev, int numvfs) > +{ > + int ret = 0; > + > + if (numvfs == 0) > + pci_disable_sriov(pdev); > + else > + ret = pci_enable_sriov(pdev, numvfs); > + > + return ret ? ret : numvfs; > +} I do not subscribe to the belief that every function should have a single exit. In this case, I think it makes the function much harder to understand than this: if (numvfs == 0) pci_disable_sriov(pdev); return 0; } return pci_enable_sriov(pdev, numvfs); > #ifdef CONFIG_PM_SLEEP > static int nvme_suspend(struct device *dev) > { > @@ -2127,6 +2139,7 @@ static struct pci_driver nvme_driver = { > .driver = { > .pm = &nvme_dev_pm_ops, > }, > + .sriov_configure = nvme_sriov_configure, > .err_handler = &nvme_err_handler, > }; > > -- > 2.7.2 > > -- > 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 -- 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
On Mon, May 23, 2016 at 12:06:52PM -0500, Bjorn Helgaas wrote: > > +static int nvme_sriov_configure(struct pci_dev *pdev, int numvfs) > > +{ > > + int ret = 0; > > + > > + if (numvfs == 0) > > + pci_disable_sriov(pdev); > > + else > > + ret = pci_enable_sriov(pdev, numvfs); > > + > > + return ret ? ret : numvfs; > > +} > > I do not subscribe to the belief that every function should have a > single exit. In this case, I think it makes the function much harder > to understand than this: > > if (numvfs == 0) > pci_disable_sriov(pdev); > return 0; > } > > return pci_enable_sriov(pdev, numvfs); I'd tend to agree. But then again the real issue here is that we should have two methods instead, and useful defaults. I.e. if the SR-IOV API was properly designed NVMe wouldn't even need this callout at all, and drivers that need it should have one method each for enable / disable. -- 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
On Mon, May 23, 2016 at 12:06:52PM -0500, Bjorn Helgaas wrote: > I do not subscribe to the belief that every function should have a > single exit. In this case, I think it makes the function much harder > to understand than this: No problem with multiple exists. > if (numvfs == 0) > pci_disable_sriov(pdev); > return 0; > } > > return pci_enable_sriov(pdev, numvfs); Slight change to the above: pci_enable_sriov returns 0 on success, but a driver's .sriov_configure is supposed to return the number of VF's successfully configured, so need to return 'numvfs' on success. -- 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
On Mon, May 23, 2016 at 01:21:44PM -0400, Keith Busch wrote: > On Mon, May 23, 2016 at 12:06:52PM -0500, Bjorn Helgaas wrote: > > I do not subscribe to the belief that every function should have a > > single exit. In this case, I think it makes the function much harder > > to understand than this: > > No problem with multiple exists. > > > if (numvfs == 0) > > pci_disable_sriov(pdev); > > return 0; > > } > > > > return pci_enable_sriov(pdev, numvfs); > > Slight change to the above: pci_enable_sriov returns 0 on success, > but a driver's .sriov_configure is supposed to return the number of > VF's successfully configured, so need to return 'numvfs' on success. Oops, sorry, excuse me while I wipe the egg off my face :) -- 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/nvme/host/pci.c b/drivers/nvme/host/pci.c index fbe8cc2..8379b9a 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -2033,6 +2033,18 @@ static void nvme_remove(struct pci_dev *pdev) nvme_put_ctrl(&dev->ctrl); } +static int nvme_sriov_configure(struct pci_dev *pdev, int numvfs) +{ + int ret = 0; + + if (numvfs == 0) + pci_disable_sriov(pdev); + else + ret = pci_enable_sriov(pdev, numvfs); + + return ret ? ret : numvfs; +} + #ifdef CONFIG_PM_SLEEP static int nvme_suspend(struct device *dev) { @@ -2127,6 +2139,7 @@ static struct pci_driver nvme_driver = { .driver = { .pm = &nvme_dev_pm_ops, }, + .sriov_configure = nvme_sriov_configure, .err_handler = &nvme_err_handler, };
Registers a standard boiler-plate SR-IOV configure callback for NVMe. Signed-off-by: Keith Busch <keith.busch@intel.com> --- drivers/nvme/host/pci.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)