[PATCH] nvme-pci: let platform handle subsystem reset fallout

[PATCH] nvme-pci: let platform handle subsystem reset fallout

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Scheduling reset_work after a nvme subsystem reset is expected to fail,
but this also prevents potential handling the platform may provide from
successfully recovering the link without re-enumeration. Provide a pci
specific operation that safely initiates a subsystem reset, and instead
of scheduling reset work, read back the status register to trigger a
pcie read error.

Reported-by: Nilay Shroff <nilay@linux.ibm.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/nvme.h |  3 +++
 drivers/nvme/host/pci.c  | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 68b400f9c42d5..f581cb61a34d2 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -551,6 +551,7 @@ struct nvme_ctrl_ops {
 	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 *val);
 	void (*free_ctrl)(struct nvme_ctrl *ctrl);
 	void (*submit_async_event)(struct nvme_ctrl *ctrl);
+	int (*subsystem_reset)(struct nvme_ctrl *ctrl);
 	void (*delete_ctrl)(struct nvme_ctrl *ctrl);
 	void (*stop_ctrl)(struct nvme_ctrl *ctrl);
 	int (*get_address)(struct nvme_ctrl *ctrl, char *buf, int size);
@@ -653,6 +654,8 @@ static inline int nvme_reset_subsystem(struct nvme_ctrl *ctrl)
 
 	if (!ctrl->subsystem)
 		return -ENOTTY;
+	if (ctrl->ops->subsystem_reset)
+		return ctrl->ops->subsystem_reset(ctrl);
 	if (!nvme_wait_reset(ctrl))
 		return -EBUSY;
 
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 102a9fb0c65ff..4465e06c5583b 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1143,6 +1143,37 @@ static void nvme_pci_submit_async_event(struct nvme_ctrl *ctrl)
 	spin_unlock(&nvmeq->sq_lock);
 }
 
+static int nvme_pci_subsystem_reset(struct nvme_ctrl *ctrl)
+{
+	struct nvme_dev *dev = to_nvme_dev(ctrl);
+	int ret = 0;
+
+	/*
+	 * Taking the shutdown_lock ensures the iomap is not being altered by
+	 * reset_work. Holding this lock before the RESETTING state change, if
+	 * successful, also ensures nvme_remove won't be able to proceed to
+	 * iounmap until we're done.
+	 */
+	mutex_lock(&dev->shutdown_lock);
+	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING)) {
+		ret = -EBUSY;
+		goto unlock;
+	}
+
+	if (!dev->bar_mapped_size) {
+		ret = -ENODEV;
+		goto unlock;
+	}
+
+	writel(0x4E564D65, dev->bar + NVME_REG_NSSR);
+
+	/* Read back to trigger platform error handling, if any */
+	readl(dev->bar + NVME_REG_CSTS);
+unlock:
+	mutex_unlock(&dev->shutdown_lock);
+	return ret;
+}
+
 static int adapter_delete_queue(struct nvme_dev *dev, u8 opcode, u16 id)
 {
 	struct nvme_command c = { };
@@ -2859,6 +2890,7 @@ static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = {
 	.reg_read64		= nvme_pci_reg_read64,
 	.free_ctrl		= nvme_pci_free_ctrl,
 	.submit_async_event	= nvme_pci_submit_async_event,
+	.subsystem_reset	= nvme_pci_subsystem_reset,
 	.get_address		= nvme_pci_get_address,
 	.print_device_info	= nvme_pci_print_device_info,
 	.supports_pci_p2pdma	= nvme_pci_supports_pci_p2pdma,
-- 
2.43.0

Re: [PATCH] nvme-pci: let platform handle subsystem reset fallout

[Christoph Hellwig]

On Mon, Jun 24, 2024 at 09:07:56AM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Scheduling reset_work after a nvme subsystem reset is expected to fail,
> but this also prevents potential handling the platform may provide from
> successfully recovering the link without re-enumeration. Provide a pci
> specific operation that safely initiates a subsystem reset, and instead
> of scheduling reset work, read back the status register to trigger a
> pcie read error.

What does platform mean here?

> @@ -653,6 +654,8 @@ static inline int nvme_reset_subsystem(struct nvme_ctrl *ctrl)
>  
>  	if (!ctrl->subsystem)
>  		return -ENOTTY;
> +	if (ctrl->ops->subsystem_reset)
> +		return ctrl->ops->subsystem_reset(ctrl);
>  	if (!nvme_wait_reset(ctrl))
>  		return -EBUSY;

Branching out into a method but having the default inline here
without any comment explaining it feels weird.

If apple nvme devices supported subsystems resets, the PCIe version
would probably the right thing to do for them as well, but my guess
is they don't anyway.  So maybe return -ENOTTY if no method is
weird up, and turn the old generic one into nvmf_subsystem_reset in
fabrics.c?

> +	/*
> +	 * Taking the shutdown_lock ensures the iomap is not being altered by

s/iomap/BAR mapping/ ?

> +	writel(0x4E564D65, dev->bar + NVME_REG_NSSR);

And now that we're duplicating this constant it could really use a
symbolic name.

> +	/* Read back to trigger platform error handling, if any */
> +	readl(dev->bar + NVME_REG_CSTS);

.. also to flush the posted write above.

Re: [PATCH] nvme-pci: let platform handle subsystem reset fallout

[Keith Busch]

On Mon, Jun 24, 2024 at 06:15:58PM +0200, Christoph Hellwig wrote:
> On Mon, Jun 24, 2024 at 09:07:56AM -0700, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > Scheduling reset_work after a nvme subsystem reset is expected to fail,
> > but this also prevents potential handling the platform may provide from
> > successfully recovering the link without re-enumeration. Provide a pci
> > specific operation that safely initiates a subsystem reset, and instead
> > of scheduling reset work, read back the status register to trigger a
> > pcie read error.
> 
> What does platform mean here?

I'm thinking of pcie port serices: AER, DPC, or hotplug. Nilay's power
platfrom has their own proprietary mechanism called EEH.

If you're platform has none of these, you'll have to wait for an IO
timeout for the driver to try to do anything to recover. Before this
patch, the driver would just quickly unbind.