Re: [PATCH 2/2] nvme-apple: Prevent tag collision across queues even if tag space is shared

[Nick Chan <towinchenmi@gmail.com>]

David Laight 於 2026/6/6 晚上10:29 寫道:
> On Sat, 06 Jun 2026 21:25:26 +0800
> Nick Chan <towinchenmi@gmail.com> wrote:
> 
>> From: Yuriy Havrylyuk <yhavry@gmail.com>
>>
>> Apple NVMe controllers require tags of pending commands to not be shared
>> across admin and IO queues. However, on Apple A11 without linear SQ, it is
>> not possible for either queue to skip over some tags and must go from 0 to
>> the configured maximum before wrapping around.
>>
>> If a pending command tag is duplicated across queues, the firmware
>> crashes with: "duplicate tag error for tag N", with N being the tag.
>>
>> Instead of partitioning the tag space, which is not possible without
>> linear SQ, prevent tag collisions by keeping track of which tags are
>> currently in-flight across either queues, and return BLK_STS_RESOURCE to
>> temporaily block command submission when a collision would have occurred.
> 
> I look at using the atomic64_xxx() functions rather than the bitmask ones.
> The for_each_bit_set() loop is then an atmomic64_andnot() call.

That does in fact simplify the loop code. However, using the atomic function
complicates the apple_nvme_reserve_tag_t8015() and
apple_nvme_release_tag_t8015() since those functions deal with set/clear a
a bit in terms of an integer (the tag).

Especially in the apple_nvme_reserve_tag_t8015() function the function body
is then

	u64 tag_bit = BIT(nvme_tag_from_cid(cmd->common.command_id));

	return !(atomic64_fetch_or(tag_bit, &anv->t8015_active_tags) & tag_bit);

The function would need to explictly convert the tag to a bit, and then
explictly extract the bit value after performing the atomic operation,
both which of could have been done by test_and_set_bit().

So I do not see any overall benefit for using atomic_xxx() functions.

Best Regards,
Nick Chan

