PCI: Device driver function reset notification

Disable and shutdown the device prior to reset, and restart the device after.

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/block/nvme-core.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/block/nvme-core.c +11 -1

index 625259d..273ff12 100644
@@ -2605,6 +2605,16 @@ static void nvme_reset_failed_dev(struct work_struct *ws)
        nvme_dev_reset(dev);
 }
 
+static void nvme_reset_notify(struct pci_dev *pdev, int prepare)
+{
+       struct nvme_dev *dev = pci_get_drvdata(pdev);
+
+       if (prepare)
+               nvme_dev_shutdown(dev);
+       else
+               nvme_dev_resume(dev);
+}
+
 static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
        int result = -ENOMEM;

@@ -2744,6 +2754,7 @@ static const struct pci_error_handlers nvme_err_handler = {
        .link_reset     = nvme_link_reset,
        .slot_reset     = nvme_slot_reset,
        .resume         = nvme_error_resume,
+       .reset_notify   = nvme_reset_notify,
 };
 
 /* Move to pci_ids.h later */
--

1.7.10.4

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://merlin.infradead.org/mailman/listinfo/linux-nvme

Hi Keith,

I think NVMe Reset should apply to PF mode driver only, and not to VF mode driver.
Is that understanding correct? Does the NVMe driver know which mode its running in?

Quoted 6 lines · Learner Study

Hi Keith,

I think NVMe Reset should apply to PF mode driver only, and not to VF mode driver.
Is that understanding correct? Does the NVMe driver know which mode its running in?

Oh, this driver doesn't enable SR-IOV and has no PF/VF awareness. Shame on us, I'll add it to my list unless someone beats me to it.

Still, I think we'd like to be able to reset a VF if only because it gets the queues back to a pristine state. A VF reset should not affect any of the other functions.

But Won't resetting from a VF impact functionality of other VFs?

Quoted 14 lines · Keith Busch

Quoted 6 lines · Learner Study

Hi Keith,

I think NVMe Reset should apply to PF mode driver only, and not to VF mode driver.
Is that understanding correct? Does the NVMe driver know which mode its running in?

Oh, this driver doesn't enable SR-IOV and has no PF/VF awareness. Shame on us, I'll add it to my list unless someone beats me to it.

Still, I think we'd like to be able to reset a VF if only because it gets the queues back to a pristine state. A VF reset should not affect any of the other functions.

But Won't resetting from a VF impact functionality of other VFs?

Quoted 14 lines · Keith Busch

Quoted 6 lines · Learner Study

Hi Keith,

I think NVMe Reset should apply to PF mode driver only, and not to VF mode driver.
Is that understanding correct? Does the NVMe driver know which mode its running in?

Oh, this driver doesn't enable SR-IOV and has no PF/VF awareness. Shame on us, I'll add it to my list unless someone beats me to it.

Still, I think we'd like to be able to reset a VF if only because it gets the queues back to a pristine state. A VF reset should not affect any of the other functions.

Section 8.5 of the NVMe 1.1 spec contains the following: "While the details associated with implementing a controller that supports SR-IOV are outside the scope of this specification, such a controller shall implement fully compliant NVM Express Virtual Functions (VFs). This ensures that the same host software developed for non-virtualized environments is capable of running unmodified within an SI. No such requirement exists for the Physical Function (PF)."

If a VF is a fully NVMe compliant device then it must at least act like it implements NVMe subsystem reset. How the SR-IOV-capable controller actually implements this is left up to the vendor. But the spec does not require that a reset of one VF initiate a reset of other VF.

-----Original Message-----
From: Linux-nvme [mailto:linux-nvme-bounces at lists.infradead.org] On Behalf Of Learner Study
Sent: Monday, April 21, 2014 8:34 PM
To: Busch, Keith
Cc: bhelgaas at google.com; linux-pci at vger.kernel.org; linux-nvme
Subject: Re: [PATCH 2/2] NVMe: Implement PCI-e reset notification callback

