Re: [PATCH] soundwire: cadence: Clear message complete before signaling waiting thread

[Pierre-Louis Bossart]

On 3/10/26 04:31, Richard Fitzgerald wrote:
> Clear the CDNS_MCP_INT_RX_WL interrupt before signaling completion.
> 
> This is to prevent the potential race where:
> - The main thread is scheduled immediately the completion is signaled,
>    and starts a new message
> - The RX_WL IRQ for this new message happens before sdw_cdns_irq() has
>   been re-scheduled.
> - When sdw_cdns_irq() is re-scheduled it clears the new RX_WL interrupt.
> 
> MAIN THREAD                        |  IRQ THREAD
>                                    |
>   _cdns_xfer_msg()                 |
>   {                                |
>      write data to FIFO            |
>      wait_for_completion_timeout() |
>      <BLOCKED>                     |                       <---- RX_WL IRQ
>                                    | sdw_cdns_irq()
>                                    | {
>                                    |    signal completion
>                           <== RESCHEDULE <==
>   Handle message completion        |
>   }                                |
>                                    |
> Start new message                  |
>   _cdns_xfer_msg()                 |
>   {                                |
>      write data to FIFO            |
>      wait_for_completion_timeout() |
>      <BLOCKED>                     |                       <---- RX_WL IRQ
>                           ==> RESCHEDULE ==>
>                                    |    // New RX_WL IRQ is cleared before
>                                    |    // it has been handled.
>                                    |    clear CDNS_MCP_INTSTAT
> 
>                                    |    return IRQ_HANDLED;
>                                    | }
> 
> Before this change, this error message was sometimes seen on kernels
> that have large amounts of debugging enabled:
> 
>    SCP Msg trf timed out
> 
> This error indicates that the completion has not been signalled after
> 500ms.
> 
> Signed-off-by: Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx>
> Fixes: 956baa1992f9 ("soundwire: cdns: Add sdw_master_ops and IO transfer support")
> Reported-by: Norman Bintang <normanbt@xxxxxxxxxx>
> Closes: https://issuetracker.google.com/issues/477099834

Makes sense to me, nice fix!

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxx>

> ---
>  drivers/soundwire/cadence_master.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> index f245c3ffb9e9..b8b62735c893 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -933,6 +933,14 @@ irqreturn_t sdw_cdns_irq(int irq, void *dev_id)
>  
>  		cdns_read_response(cdns);
>  
> +		/*
> +		 * Clear interrupt before signalling the completion to avoid
> +		 * a race between this thread and the main thread starting
> +		 * another TX.
> +		 */
> +		cdns_writel(cdns, CDNS_MCP_INTSTAT, CDNS_MCP_INT_RX_WL);
> +		int_status &= ~CDNS_MCP_INT_RX_WL;
> +
>  		if (defer && defer->msg) {
>  			cdns_fill_msg_resp(cdns, defer->msg,
>  					   defer->length, 0);