Re: [PATCH] accel/amdxdna: document trusted mailbox ring geometry and drop the power-of-two check

[Lizhi Hou <lizhi.hou@amd.com>]

On 10/2/26 13:58, Eva Crystal wrote:
> The mailbox ring buffer geometry comes from AMD signed firmware, through
> the management or mailbox information block, or through the CREATE_CONTEXT
> response, and the driver trusts it by design.
>
> The power-of-two test on rb_size was a firmware debugging aid. Nothing in
> the driver derives a mask, a shift or a modulo from rb_size, so no code
> depends on the property it tested. The ring index wrap is handled by the
> explicit comparisons in mailbox_send_msg() and mailbox_get_msg().
>
> Replace the test with a comment that records the design, so that
> automated review does not flag the absence of a bound here again.
>
> Suggested-by: Lizhi Hou <lizhi.hou@amd.com>
> Link: https://lore.kernel.org/all/67698952-6394-44a8-01fe-3e3558b7cb4c@amd.com/
> Signed-off-by: Eva Crystal <0xiviel@gmail.com>
> ---
> The pkg_size check in xdna_mailbox_send_msg() is left in place on
> purpose: it bounds a driver-supplied size, and with Max Zhen's pool
> patch (20261002161122.1350075-1-max.zhen@amd.com) it is the only bound
> on the write into a pre-allocated message slot.
>
>   drivers/accel/amdxdna/amdxdna_mailbox.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_mailbox.c b/drivers/accel/amdxdna/amdxdna_mailbox.c
> index 05c3786de135..c4a36858668f 100644
> --- a/drivers/accel/amdxdna/amdxdna_mailbox.c
> +++ b/drivers/accel/amdxdna/amdxdna_mailbox.c
> @@ -513,11 +513,13 @@ xdna_mailbox_start_channel(struct mailbox_channel *mb_chann,
>   {
>   	int ret;
>   
> -	if (!is_power_of_2(x2i->rb_size) || !is_power_of_2(i2x->rb_size)) {
> -		pr_err("Ring buf size must be power of 2\n");
> -		return -EINVAL;
> -	}
> -
> +	/*
> +	 * The ring buffer geometry, rb_start_addr and rb_size for both the
> +	 * x2i and the i2x channel, comes from AMD signed firmware, through
> +	 * the management or mailbox information block, or through the
> +	 * CREATE_CONTEXT response. The driver trusts those values by design
> +	 * and does not bound them against the mapped region.
> +	 */
Reviewed-by: Lizhi Hou <lizhi.hou@amd.com>
>   	mb_chann->msix_irq = mb_irq;
>   	mb_chann->iohub_int_addr = iohub_int_addr;
>   	memcpy(&mb_chann->res[CHAN_RES_X2I], x2i, sizeof(*x2i));