> 
> -- David
> 
> 
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 04d8ecf37b5e ("nvme: apple: Add Apple A11 support")
>> Signed-off-by: Yuriy Havrylyuk <yhavry@gmail.com>
>> Co-developed-by: Nick Chan <towinchenmi@gmail.com>
>> Signed-off-by: Nick Chan <towinchenmi@gmail.com>
>> ---
>>  drivers/nvme/host/apple.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 65 insertions(+)
>>
>> diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
>> index c1115e27a0d6..6354edf27225 100644
>> --- a/drivers/nvme/host/apple.c
>> +++ b/drivers/nvme/host/apple.c
>> @@ -203,6 +203,20 @@ struct apple_nvme {
>>  
>>  	int irq;
>>  	spinlock_t lock;
>> +
>> +	/*
>> +	 * Tags of pending commands must be unique across both Admin and IO
>> +	 * queue. However, on T8015, unlike T8103, without linear submission
>> +	 * queues, it is not possible for the either queue to skip some tags,
>> +	 * and both queues must go from 0 to their respective configured
>> +	 * maximum.
>> +	 *
>> +	 * Instead of reserving some tags for the admin queue, use a bitfield
>> +	 * to keep track of pending commands on either queue, and temporaily
>> +	 * block command submission by returning BLK_STS_RESOURCE until the
>> +	 * tag is freed on the other queue.
>> +	 */
>> +	unsigned long t8015_active_tags;
>>  };
>>  
>>  static_assert(sizeof(struct nvme_command) == 64);
>> @@ -290,6 +304,28 @@ static void apple_nvmmu_inval(struct apple_nvme_queue *q, unsigned int tag)
>>  				     "NVMMU TCB invalidation failed\n");
>>  }
>>  
>> +static bool apple_nvme_reserve_tag_t8015(struct apple_nvme *anv,
>> +					 struct nvme_command *cmd)
>> +{
>> +	u16 tag = nvme_tag_from_cid(cmd->common.command_id);
>> +
>> +	if (WARN_ON_ONCE(tag >= BITS_PER_LONG))
>> +		return false;
>> +
>> +	return !test_and_set_bit(tag, &anv->t8015_active_tags);
>> +}
>> +
>> +static void apple_nvme_release_tag_t8015(struct apple_nvme *anv,
>> +					 __u16 command_id)
>> +{
>> +	u16 tag = nvme_tag_from_cid(command_id);
>> +
>> +	if (WARN_ON_ONCE(tag >= BITS_PER_LONG))
>> +		return;
>> +
>> +	clear_bit(tag, &anv->t8015_active_tags);
>> +}
>> +
>>  static void apple_nvme_submit_cmd_t8015(struct apple_nvme_queue *q,
>>  				  struct nvme_command *cmd)
>>  {
>> @@ -652,6 +688,8 @@ static inline void apple_nvme_update_cq_head(struct apple_nvme_queue *q)
>>  static bool apple_nvme_poll_cq(struct apple_nvme_queue *q,
>>  			       struct io_comp_batch *iob)
>>  {
>> +	struct apple_nvme *anv = queue_to_apple_nvme(q);
>> +	unsigned long completed_tags = 0;
>>  	bool found = false;
>>  
>>  	while (apple_nvme_cqe_pending(q)) {
>> @@ -664,11 +702,26 @@ static bool apple_nvme_poll_cq(struct apple_nvme_queue *q,
>>  		dma_rmb();
>>  		apple_nvme_handle_cqe(q, iob, q->cq_head);
>>  		apple_nvme_update_cq_head(q);
>> +
>> +		if (!anv->hw->has_lsq_nvmmu) {
>> +			struct nvme_completion *cqe = &q->cqes[q->cq_head];
>> +			u16 tag = nvme_tag_from_cid(READ_ONCE(cqe->command_id));
>> +
>> +			if (!WARN_ON_ONCE(tag >= BITS_PER_LONG))
>> +				__set_bit(tag, &completed_tags);
>> +		}
>>  	}
>>  
>>  	if (found)
>>  		writel(q->cq_head, q->cq_db);
>>  
>> +	if (!anv->hw->has_lsq_nvmmu && completed_tags) {
>> +		unsigned long tag_bit;
>> +
>> +		for_each_set_bit(tag_bit, &completed_tags, BITS_PER_LONG)
>> +			clear_bit(tag_bit, &anv->t8015_active_tags);
>> +	}
>> +
>>  	return found;
>>  }
>>  
>> @@ -790,6 +843,12 @@ static blk_status_t apple_nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
>>  	if (ret)
>>  		return ret;
>>  
>> +	if (!anv->hw->has_lsq_nvmmu &&
>> +	    !apple_nvme_reserve_tag_t8015(anv, cmnd)) {
>> +		ret = BLK_STS_RESOURCE;
>> +		goto out_free_cmd;
>> +	}
>> +
>>  	if (blk_rq_nr_phys_segments(req)) {
>>  		ret = apple_nvme_map_data(anv, req, cmnd);
>>  		if (ret)
>> @@ -806,6 +865,9 @@ static blk_status_t apple_nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
>>  	return BLK_STS_OK;
>>  
>>  out_free_cmd:
>> +	if (!anv->hw->has_lsq_nvmmu)
>> +		apple_nvme_release_tag_t8015(anv, cmnd->common.command_id);
>> +
>>  	nvme_cleanup_cmd(req);
>>  	return ret;
>>  }
>> @@ -1165,6 +1227,9 @@ static void apple_nvme_reset_work(struct work_struct *work)
>>  	if (ret)
>>  		goto out;
>>  
>> +	if (!anv->hw->has_lsq_nvmmu)
>> +		WRITE_ONCE(anv->t8015_active_tags, 0);
>> +
>>  	dev_dbg(anv->dev, "Starting admin queue");
>>  	apple_nvme_init_queue(&anv->adminq);
>>  	nvme_unquiesce_admin_queue(&anv->ctrl);
>>
>