Re: [PATCH v4 2/3] remoteproc: xlnx: assign mbox client per mbox channel

["Shah, Tanmay" <tanmays@amd.com>]

On 2/9/2026 5:27 PM, Jassi Brar wrote:
> On Tue, Feb 3, 2026 at 4:59 PM Tanmay Shah <tanmay.shah@amd.com> wrote:
>>
>> Sharing mbox client data structure between "tx" and "rx" channels
>> can lead to data corruption. Instead each channel should have its own
>> mbox client data structure.
>>
>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>> ---
>>
>> changes in v4:
>>   - separate remoteproc driver patch in to two patches
>>
>>  drivers/remoteproc/xlnx_r5_remoteproc.c | 19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
>> index bd619a6c42aa..109831c5815c 100644
>> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
>> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
>> @@ -74,7 +74,8 @@ struct zynqmp_sram_bank {
>>   * @tx_mc_buf: to copy data to mailbox tx channel
>>   * @r5_core: this mailbox's corresponding r5_core pointer
>>   * @mbox_work: schedule work after receiving data from mailbox
>> - * @mbox_cl: mailbox client
>> + * @mbox_tx_cl: tx channel mailbox client
>> + * @mbox_rx_cl: rx channel mailbox client
>>   * @tx_chan: mailbox tx channel
>>   * @rx_chan: mailbox rx channel
>>   */
>> @@ -83,7 +84,8 @@ struct mbox_info {
>>         unsigned char tx_mc_buf[MBOX_CLIENT_BUF_MAX];
>>         struct zynqmp_r5_core *r5_core;
>>         struct work_struct mbox_work;
>> -       struct mbox_client mbox_cl;
>> +       struct mbox_client mbox_tx_cl;
>> +       struct mbox_client mbox_rx_cl;
>>         struct mbox_chan *tx_chan;
>>         struct mbox_chan *rx_chan;
>>  };
>> @@ -230,7 +232,7 @@ static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
>>         struct mbox_info *ipi;
>>         size_t len;
>>
>> -       ipi = container_of(cl, struct mbox_info, mbox_cl);
>> +       ipi = container_of(cl, struct mbox_info, mbox_rx_cl);
>>
>>         /* copy data from ipi buffer to r5_core */
>>         ipi_msg = (struct zynqmp_ipi_message *)msg;
>> @@ -269,8 +271,8 @@ static struct mbox_info *zynqmp_r5_setup_mbox(struct device *cdev)
>>         if (!ipi)
>>                 return NULL;
>>
>> -       mbox_cl = &ipi->mbox_cl;
>> -       mbox_cl->rx_callback = zynqmp_r5_mb_rx_cb;
>> +       mbox_cl = &ipi->mbox_tx_cl;
>> +       mbox_cl->rx_callback = NULL;
>>         mbox_cl->tx_block = false;
>>         mbox_cl->knows_txdone = false;
>>         mbox_cl->tx_done = NULL;
>> @@ -285,6 +287,13 @@ static struct mbox_info *zynqmp_r5_setup_mbox(struct device *cdev)
>>                 return NULL;
>>         }
>>
>> +       mbox_cl = &ipi->mbox_rx_cl;
>> +       mbox_cl->rx_callback = zynqmp_r5_mb_rx_cb;
>> +       mbox_cl->tx_block = false;
>> +       mbox_cl->knows_txdone = false;
>> +       mbox_cl->tx_done = NULL;
>> +       mbox_cl->dev = cdev;
>> +
> hmm... this looks bad. Sorry.
> I think all we need is to export
>   unsigned int mbox_chan_tx_slots_available(struct mbox_chan *chan) {
> return MBOX_TX_QUEUE_LEN - chan->msg_count}
> from mailbox.c, of course with protection of the channel lock.
> That should work for you, right?
> 

Hi Jassi,

Yes. Similar design was proposed in v1 [1]. That patch had some other
problems, but the design was similar.

I see your new patch, that should work for me. I will test the patch and
provide tested-by tag.

Thanks,
Tanmay

[1]
https://lore.kernel.org/all/20250925185043.3013388-1-tanmay.shah@amd.com/#t

> Regards,
> Jassi