FAILED: patch "[PATCH] nvme-apple: reset controller during shutdown" failed to apply to 6.1-stable tree

FAILED: patch "[PATCH] nvme-apple: reset controller during shutdown" failed to apply to 6.1-stable tree

[gregkh]

The patch below does not apply to the 6.1-stable tree.
If someone wants it applied there, or to any other stable or longterm
tree, then please email the backport, including the original git commit
id to <stable@vger.kernel.org>.

Possible dependencies:

c06ba7b892a5 ("nvme-apple: reset controller during shutdown")
285b6e9b5717 ("nvme: merge nvme_shutdown_ctrl into nvme_disable_ctrl")
e6d275de2e4a ("nvme: use nvme_wait_ready in nvme_shutdown_ctrl")
c76b8308e4c9 ("nvme-apple: fix controller shutdown in apple_nvme_disable")
9f27bd701d18 ("nvme: rename the queue quiescing helpers")
91c11d5f3254 ("nvme-rdma: stop auth work after tearing down queues in error recovery")
1f1a4f89562d ("nvme-tcp: stop auth work after tearing down queues in error recovery")
eac3ef262941 ("nvme-pci: split the initial probe from the rest path")
a6ee7f19ebfd ("nvme-pci: call nvme_pci_configure_admin_queue from nvme_pci_enable")
3f30a79c2e2c ("nvme-pci: set constant paramters in nvme_pci_alloc_ctrl")
2e87570be9d2 ("nvme-pci: factor out a nvme_pci_alloc_dev helper")
081a7d958ce4 ("nvme-pci: factor the iod mempool creation into a helper")
94cc781f69f4 ("nvme: move OPAL setup from PCIe to core")
cd50f9b24726 ("nvme: split nvme_kill_queues")
6bcd5089ee13 ("nvme: don't unquiesce the admin queue in nvme_kill_queues")
0ffc7e98bfaa ("nvme-pci: refactor the tagset handling in nvme_reset_work")
71b26083d59c ("block: set the disk capacity to 0 in blk_mark_disk_dead")

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From c06ba7b892a50b48522ad441a40053f483dfee9e Mon Sep 17 00:00:00 2001
From: Janne Grunau <j@jannau.net>
Date: Tue, 17 Jan 2023 19:25:00 +0100
Subject: [PATCH] nvme-apple: reset controller during shutdown

This is a functional revert of c76b8308e4c9 ("nvme-apple: fix controller
shutdown in apple_nvme_disable").

The commit broke suspend/resume since apple_nvme_reset_work() tries to
disable the controller on resume. This does not work for the apple NVMe
controller since register access only works while the co-processor
firmware is running.

Disabling the NVMe controller in the shutdown path is also required
for shutting the co-processor down. The original code was appropriate
for this hardware. Add a comment to prevent a similar breaking changes
in the future.

Fixes: c76b8308e4c9 ("nvme-apple: fix controller shutdown in apple_nvme_disable")
Reported-by: Janne Grunau <j@jannau.net>
Link: https://lore.kernel.org/all/20230110174745.GA3576@jannau.net/
Signed-off-by: Janne Grunau <j@jannau.net>
[hch: updated with a more descriptive comment from Hector Martin]
Signed-off-by: Christoph Hellwig <hch@lst.de>

diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
index bf1c60edb7f9..146c9e63ce77 100644
--- a/drivers/nvme/host/apple.c
+++ b/drivers/nvme/host/apple.c
@@ -829,7 +829,23 @@ static void apple_nvme_disable(struct apple_nvme *anv, bool shutdown)
 			apple_nvme_remove_cq(anv);
 		}
 
-		nvme_disable_ctrl(&anv->ctrl, shutdown);
+		/*
+		 * Always disable the NVMe controller after shutdown.
+		 * We need to do this to bring it back up later anyway, and we
+		 * can't do it while the firmware is not running (e.g. in the
+		 * resume reset path before RTKit is initialized), so for Apple
+		 * controllers it makes sense to unconditionally do it here.
+		 * Additionally, this sequence of events is reliable, while
+		 * others (like disabling after bringing back the firmware on
+		 * resume) seem to run into trouble under some circumstances.
+		 *
+		 * Both U-Boot and m1n1 also use this convention (i.e. an ANS
+		 * NVMe controller is handed off with firmware shut down, in an
+		 * NVMe disabled state, after a clean shutdown).
+		 */
+		if (shutdown)
+			nvme_disable_ctrl(&anv->ctrl, shutdown);
+		nvme_disable_ctrl(&anv->ctrl, false);
 	}
 
 	WRITE_ONCE(anv->ioq.enabled, false);

