Hej,
On 2023-02-20 12:40:25 +0100, gregkh@xxxxxxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxx>.
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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> Link: https://lore.kernel.org/all/20230110174745.GA3576@xxxxxxxxxx/
> Signed-off-by: Janne Grunau <j@xxxxxxxxxx>
> [hch: updated with a more descriptive comment from Hector Martin]
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
>
> 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);
>
