Re: [PATCH V1] accel/amdxdna: eliminate GFP_KERNEL allocation from mailbox send path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] amdxdna: ABA vulnerability in msg_id accepts stale firmware responses
- [High] amdxdna: management channel bottleneck causes concurrent -ENOBUFS failures
- [Medium] amdxdna: xdna_mailbox_wait_ack() misses completion due to TOCTOU on tail
--

commit ea210dde67519c35a295be58a57ebd45f0948e98
Author: Max Zhen <max.zhen@amd.com>

accel/amdxdna: eliminate GFP_KERNEL allocation from mailbox send path

This patch replaces the per-message dynamic allocations with a pre-allocated
pool of N slots. The message ID is updated to encode the slot index directly
for O(1) lookup, and the management channel size is bound statically based on
the maximum number of asynchronous events plus one.

> --- a/drivers/accel/amdxdna/aie2_pci.c
> +++ b/drivers/accel/amdxdna/aie2_pci.c
> @@ -385,11 +386,16 @@ static int aie2_hw_start(struct amdxdna_dev *xdna)
>  	}
>  
>  	xdna_mailbox_intr_reg = ndev->aie.mgmt_i2x.mb_head_ptr_reg + 4;
> +	/*
> +	 * At any given time, at most AMDXDNA_MAX_ASYNC_EVENT_BUFS async event
> +	 * messages plus 1 other management command can be unresponded.
> +	 */
>  	ret = xdna_mailbox_start_channel(ndev->aie.mgmt_chann,
>  					 &ndev->aie.mgmt_x2i,
>  					 &ndev->aie.mgmt_i2x,
>  					 xdna_mailbox_intr_reg,
> -					 mgmt_mb_irq);
> +					 mgmt_mb_irq,
> +					 AMDXDNA_MAX_ASYNC_EVENT_BUFS + 1);

[Severity: High]
Could this hardcoded capacity lead to spurious -ENOBUFS failures for userspace?

Since 4 of the 5 slots are permanently consumed by asynchronous event buffers
waiting for firmware errors, only 1 slot is left for other management commands.
If two threads execute concurrent IOCTLs that issue management commands, won't
the second thread immediately fail with -ENOBUFS?

> --- a/drivers/accel/amdxdna/amdxdna_mailbox.c
> +++ b/drivers/accel/amdxdna/amdxdna_mailbox.c
[ ... ]
> @@ -248,8 +297,9 @@ mailbox_get_resp(struct mailbox_channel *mb_chann, struct xdna_msg_header *heade
>  		 void __iomem *data)
>  {
[ ... ]
> -	msg_id &= ~MAGIC_VAL_MASK;
> -	mb_msg = xa_erase_irq(&mb_chann->chan_xa, msg_id);
> -	if (!mb_msg) {
> -		MB_ERR(mb_chann, "Cannot find msg 0x%x", msg_id);
> +	slot = msg_id & ~MAGIC_VAL_MASK;
> +	if (unlikely(slot >= mb_chann->msg_buf_num)) {
> +		MB_ERR(mb_chann, "Invalid msg_id 0x%x", msg_id);
>  		return -EINVAL;
>  	}
> +	mb_msg = mailbox_msg_ptr(mb_chann, slot);
>  
>  	MB_DBG(mb_chann, "opcode 0x%x size %d id 0x%x",
>  	       header->opcode, header->total_size, header->id);
> -	if (mb_msg->notify_cb) {
> -		ret = mb_msg->notify_cb(mb_msg->handle, data, header->total_size);
> -		if (unlikely(ret))
> -			MB_ERR(mb_chann, "Message callback ret %d", ret);
> +
> +	if (unlikely(!mb_msg->busy)) {
> +		MB_WARN_ONCE(mb_chann, "Unexpected response for idle slot 0x%x", msg_id);
> +		return 0;
>  	}

[Severity: High]
Does replacing xa_alloc_cyclic_irq with a static ID using slot | MAGIC_VAL
introduce an ABA vulnerability here?

If a stale or duplicate firmware response arrives after a slot has been
reused for a completely new message, mb_msg->busy will be true again.
Since the msg_id no longer contains a sequence number to differentiate
between the old and new message, won't this allow a stale firmware
response to bypass the check and invoke the wrong callback with invalid
data?

[ ... ]
> @@ -398,8 +451,33 @@ static void mailbox_rx_worker(struct work_struct *rx_work)
>  		goto again;
>  }
>  
> +static int xdna_mailbox_wait_ack(struct mailbox_channel *mb_chann, u64 tx_timeout_ms)
> +{
> +	u32 tail = mb_chann->x2i_tail;
> +	u32 head;
> +	int ret;
> +
> +	/*
> +	 * Poll until firmware advances the head pointer past our message,
> +	 * confirming it has consumed (acknowledged) the send.
> +	 */
> +	ret = read_poll_timeout(mailbox_get_headptr, head,
> +				head == tail, 1000, tx_timeout_ms * 1000,
> +				false, mb_chann, CHAN_RES_X2I);

[Severity: Medium]
Can this polling loop stall for the full timeout duration due to a race
condition on the tail pointer?

The tail pointer is read outside the lock and might reflect a subsequent
message's tail if another thread concurrently sends a message. When the
firmware processes both messages, the head pointer will advance past the
expected tail.

Since the completion condition uses strict equality (head == tail), won't it
permanently fail and cause the caller to stall until timeout if the head
overtakes the captured tail?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930164704.1049735-1-max.zhen@amd.com?part=1