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 <[email protected]> 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/[email protected]?part=14