Per the spec, "When an NVM Subsystem Reset occurs, the entire NVM subsystem is reset"....so all VFs would get impacted, if a VF does a Reset. So, I think Reset kind of control should be in PF mode only.

Quoted 18 lines · Learner

But Won't resetting from a VF impact functionality of other VFs?

Quoted 14 lines · Keith Busch

Quoted 6 lines · Learner Study

Hi Keith,

I think NVMe Reset should apply to PF mode driver only, and not to VF mode driver.
Is that understanding correct? Does the NVMe driver know which mode its running in?

Oh, this driver doesn't enable SR-IOV and has no PF/VF awareness. Shame on us, I'll add it to my list unless someone beats me to it.

Still, I think we'd like to be able to reset a VF if only because it gets the queues back to a pristine state. A VF reset should not affect any of the other functions.

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Hi, everyone,
I want to ask that weather the NVMe driver has been tried on the
IBM or Lenovo server?
I use it on IBM(Lenovo) server with suse 11 SP3, but the MSI-X
irq is always could not be got by cpu(may be it is masked).
Has anyone got this kind of problem?

Quoted 7 lines · On Tue, Apr 22, 2014 at 5:33 AM, liaohengquan1986 <[email protected]>…

Hi, everyone,
I want to ask that weather the NVMe driver has been tried on the
IBM or Lenovo server?
I use it on IBM(Lenovo) server with suse 11 SP3, but the MSI-X
irq is always could not be got by cpu(may be it is masked).
Has anyone got this kind of problem?

I think your original email contained non-plain text, so it won't appear on the mailing list, and some recipients may auto-discard it as well. See http://vger.kernel.org/majordomo-info.html

If you can reproduce the problem with an upstream kernel, this is the right place to debug it. We'd need a complete dmesg log and "lspci -vv" output to start with.

If the problem only happens with SUSE, then you'd want to work with SUSE to figure it out.

Bjorn

Section 8.5 of the NVMe 1.1 spec contains the following: "While the details associated with implementing a controller that supports SR-IOV are outside the scope of this specification, such a controller shall implement fully compliant NVM Express Virtual Functions (VFs). This ensures that the same host software developed for non-virtualized environments is capable of running unmodified within an SI. No such requirement exists for the Physical Function (PF)."

If a VF is a fully NVMe compliant device then it must at least act like it implements NVMe subsystem reset. How the SR-IOV-capable controller actually implements this is left up to the vendor. But the spec does not require that a reset of one VF initiate a reset of other VF.

Quoted 11 lines · Barrett N

Section 8.5 of the NVMe 1.1 spec contains the following: "While the details
associated with implementing a controller that supports SR-IOV are outside the scope of this specification, such a controller shall implement fully compliant NVM Express Virtual Functions (VFs). This ensures that the same host software developed for non-virtualized environments is capable of running unmodified within an SI. No such requirement exists for the Physical
Function (PF)."

Quoted 6 lines

If a VF is a fully NVMe compliant device then it must at least act like it implements NVMe subsystem reset. How the SR-IOV-capable controller actually
implements this is left up to the vendor. But the spec does not require that
a reset of one VF initiate a reset of other VF.

The NVMe spec does not, but PCI does.

In case it wasn't clear from PATCH 1/2, this proposed callback is for a function level reset. The PCI SR-IOV spec says all VF's implement FLR and that this reset does not affect any other functions, from section 2.2.2:

VFs must support Function Level Reset (FLR).

Note: Software may use FLR to reset a VF. FLR to a VF affects a VF?s state but does not affect its existence in PCI Configuration Space or PCI Bus address space. The VFs BARn values (see Section 3.3.14) and VF MSE (see Section 3.3.3.4) in the PF?s SR-IOV extended capability are unaffected by FLRs issued to the VF.

Further, an NVMe subsystem reset is not the same as a controller or function level reset. I have not proposed doing a subsytem reset here.

Since this is a call-back invoked from an FLR that happens outside the driver whether the driver wants it to happen or not, it's better the driver is aware and prepared that this occured rather than not knowing and left to wonder why the heck the controller stopped responding.

