Re: [PATCH 4/4] mailbox: riscv-sbi-mpxy: Move to send_data_sync() for sending messages

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] pmdomain: riscv: missing tx_sync causes RPMI PM domain probe failure
- [High] mailbox: riscv-sbi-mpxy: missing IRQ disable causes payload corruption
- [Medium] mailbox: riscv-rpmi: atomic context calls sleepable mbox_send_message_sync()
--

commit 612dfab344d90d3c136efe88349ea5fa93e33818
Author: Anup Patel <anup.patel@oss.qualcomm.com>

mailbox: riscv-sbi-mpxy: Move to send_data_sync() for sending messages

This commit transitions the SBI MPXY mailbox driver from the asynchronous
send_data() callback to the synchronous send_data_sync() callback. It removes
the per-channel spinlock and the manual mbox_client_txdone() call to allow
parallel message sending from multiple CPUs.

> diff --git a/drivers/clk/clk-rpmi.c b/drivers/clk/clk-rpmi.c
> index 921296aafa686..e365cb8a8dac3 100644
> --- a/drivers/clk/clk-rpmi.c
> +++ b/drivers/clk/clk-rpmi.c
> @@ -527,7 +527,7 @@ static int rpmi_clk_probe(struct platform_device *pdev)
>  	context->client.dev		= context->dev;
>  	context->client.rx_callback	= NULL;
>  	context->client.tx_block	= false;
> -	context->client.knows_txdone	= true;
> +	context->client.tx_sync		= true;

[Severity: High]
This updates the clock driver to set tx_sync = true, but was the pmdomain
driver in drivers/pmdomain/riscv/riscv-rpmi-device-power.c missed?

The core mailbox framework in mbox_send_message_sync() returns -EOPNOTSUPP
if MBOX_TXDONE_BY_RETURN is missing (which happens when tx_sync is false).

Since rpmi_pm_domain_probe() still sets knows_txdone = true rather than
tx_sync = true, won't its mailbox messages be rejected during probe, causing
it to fail unconditionally?

>  	context->client.tx_tout		= 0;
>  
>  	context->chan = mbox_request_channel(&context->client, 0);

> [ ... ]

> diff --git a/drivers/mailbox/riscv-sbi-mpxy-mbox.c b/drivers/mailbox/riscv-sbi-mpxy-mbox.c
> index cba95b8406ee9..18d5ed0b15610 100644
> --- a/drivers/mailbox/riscv-sbi-mpxy-mbox.c
> +++ b/drivers/mailbox/riscv-sbi-mpxy-mbox.c
> [ ... ]
> @@ -138,52 +138,54 @@ static void mpxy_mbox_send_rpmi_data(struct mpxy_mbox_channel *mchan,
> [ ... ]
>  		if (!(mchan->attrs.capability & SBI_MPXY_CHAN_CAP_SEND_WITH_RESP)) {
> -			msg->error = -EIO;
> +			error = -EIO;
>  			break;
>  		}
> -		msg->error = sbi_mpxy_send_message_with_resp(mchan->channel_id,
> -							     msg->data.service_id,
> -							     msg->data.request,
> -							     msg->data.request_len,
> -							     msg->data.response,
> -							     msg->data.max_response_len,
> -							     &msg->data.out_response_len);
> +		error = sbi_mpxy_send_message_with_resp(mchan->channel_id,
> +							msg->data.service_id,
> +							msg->data.request,
> +							msg->data.request_len,
> +							msg->data.response,
> +							msg->data.max_response_len,
> +							&msg->data.out_response_len);

[Severity: High]
By moving to mbox_send_message_sync(), we no longer take the channel spinlock
(which safely serialized calls and disabled hard interrupts) before sending
messages.

Inside sbi_mpxy_send_message_with_resp(), mpxy_local_get() uses get_cpu(),
which only disables preemption but leaves local interrupts enabled.

If a hardirq (like rpmi_sysmsi_irq_mask()) preempts a process-context message
on the same CPU, won't both concurrently access the exact same per-CPU
mpxy->shmem buffer?

Could this silently overwrite the interrupted thread's payload and lead to
data corruption in the SBI MPXY payload before it reaches the firmware?

>  		break;
>  	case RPMI_MBOX_MSG_TYPE_SEND_WITHOUT_RESPONSE:

> [ ... ]

> diff --git a/include/linux/mailbox/riscv-rpmi-message.h b/include/linux/mailbox/riscv-rpmi-message.h
> index d5362b5821f95..5269ccf064930 100644
> --- a/include/linux/mailbox/riscv-rpmi-message.h
> +++ b/include/linux/mailbox/riscv-rpmi-message.h
> [ ... ]
> @@ -238,17 +233,8 @@ static inline void *rpmi_mbox_get_msg_response(struct rpmi_mbox_message *msg)
>  static inline int rpmi_mbox_send_message(struct mbox_chan *chan,
>  					 struct rpmi_mbox_message *msg)
>  {
> -	int ret;
> -
> -	/* Send message for the underlying mailbox channel */
> -	ret = mbox_send_message(chan, msg);
> -	if (ret < 0)
> -		return ret;
> -
> -	/* Explicitly signal txdone for mailbox channel */
> -	ret = msg->error;
> -	mbox_client_txdone(chan, ret);
> -	return ret;
> +	/* Send message for the underlying mailbox channel synchronously */
> +	return mbox_send_message_sync(chan, msg);

[Severity: Medium]
The mbox_send_message_sync() API is explicitly documented as a sleepable
function that must not be called from atomic context.

However, rpmi_mbox_send_message() is indirectly called by
rpmi_sysmsi_irq_mask() from the core IRQ framework in an atomic context
with raw spinlocks held.

Although the current riscv-sbi-mpxy controller implementation might not
sleep, could this API contract violation trigger a warning or break if the
mailbox core adds might_sleep() checks?

>  }
>  
>  #endif /* _LINUX_RISCV_RPMI_MESSAGE_H_ */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930150213.799245-1-anup.patel@oss.qualcomm.com?part=4