* [PATCH] nvme-apple: Use acquire/release for queue enabled state
@ 2026-06-03 7:22 Gui-Dong Han
2026-06-03 9:54 ` Keith Busch
0 siblings, 1 reply; 3+ messages in thread
From: Gui-Dong Han @ 2026-06-03 7:22 UTC (permalink / raw)
To: sven, kbusch, linux-nvme
Cc: axboe, hch, sagi, j, neal, asahi, linux-kernel, linux-arm-kernel,
baijiaju1990, Gui-Dong Han
apple_nvme_init_queue() initializes queue state and then marks the queue
enabled. The interrupt and request paths check enabled before using that
queue state.
The existing wmb() after WRITE_ONCE(enabled, true) orders the enabled
store before later queue use, but it does not publish the earlier
initialization before enabled becomes visible.
Use a release store when enabling the queue and acquire loads when
testing it. Keep the existing wmb() in place for the
store-before-later-use ordering.
Fixes: 5bd2927aceba ("nvme-apple: Add initial Apple SoC NVMe driver")
Signed-off-by: Gui-Dong Han <hanguidong02@gmail•com>
---
Found by auditing READ_ONCE() used for synchronization.
A similar fix can be found in 8df672bfe3ec.
---
drivers/nvme/host/apple.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
index c692fc73babf..fcaf8f230b19 100644
--- a/drivers/nvme/host/apple.c
+++ b/drivers/nvme/host/apple.c
@@ -151,6 +151,18 @@ struct apple_nvme_queue {
bool enabled;
};
+static inline bool apple_nvme_queue_enabled(struct apple_nvme_queue *q)
+{
+ /* Pair with apple_nvme_enable_queue(). */
+ return smp_load_acquire(&q->enabled);
+}
+
+static inline void apple_nvme_enable_queue(struct apple_nvme_queue *q)
+{
+ /* Publish queue initialization before setting q->enabled. */
+ smp_store_release(&q->enabled, true);
+}
+
/*
* The apple_nvme_iod describes the data in an I/O.
*
@@ -677,7 +689,7 @@ static bool apple_nvme_handle_cq(struct apple_nvme_queue *q, bool force)
bool found;
DEFINE_IO_COMP_BATCH(iob);
- if (!READ_ONCE(q->enabled) && !force)
+ if (!apple_nvme_queue_enabled(q) && !force)
return false;
found = apple_nvme_poll_cq(q, &iob);
@@ -780,7 +792,7 @@ static blk_status_t apple_nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
* We should not need to do this, but we're still using this to
* ensure we can drain requests on a dying queue.
*/
- if (unlikely(!READ_ONCE(q->enabled)))
+ if (unlikely(!apple_nvme_queue_enabled(q)))
return BLK_STS_IOERR;
if (!nvme_check_ready(&anv->ctrl, req, true))
@@ -1016,7 +1028,7 @@ static void apple_nvme_init_queue(struct apple_nvme_queue *q)
memset(q->tcbs, 0, anv->hw->max_queue_depth
* sizeof(struct apple_nvmmu_tcb));
memset(q->cqes, 0, depth * sizeof(struct nvme_completion));
- WRITE_ONCE(q->enabled, true);
+ apple_nvme_enable_queue(q);
wmb(); /* ensure the first interrupt sees the initialization */
}
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] nvme-apple: Use acquire/release for queue enabled state
2026-06-03 7:22 [PATCH] nvme-apple: Use acquire/release for queue enabled state Gui-Dong Han
@ 2026-06-03 9:54 ` Keith Busch
2026-06-03 11:45 ` Gui-Dong Han
0 siblings, 1 reply; 3+ messages in thread
From: Keith Busch @ 2026-06-03 9:54 UTC (permalink / raw)
To: Gui-Dong Han
Cc: sven, linux-nvme, axboe, hch, sagi, j, neal, asahi, linux-kernel,
linux-arm-kernel, baijiaju1990
On Wed, Jun 03, 2026 at 03:22:05PM +0800, Gui-Dong Han wrote:
> apple_nvme_init_queue() initializes queue state and then marks the queue
> enabled. The interrupt and request paths check enabled before using that
> queue state.
>
> The existing wmb() after WRITE_ONCE(enabled, true) orders the enabled
> store before later queue use, but it does not publish the earlier
> initialization before enabled becomes visible.
>
> Use a release store when enabling the queue and acquire loads when
> testing it. Keep the existing wmb() in place for the
> store-before-later-use ordering.
Doesn't smp_store_release() already get you those semantics?
Also, there are a few other places doing the READ/WRITE_ONCE() calls in
apple_nvme_disable. Do you want to update those too for consistency?
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] nvme-apple: Use acquire/release for queue enabled state
2026-06-03 9:54 ` Keith Busch
@ 2026-06-03 11:45 ` Gui-Dong Han
0 siblings, 0 replies; 3+ messages in thread
From: Gui-Dong Han @ 2026-06-03 11:45 UTC (permalink / raw)
To: Keith Busch
Cc: sven, linux-nvme, axboe, hch, sagi, j, neal, asahi, linux-kernel,
linux-arm-kernel, baijiaju1990
On Wed, Jun 3, 2026 at 5:54 PM Keith Busch <kbusch@kernel•org> wrote:
>
> On Wed, Jun 03, 2026 at 03:22:05PM +0800, Gui-Dong Han wrote:
> > apple_nvme_init_queue() initializes queue state and then marks the queue
> > enabled. The interrupt and request paths check enabled before using that
> > queue state.
> >
> > The existing wmb() after WRITE_ONCE(enabled, true) orders the enabled
> > store before later queue use, but it does not publish the earlier
> > initialization before enabled becomes visible.
> >
> > Use a release store when enabling the queue and acquire loads when
> > testing it. Keep the existing wmb() in place for the
> > store-before-later-use ordering.
>
> Doesn't smp_store_release() already get you those semantics?
Yes, smp_store_release() is enough for the ordering intended by that
comment.
The old wmb() after the store does not really provide that ordering. I
kept it only because it is not identical to a release store, and I was
worried it might have some other ordering effect with later queue-start
operations.
If you prefer, I can drop the wmb() in v2.
>
> Also, there are a few other places doing the READ/WRITE_ONCE() calls in
> apple_nvme_disable. Do you want to update those too for consistency?
For apple_nvme_disable(), I left those READ_ONCE()/WRITE_ONCE() users
unchanged because they are used for the shutdown path rather than for
publishing queue initialization.
The READ_ONCE(anv->ioq.enabled) only decides whether to issue delete
SQ/CQ commands. It does not consume the queue state initialized before
enabled is set.
The WRITE_ONCE(false) users are followed by mb(), which orders the
shutdown/quiesce path. A release store would not replace that.
If you prefer the consistency, I can update them in v2 too.
Thanks.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-06-03 11:45 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-03 7:22 [PATCH] nvme-apple: Use acquire/release for queue enabled state Gui-Dong Han
2026-06-03 9:54 ` Keith Busch
2026-06-03 11:45 ` Gui-Dong Han
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox