FAILED: patch "[PATCH] nvme-apple: reset controller during shutdown" failed to apply to 6.1-stable tree
Subsystems (3)
[email protected]
maintainerThe 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 <[email protected]>.
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 <[email protected]>
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.
drivers/nvme/host/apple.c +17 -1
@@ -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);
Janne Grunau
maintainerHej,
Quoted 5 lines · On 2023-02-20 12:40:25 +0100, [email protected] 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 <[email protected]>.
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.
Quoted 3 lines
Possible dependencies:
c06ba7b892a5("nvme-apple: reset controller during shutdown")
certainly doesn't depend on itself
...
Quoted 1 line
c76b8308e4c9("nvme-apple: fix controller shutdown inapple_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
Quoted 57 lines
------------------ original commit in Linus's tree ------------------
From
c06ba7b892a50b48522ad441a40053f483dfee9eMon Sep 17 00:00:00 2001
From: Janne Grunau <[email protected]>
Date: Tue, 17 Jan 2023 19:25:00 +0100
Subject: [PATCH] nvme-apple: reset controller during shutdownThis is a functional revert of
c76b8308e4c9("nvme-apple: fix controller shutdown inapple_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 <[email protected]>Signed-off-by Janne Grunau <[email protected]>[hch: updated with a more descriptive comment from Hector Martin]Signed-off-by Christoph Hellwig <[email protected]>drivers/nvme/host/apple.c +17
-1@@ -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);
Greg KH
maintainerQuoted 18 lines · Janne Grunau
Hej,
Quoted 5 lines · On 2023-02-20 12:40:25 +0100, [email protected] 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 <[email protected]>.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.
Quoted 9 lines
Quoted 3 lines
Possible dependencies:
c06ba7b892a5("nvme-apple: reset controller during shutdown")certainly doesn't depend on itself
...
Quoted 1 line
c76b8308e4c9("nvme-apple: fix controller shutdown inapple_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.
Quoted 7 lines
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
Janne Grunau
maintainerQuoted 23 lines · Greg KH
Quoted 22 lines · Janne Grunau
Hej,
Quoted 5 lines · On 2023-02-20 12:40:25 +0100, [email protected] 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 <[email protected]>.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.
Quoted 3 lines
Possible dependencies:
c06ba7b892a5("nvme-apple: reset controller during shutdown")
Quoted 8 lines
Quoted 5 lines
certainly doesn't depend on itself
...
Quoted 1 line
c76b8308e4c9("nvme-apple: fix controller shutdown inapple_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?
Quoted 10 lines
Quoted 7 lines
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 inapple_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