Re: FAILED: patch "[PATCH] nvme-apple: reset controller during shutdown" failed to apply to 6.1-stable tree

[Janne Grunau]

Hej,

On 2023-02-20 12:40:25 +0100, gregkh@linuxfoundation.org wrote:
> 
> The patch below does not apply to the 6.1-stable tree.
> If someone wants it applied there, or to any other stable or longterm
> tree, then please email the backport, including the original git commit
> id to <stable@vger.kernel.org>.

Why was this patch even considered for the 6.1-stable tree? Its "Fixes:" 
tag references a commit which only appeared in v6.2-rc1.

There is no need to backport this to any stable tree.

The message suggest that ignoring this would have been fine but it seems 
worthwhile to report a potential bug or ommission in the stable backport 
tooling.

> Possible dependencies:
> 
> c06ba7b892a5 ("nvme-apple: reset controller during shutdown")

certainly doesn't depend on itself

...

> c76b8308e4c9 ("nvme-apple: fix controller shutdown in apple_nvme_disable")

commit from "Fixes:"

...

If this is intended since "Fixes:" tags are in general not relieable 
enough to decide to which trees regression fixes have to be applied to 
that's acceptable

ciao

Janne

> ------------------ original commit in Linus's tree ------------------
> 
> From c06ba7b892a50b48522ad441a40053f483dfee9e Mon Sep 17 00:00:00 2001
> From: Janne Grunau <j@jannau.net>
> Date: Tue, 17 Jan 2023 19:25:00 +0100
> Subject: [PATCH] nvme-apple: reset controller during shutdown
> 
> This is a functional revert of c76b8308e4c9 ("nvme-apple: fix controller
> shutdown in apple_nvme_disable").
> 
> The commit broke suspend/resume since apple_nvme_reset_work() tries to
> disable the controller on resume. This does not work for the apple NVMe
> controller since register access only works while the co-processor
> firmware is running.
> 
> Disabling the NVMe controller in the shutdown path is also required
> for shutting the co-processor down. The original code was appropriate
> for this hardware. Add a comment to prevent a similar breaking changes
> in the future.
> 
> Fixes: c76b8308e4c9 ("nvme-apple: fix controller shutdown in apple_nvme_disable")
> Reported-by: Janne Grunau <j@jannau.net>
> Link: https://lore.kernel.org/all/20230110174745.GA3576@jannau.net/
> Signed-off-by: Janne Grunau <j@jannau.net>
> [hch: updated with a more descriptive comment from Hector Martin]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
> index bf1c60edb7f9..146c9e63ce77 100644
> --- a/drivers/nvme/host/apple.c
> +++ b/drivers/nvme/host/apple.c
> @@ -829,7 +829,23 @@ static void apple_nvme_disable(struct apple_nvme *anv, bool shutdown)
>  			apple_nvme_remove_cq(anv);
>  		}
>  
> -		nvme_disable_ctrl(&anv->ctrl, shutdown);
> +		/*
> +		 * Always disable the NVMe controller after shutdown.
> +		 * We need to do this to bring it back up later anyway, and we
> +		 * can't do it while the firmware is not running (e.g. in the
> +		 * resume reset path before RTKit is initialized), so for Apple
> +		 * controllers it makes sense to unconditionally do it here.
> +		 * Additionally, this sequence of events is reliable, while
> +		 * others (like disabling after bringing back the firmware on
> +		 * resume) seem to run into trouble under some circumstances.
> +		 *
> +		 * Both U-Boot and m1n1 also use this convention (i.e. an ANS
> +		 * NVMe controller is handed off with firmware shut down, in an
> +		 * NVMe disabled state, after a clean shutdown).
> +		 */
> +		if (shutdown)
> +			nvme_disable_ctrl(&anv->ctrl, shutdown);
> +		nvme_disable_ctrl(&anv->ctrl, false);
>  	}
>  
>  	WRITE_ONCE(anv->ioq.enabled, false);
> 

