[PATCH] nvme-apple: Use acquire/release for queue enabled state

[PATCH] nvme-apple: Use acquire/release for queue enabled state

[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

Re: [PATCH] nvme-apple: Use acquire/release for queue enabled state

[Keith Busch]

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?

Re: [PATCH] nvme-apple: Use acquire/release for queue enabled state

[Gui-Dong Han]

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.