Re: [PATCH] accel/amdxdna: Check interrupt register before mailbox_rx_worker exits

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

On 2/26/25 01:30, Jacek Lawrynowicz wrote:
> Hi,
>
> On 2/25/2025 6:26 PM, Lizhi Hou wrote:
>> There is a timeout failure been found during stress tests. If the firmware
>> generates a mailbox response right after driver clears the mailbox channel
>> interrupt register, the hardware will not generate an interrupt for the
>> response. This causes the unexpected mailbox command timeout.
>>
>> To handle this failure, driver checks the interrupt register before
>> exiting mailbox_rx_worker(). If there is a new response, driver goes back
>> to process it.
>>
>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
>> ---
>>   drivers/accel/amdxdna/amdxdna_mailbox.c | 18 +++++++++++++++---
>>   1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/accel/amdxdna/amdxdna_mailbox.c b/drivers/accel/amdxdna/amdxdna_mailbox.c
>> index de7bf0fb4594..efe6cbc44d14 100644
>> --- a/drivers/accel/amdxdna/amdxdna_mailbox.c
>> +++ b/drivers/accel/amdxdna/amdxdna_mailbox.c
>> @@ -348,8 +348,6 @@ static irqreturn_t mailbox_irq_handler(int irq, void *p)
>>   	trace_mbox_irq_handle(MAILBOX_NAME, irq);
>>   	/* Schedule a rx_work to call the callback functions */
>>   	queue_work(mb_chann->work_q, &mb_chann->rx_work);
>> -	/* Clear IOHUB register */
>> -	mailbox_reg_write(mb_chann, mb_chann->iohub_int_addr, 0);
>>   
>>   	return IRQ_HANDLED;
>>   }
>> @@ -357,6 +355,7 @@ static irqreturn_t mailbox_irq_handler(int irq, void *p)
>>   static void mailbox_rx_worker(struct work_struct *rx_work)
>>   {
>>   	struct mailbox_channel *mb_chann;
>> +	u32 iohub;
> There is no need for this variable. Just use if (mailbox_reg_read()).
Sure. Will fix this.
>
>>   	int ret;
>>   
>>   	mb_chann = container_of(rx_work, struct mailbox_channel, rx_work);
>> @@ -366,6 +365,9 @@ static void mailbox_rx_worker(struct work_struct *rx_work)
>>   		return;
>>   	}
>>   
>> +again:
>> +	mailbox_reg_write(mb_chann, mb_chann->iohub_int_addr, 0);
>> +
>>   	while (1) {
>>   		/*
>>   		 * If return is 0, keep consuming next message, until there is
>> @@ -380,9 +382,19 @@ static void mailbox_rx_worker(struct work_struct *rx_work)
>>   			MB_ERR(mb_chann, "Unexpected ret %d, disable irq", ret);
>>   			WRITE_ONCE(mb_chann->bad_state, true);
>>   			disable_irq(mb_chann->msix_irq);
>> -			break;
>> +			return;
> disable_irq() should not be called here. It will be called for the second time in xdna_mailbox_stop_channel() and enable/disable calls should be balanced.

Agree. I will remove disable_irg().

Thanks,

Lizhi

>
>>   		}
>>   	}
>> +
>> +	/*
>> +	 * The hardware will not generate interrupt if firmware creates a new
>> +	 * response right after driver clears interrupt register. Check
>> +	 * the interrupt register to make sure there is not any new response
>> +	 * before exiting.
>> +	 */
>> +	iohub = mailbox_reg_read(mb_chann, mb_chann->iohub_int_addr);
>> +	if (iohub)
>> +		goto again;
>>   }
>>   
>>   int xdna_mailbox_send_msg(struct mailbox_channel *mb_chann,
> Regards,
> Jacek