A user can issue a pci function level reset to a device using sysfs entry /sys/bus/pci/devices/.../reset. A kernel driver handling the pci device might like to know this reset is about to occur and when the reset attempt completes. This is so the driver has a chance to take appropriate device specific actions; for example, it may need to quiesce before the reset, and reinitialize the device after.

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/pci/pci.c   |   13 +++++++++++++
 include/linux/pci.h |    3 +++
 2 files changed, 16 insertions(+)

drivers/pci/pci.c +12 -0

index fdbc294..cb24bbe 100644
@@ -3320,6 +3320,15 @@ static int pci_dev_reset(struct pci_dev *dev, int probe)
 
 	return rc;
 }
+
+static void pci_reset_notify(struct pci_dev *dev, bool prepare)
+{
+	const struct pci_error_handlers *err_handler =
+			dev->driver ? dev->driver->err_handler : NULL;
+	if (err_handler && err_handler->reset_notify)
+		err_handler->reset_notify(dev, prepare);
+}
+
 /**
  * __pci_reset_function - reset a PCI device function
  * @dev: PCI device to reset

@@ -3408,11 +3417,13 @@ int pci_reset_function(struct pci_dev *dev)
 	if (rc)
 		return rc;
 
+	pci_reset_notify(dev, true);
 	pci_dev_save_and_disable(dev);
 
 	rc = pci_dev_reset(dev, 0);
 
 	pci_dev_restore(dev);
+	pci_reset_notify(dev, false);
 
 	return rc;
 }

@@ -3432,6 +3443,7 @@ int pci_try_reset_function(struct pci_dev *dev)
 	if (rc)
 		return rc;
 
+	pci_reset_notify(dev, true);
 	pci_dev_save_and_disable(dev);
 
 	if (pci_dev_trylock(dev)) {

drivers/pci/pci.c +0 -0

 	return rc;
 }

include/linux/pci.h +3 -0

index 33aa2ca..d82dd3f 100644
@@ -603,6 +603,9 @@ struct pci_error_handlers {
 	/* PCI slot has been reset */
 	pci_ers_result_t (*slot_reset)(struct pci_dev *dev);
 
+	/* PCI function reset prepare or completed */
+	void (*reset_notify)(struct pci_dev *dev, bool prepare);
+
 	/* Device driver may resume normal operations */
 	void (*resume)(struct pci_dev *dev);
 };

Signature · 1 line

1.7.10.4

On Tue, Apr 08, 2014@05:42:20PM -0600, Keith Busch wrote:
You put the notify in these functions:

pci_reset_function()
pci_try_reset_function()

but what about these:

pci_reset_slot()
pci_try_reset_slot()
pci_reset_bus()
pci_try_reset_bus()

Quoted 7 lines · Bjorn Helgaas

On Tue, Apr 08, 2014@05:42:20PM -0600, Keith Busch wrote:
You put the notify in these functions:

pci_reset_function()
pci_try_reset_function()

Quoted 6 lines

but what about these:

pci_reset_slot()
pci_try_reset_slot()
pci_reset_bus()
pci_try_reset_bus()

Good point. These all call pci_dev_save_and_disable at the start and pci_dev_restore after, so I think just adding the reset prepare/complete to those functions should cover all scenarios. Does this sound ok?

Quoted 16 lines · Keith Busch

Quoted 13 lines · Bjorn Helgaas

On Tue, Apr 08, 2014@05:42:20PM -0600, Keith Busch wrote:
You put the notify in these functions:

pci_reset_function()
pci_try_reset_function()

but what about these:

pci_reset_slot()
pci_try_reset_slot()
pci_reset_bus()
pci_try_reset_bus()

Quoted 3 lines

Good point. These all call pci_dev_save_and_disable at the start and pci_dev_restore after, so I think just adding the reset prepare/complete to those functions should cover all scenarios. Does this sound ok?

Sound OK to me, as long as the save/restore functions aren't used elsewhere.