Re: [PATCH v1 14/27] drm/panthor: Add AM message helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardware race condition in panthor_am_msg_send() allows mailbox overwrite
- [Medium] gpu_read64() unspecified evaluation order in panthor_am_msg_read()
--

commit f795794def90956d30543ae04795a4ad4e76dd7e
Author: Karunika Choo <karunika.choo@arm.com>

drm/panthor: Add AM message helpers

This commit adds a shared helper layer for the AM_MESSAGE registers used by
v15 GPUs. It defines message IDs, register accessors, and a FIFO-backed
sending mechanism with spinlock serialization.

> diff --git a/drivers/gpu/drm/panthor/panthor_am_msg.h b/drivers/gpu/drm/panthor/panthor_am_msg.h
> new file mode 100644
> index 0000000000000..fc1e3f1051fe5
> --- /dev/null
> +++ b/drivers/gpu/drm/panthor/panthor_am_msg.h
[ ... ]
> +static inline u64 panthor_am_msg_read(struct panthor_am_msg *msg)
> +{
> +	return gpu_read64(msg->iomem, AM_INCOMING_MESSAGE0);
> +}

[Severity: Medium]
Could the unspecified evaluation order of the bitwise OR in gpu_read64()
cause hardware side-effect issues here?

Looking at gpu_read64() in panthor_device_io.h:

static inline u64 gpu_read64(void __iomem *iomem, u32 reg)
{
	return (gpu_read(iomem, reg) | ((u64)gpu_read(iomem, reg + 4) << 32));
}

The C standard leaves the evaluation order of bitwise OR operands unspecified.

If reading AM_INCOMING_MESSAGE0 has side effects (like clearing an interrupt
or popping a message queue), could the compiler evaluating the upper word
read before the lower word read lead to corrupted data or desynchronized
hardware state?

[ ... ]
> +static inline int panthor_am_msg_retry(struct panthor_am_msg *msg)
> +{
> +	u64 message;
> +
> +	guard(spinlock_irqsave)(&msg->lock);
> +
> +	if (kfifo_is_empty(&msg->send_fifo))
> +		return 0;
> +
> +	if (panthor_am_msg_pending(msg))
> +		return -EBUSY;

[Severity: High]
Is it possible for a race condition to occur with the hardware status update
here?

If the workqueue executes rapidly after a previous write,
panthor_am_msg_pending() might read as not busy due to hardware or internal
firmware propagation delays.

If the hardware status hasn't updated yet, could this cause the software to
overwrite the previous message in the mailbox?

> +
> +	/* FIFO should never be empty at this point */
> +	if (!kfifo_get(&msg->send_fifo, &message))
> +		return -EINVAL;
> +
> +	panthor_am_msg_write(msg, message);
> +
> +	/* There are still messages in the FIFO, notify caller to retry again */
> +	if (!kfifo_is_empty(&msg->send_fifo))
> +		return -EAGAIN;
> +
> +	return 0;
> +}
> +
> +static inline int panthor_am_msg_send(struct panthor_am_msg *msg, u64 message)
> +{
> +	guard(spinlock_irqsave)(&msg->lock);
> +
> +	/*
> +	 * If there already is a pending message in the FIFO or the outgoing
> +	 * message is still not read by the receipient, add to the FIFO.
> +	 */
> +	if (!kfifo_is_empty(&msg->send_fifo) || panthor_am_msg_pending(msg)) {

[Severity: High]
Similar to panthor_am_msg_retry(), could back-to-back calls to
panthor_am_msg_send() fail to observe the busy state from a preceding write?

If a CPU sends a message and another CPU (or the same CPU) immediately
acquires the lock to send a second message, the hardware status might
falsely read as 0.

Could this cause the second message to overwrite the first message in the
hardware mailbox before the firmware has processed it, leading to lost
messages?

> +		if (!kfifo_put(&msg->send_fifo, message))
> +			return -ENOSPC;
> +
> +		/*
> +		 * return -EBUSY to indicate to the caller to schedule work to
> +		 * retry sending messages in the FIFO.
> +		 */
> +		return -EBUSY;
> +	}
> +
> +	/* We are free to write to AM_OUTGOING_MESSAGE */
> +	panthor_am_msg_write(msg, message);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922204535.2850094-1-karunika.choo@arm.com?part=14