Re: FAILED: patch "[PATCH] nvme-apple: reset controller during shutdown" failed to apply to 6.1-stable tree

[Greg KH]

On Mon, Feb 20, 2023 at 05:45:59PM +0100, Janne Grunau wrote:
> Hej,
> 
> On 2023-02-20 12:40:25 +0100, gregkh@linuxfoundation.org wrote:
> > 
> > The patch below does not apply to the 6.1-stable tree.
> > If someone wants it applied there, or to any other stable or longterm
> > tree, then please email the backport, including the original git commit
> > id to <stable@vger.kernel.org>.
> 
> Why was this patch even considered for the 6.1-stable tree? Its "Fixes:" 
> tag references a commit which only appeared in v6.2-rc1.
> 
> There is no need to backport this to any stable tree.
> 
> The message suggest that ignoring this would have been fine but it seems 
> worthwhile to report a potential bug or ommission in the stable backport 
> tooling.
> 
> > Possible dependencies:
> > 
> > c06ba7b892a5 ("nvme-apple: reset controller during shutdown")
> 
> certainly doesn't depend on itself
> 
> ...
> 
> > c76b8308e4c9 ("nvme-apple: fix controller shutdown in apple_nvme_disable")

This is the commit that is now in the 6.1-stable queue, which is why I
sent the "FAILED" email for the commit that said this was a fix.

> commit from "Fixes:"
> 
> ...
> 
> If this is intended since "Fixes:" tags are in general not relieable 
> enough to decide to which trees regression fixes have to be applied to 
> that's acceptable

Should I just drop c76b8308e4c9 ("nvme-apple: fix controller shutdown in
apple_nvme_disable") instead?

thanks,

greg k-h

Re: FAILED: patch "[PATCH] nvme-apple: reset controller during shutdown" failed to apply to 6.1-stable tree

[Janne Grunau]

On 2023-02-20 19:09:22 +0100, Greg KH wrote:
> On Mon, Feb 20, 2023 at 05:45:59PM +0100, Janne Grunau wrote:
> > Hej,
> > 
> > On 2023-02-20 12:40:25 +0100, gregkh@linuxfoundation.org wrote:
> > > 
> > > The patch below does not apply to the 6.1-stable tree.
> > > If someone wants it applied there, or to any other stable or longterm
> > > tree, then please email the backport, including the original git commit
> > > id to <stable@vger.kernel.org>.
> > 
> > Why was this patch even considered for the 6.1-stable tree? Its "Fixes:" 
> > tag references a commit which only appeared in v6.2-rc1.
> > 
> > There is no need to backport this to any stable tree.
> > 
> > The message suggest that ignoring this would have been fine but it seems 
> > worthwhile to report a potential bug or ommission in the stable backport 
> > tooling.
> > 
> > > Possible dependencies:
> > > 
> > > c06ba7b892a5 ("nvme-apple: reset controller during shutdown")
> > 
> > certainly doesn't depend on itself
> > 
> > ...
> > 
> > > c76b8308e4c9 ("nvme-apple: fix controller shutdown in apple_nvme_disable")
> 
> This is the commit that is now in the 6.1-stable queue, which is why I
> sent the "FAILED" email for the commit that said this was a fix.

I didn't think of that, I wouldn't have considered c76b8308e4c9 as a 
candidate for stable. At best it would have avoided some unnecessary 
work. Did it get queued for stanble due to the "fix" in the commit 
message?

> > commit from "Fixes:"
> > 
> > ...
> > 
> > If this is intended since "Fixes:" tags are in general not relieable 
> > enough to decide to which trees regression fixes have to be applied to 
> > that's acceptable
> 
> Should I just drop c76b8308e4c9 ("nvme-apple: fix controller shutdown in
> apple_nvme_disable") instead?

yes, I think dropping c76b8308e4c9 ("nvme-apple: fix controller shutdown 
apple_nvme_disable") from stable is the best solution.

c06ba7b892a5 ("nvme-apple: reset controller during shutdown") was 
functionally a revert. It wasn't a clean revert since 285b6e9b5717 
("nvme: merge nvme_shutdown_ctrl into nvme_disable_ctrl") modified the 
same lines substantially.

thanks

Janne