pci: Error disabling SR-IOV if in VFs assigned
PATCH 1/2 pci: Error disabling SR-IOV if in VFs assigned
Every sriov capable driver has to check if any guest is using a virtual function prior to disabling, so let's make it common code.
--- drivers/pci/pci-sysfs.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/pci/pci-sysfs.c +5 -0
@@ -487,6 +487,11 @@ static ssize_t sriov_numvfs_store(struct device *dev,if (num_vfs ==0 ) {/* disable VFs */ +if (pci_vfs_assigned (pdev)) { +dev_warn (&pdev->dev, +"Cannot disable SR-IOV VFs while assigned\n" ); +return -EPERM ; + } ret = pdev->driver->sriov_configure (pdev,0 );if (ret <0 )return ret;
Signature · 1 line
2.7.2
PATCH 2/2 nvme/pci: Enable SR-IOV capabilities
Registers a standard boiler-plate SR-IOV configure callback for NVMe.
--- drivers/nvme/host/pci.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
drivers/nvme/host/pci.c +13 -0
@@ -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, };
Signature · 1 line
2.7.2
PATCH 2/2 nvme/pci: Enable SR-IOV capabilities
Doesn't look fine (because the API is a mess), but the best we can do for now:
PATCH 2/2 nvme/pci: Enable SR-IOV capabilities
In reply to Keith BuschQuoted 16 lines · Keith Busch
Registers a standard boiler-plate SR-IOV configure callback for NVMe.
Signed-off-by Keith Busch <keith.busch at intel.com>--- drivers/nvme/host/pci.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)drivers/nvme/host/pci.c +1
-0@@ -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)
drivers/nvme/host/pci.c static void nvme_remove(struct pci_dev *pdev) · line 2046
Quoted 10 lines
drivers/nvme/host/pci.c +10
-0+{ +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);Quoted 18 lines
drivers/nvme/host/pci.c +1
-0#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, };Signature · 6 lines
2.7.2 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 2/2 nvme/pci: Enable SR-IOV capabilities
Quoted 16 lines · Bjorn Helgaas
drivers/nvme/host/pci.c static void nvme_remove(struct pci_dev *pdev) · line 2046
Quoted 11 lines
drivers/nvme/host/pci.c +11
-0+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:
Quoted 6 lines
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.
PATCH 2/2 nvme/pci: Enable SR-IOV capabilities
In reply to Bjorn HelgaasQuoted 3 lines · Bjorn Helgaas
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.
Quoted 6 lines
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.
PATCH 2/2 nvme/pci: Enable SR-IOV capabilities
Quoted 12 lines · Keith Busch
Quoted 3 lines · Bjorn Helgaas
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.
Quoted 5 lines
if (numvfs == 0) pci_disable_sriov(pdev); return 0; }
Quoted 5 lines
Quoted 1 line
return pci_enable_sriov(pdev, numvfs);Slight change to the above:
pci_enable_sriovreturns 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 :)
PATCH 1/2 pci: Error disabling SR-IOV if in VFs assigned
In reply to Keith BuschQuoted 2 lines · On Tue, 17 May 2016 15:39:58 -0600 Keith Busch <[email protected]> wrote:
Every sriov capable driver has to check if any guest is using a virtual function prior to disabling, so let's make it common code.
This is not true, the PCI_DEV_FLAGS_ASSIGNED flag is inherently racy, so checking it is really only a courtesy for broken drivers that still make use of it. I don't object to adding it here, though I wish the entire interface was deprecated, but it's only a minimal amount of insurance as a VF might get assigned immediately following your added test or might not participate in the assigned device flagging at all. I believe the better way to handle this is with proper host drivers for assigned devices that manage the driver .remove callback properly while devices are in use. The only reason to handle assigned devices specially in this case is when they don't have proper host drivers managing them, which is a problem that we've fixed. Thanks,
Alex
Quoted 21 lines
Signed-off-by Keith Busch <keith.busch at intel.com>--- drivers/pci/pci-sysfs.c | 5 +++++ 1 file changed, 5 insertions(+)drivers/pci/pci-sysfs.c +5
-0@@ -487,6 +487,11 @@ static ssize_t sriov_numvfs_store(struct device *dev,if (num_vfs ==0 ) {/* disable VFs */ +if (pci_vfs_assigned (pdev)) { +dev_warn (&pdev->dev, +"Cannot disable SR-IOV VFs while assigned\n" ); +return -EPERM ; + } ret = pdev->driver->sriov_configure (pdev,0 );if (ret <0 )return ret;
PATCH 1/2 pci: Error disabling SR-IOV if in VFs assigned
Quoted 6 lines · Alex Williamson
Quoted 2 lines · On Tue, 17 May 2016 15:39:58 -0600 Keith Busch <[email protected]> wrote:
Every sriov capable driver has to check if any guest is using a virtual function prior to disabling, so let's make it common code.
Quoted 6 lines
This is not true, the
PCI_DEV_FLAGS_ASSIGNEDflag is inherently racy, so checking it is really only a courtesy for broken drivers that still make use of it. I don't object to adding it here, though I wish the entire interface was deprecated, but it's only a minimal amount of insurance as a VF might get assigned immediately following your added test or might not participate in the assigned device flagging at all.
Si should we just kill it? As far as I can tell it's only used in these kinds of boilerplate checks.
Quoted 6 lines
I
believe the better way to handle this is with proper host drivers for assigned devices that manage the driver .remove callback properly while devices are in use. The only reason to handle assigned devices specially in this case is when they don't have proper host drivers managing them, which is a problem that we've fixed. Thanks,
We always use pci-stub now, don't we?
PATCH 1/2 pci: Error disabling SR-IOV if in VFs assigned
Quoted 14 lines · On Mon, 23 May 2016 03:55:32 -0700 Christoph Hellwig <[email protected]> wrote:
Quoted 6 lines · Alex Williamson
Quoted 2 lines · On Tue, 17 May 2016 15:39:58 -0600 Keith Busch <[email protected]> wrote:
Every sriov capable driver has to check if any guest is using a virtual function prior to disabling, so let's make it common code.
Quoted 6 lines
This is not true, the
PCI_DEV_FLAGS_ASSIGNEDflag is inherently racy, so checking it is really only a courtesy for broken drivers that still make use of it. I don't object to adding it here, though I wish the entire interface was deprecated, but it's only a minimal amount of insurance as a VF might get assigned immediately following your added test or might not participate in the assigned device flagging at all.
Quoted 2 lines
Si should we just kill it? As far as I can tell it's only used in these kinds of boilerplate checks.
Long term, yes, but perhaps KVM legacy PCI device assignment needs to be not only deprecated, but removed first.
Quoted 8 lines
Quoted 6 lines
I
believe the better way to handle this is with proper host drivers for assigned devices that manage the driver .remove callback properly while devices are in use. The only reason to handle assigned devices specially in this case is when they don't have proper host drivers managing them, which is a problem that we've fixed. Thanks,We always use pci-stub now, don't we?
pci-stub's only purpose is to prevent other drivers from binding to a device, such as while it's in used by legacy KVM device assignment. pci-stub has no visibility whether a device is in use or not, and will happily unbind the device at any point in time. Thus legacy KVM device assignment with pci-stub makes use of this horrible flag in a vain attempt to prevent devices from disappearing, littering every possible remove path with these sorts of checks when really the driver holding the assigned device should block or maybe allow an error return from the .remove callback. Xen pci-back also sets this flag, but I would hope there's a sensible solution available there and they just adopted use of this flag without really questioning that it works or makes sense. Thanks,
Alex
PATCH 1/2 pci: Error disabling SR-IOV if in VFs assigned
Quoted 17 lines · Alex Williamson
Quoted 14 lines · On Mon, 23 May 2016 03:55:32 -0700 Christoph Hellwig <[email protected]> wrote:
Quoted 12 lines · Alex Williamson
Quoted 2 lines · On Tue, 17 May 2016 15:39:58 -0600 Keith Busch <[email protected]> wrote:
Every sriov capable driver has to check if any guest is using a virtual function prior to disabling, so let's make it common code.
This is not true, the
PCI_DEV_FLAGS_ASSIGNEDflag is inherently racy, so checking it is really only a courtesy for broken drivers that still make use of it. I don't object to adding it here, though I wish the entire interface was deprecated, but it's only a minimal amount of insurance as a VF might get assigned immediately following your added test or might not participate in the assigned device flagging at all.
Quoted 5 lines
Quoted 2 lines
Si should we just kill it? As far as I can tell it's only used in these kinds of boilerplate checks.
Long term, yes, but perhaps KVM legacy PCI device assignment needs to be not only deprecated, but removed first.
Oh well. In that case I'd like to at least move it to the PCI core so that drivers don't have to care about it.
PATCH 1/2 pci: Error disabling SR-IOV if in VFs assigned
In reply to Keith BuschQuoted 4 lines · Keith Busch
Every sriov capable driver has to check if any guest is using a virtual function prior to disabling, so let's make it common code.
Signed-off-by Keith Busch <keith.busch at intel.com>
If I understand the discussion correctly, this is still racy but nobody objects to adding this until we have a better, non-racy solution.
However, you added this in common code and took advantage of it in nvme. Good so far. But we have about a dozen other drivers that call pci_vfs_assigned(). I assume some of those places could be changed so they take advantage of this check in the core instead?
Can we do that at the same time? If we add good new stuff and only use it one place, there's not as much overall goodness as there would be if we updated everybody to do it similarly.
Quoted 27 lines
--- drivers/pci/pci-sysfs.c | 5 +++++ 1 file changed, 5 insertions(+)drivers/pci/pci-sysfs.c +5
-0@@ -487,6 +487,11 @@ static ssize_t sriov_numvfs_store(struct device *dev,if (num_vfs ==0 ) {/* disable VFs */ +if (pci_vfs_assigned (pdev)) { +dev_warn (&pdev->dev, +"Cannot disable SR-IOV VFs while assigned\n" ); +return -EPERM ; + } ret = pdev->driver->sriov_configure (pdev,0 );if (ret <0 )return ret;Signature · 6 lines
2.7.2 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 1/2 pci: Error disabling SR-IOV if in VFs assigned
Quoted 9 lines · Bjorn Helgaas
Quoted 4 lines · Keith Busch
Every sriov capable driver has to check if any guest is using a virtual function prior to disabling, so let's make it common code.
Signed-off-by Keith Busch <keith.busch at intel.com>If I understand the discussion correctly, this is still racy but nobody objects to adding this until we have a better, non-racy solution.
My understanding as well: there's a potential race, but no different than what exists with today.
Quoted 8 lines
However, you added this in common code and took advantage of it in nvme. Good so far. But we have about a dozen other drivers that call
pci_vfs_assigned(). I assume some of those places could be changed so they take advantage of this check in the core instead?Can we do that at the same time? If we add good new stuff and only use it one place, there's not as much overall goodness as there would be if we updated everybody to do it similarly.
Sounds good, I'll send a series taking advantage of this for all the other PF drivers duplicating this check in their sriov_configure.
PATCH 1/2 pci: Error disabling SR-IOV if in VFs assigned
Quoted 7 lines · Keith Busch
Quoted 3 lines · Bjorn Helgaas
Can we do that at the same time? If we add good new stuff and only use it one place, there's not as much overall goodness as there would be if we updated everybody to do it similarly.
Sounds good, I'll send a series taking advantage of this for all the other PF drivers duplicating this check in their
sriov_configure.
Heh, I thought "no big deal", thinking all use was similar to NVMe's. However, most network drivers have multiple paths to sriov configuration, or have other requirements to changing the live count! There's only two drivers I find that can immediately use the simplification safely. I'll send those updates to just those ones.
PATCH 1/2 pci: Error disabling SR-IOV if in VFs assigned
Quoted 6 lines · Keith Busch
Quoted 5 lines · Keith Busch
Quoted 3 lines · Bjorn Helgaas
Can we do that at the same time? If we add good new stuff and only use it one place, there's not as much overall goodness as there would be if we updated everybody to do it similarly.
Quoted 8 lines
Quoted 2 lines
Sounds good, I'll send a series taking advantage of this for all the other PF drivers duplicating this check in their
sriov_configure.Heh, I thought "no big deal", thinking all use was similar to NVMe's. However, most network drivers have multiple paths to sriov configuration, or have other requirements to changing the live count! There's only two drivers I find that can immediately use the simplification safely. I'll send those updates to just those ones.
My take on that is, "it's a pretty trival check; just put it directly in nvme and don't bother doing anything in the PCI core." Maybe someday we'll come up with a way to make things more common. It doesn't really seem worth it to add something that only helps three drivers.
Bjorn
PATCH 1/2 pci: Error disabling SR-IOV if in VFs assigned
Quoted 5 lines · Bjorn Helgaas
My take on that is, "it's a pretty trival check; just put it directly in nvme and don't bother doing anything in the PCI core." Maybe someday we'll come up with a way to make things more common. It doesn't really seem worth it to add something that only helps three drivers.
Fair enough. Will withdraw the check in pci and resend nvme.
PATCH 1/2 pci: Error disabling SR-IOV if in VFs assigned
Quoted 8 lines · Keith Busch
Quoted 5 lines · Bjorn Helgaas
My take on that is, "it's a pretty trival check; just put it directly in nvme and don't bother doing anything in the PCI core." Maybe someday we'll come up with a way to make things more common. It doesn't really seem worth it to add something that only helps three drivers.
Fair enough. Will withdraw the check in pci and resend nvme.
Sorry for leading you down this path..
Bjorn Helgaas
maintainerPATCH 1/2 pci: Error disabling SR-IOV if in VFs assigned
Quoted 11 lines · Christoph Hellwig
Quoted 8 lines · Keith Busch
Quoted 5 lines · Bjorn Helgaas
My take on that is, "it's a pretty trival check; just put it directly in nvme and don't bother doing anything in the PCI core." Maybe someday we'll come up with a way to make things more common. It doesn't really seem worth it to add something that only helps three drivers.
Fair enough. Will withdraw the check in pci and resend nvme.
Sorry for leading you down this path..
No problem, I think it would be awesome if we could unify these drivers somehow, and unification always involves lots of dead-ends :)