Re: [PATCH net] net: emac: mal: fix W1C write race in ICINTSTAT clearing

[David Gibson <david@gibson.dropbear.id.au>]

On Thu, Jul 02, 2026 at 04:49:23PM -0700, Rosen Penev wrote:
> The ICINTSTAT register is write-1-to-clear (W1C).  The read-modify-write
> pattern in both mal_txeob() and mal_rxeob() can lose interrupts: if a bit
> that should not be cleared is already asserted when mfdcri() reads the
> register, it is included in the read value, retained by the bitwise OR, and
> then written back as 1 - inadvertently clearing a pending but unhandled
> interrupt.
> 
> Fix by writing only the specific bit to clear (ICINTSTAT_ICTX for TXEOB,
> ICINTSTAT_ICRX for RXEOB).  W1C semantics guarantee that writing 0 to the
> other bits has no effect.

Wow, it's a long time since I thought about the MAL.

> Fixes: 1d3bb996481e ("Device tree aware EMAC driver")

This doesn't appear correct.  The lines in question were added by
fbcc4bacee30c ("ibm_newemac: MAL support for PowerPC 405EZ")

> Assisted-by: opencode:big-pickle
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

Assuming ICINTSTAT is indeed a W1C register (or "read/clear" as I
believe they were termed in the 405 documentation) the change looks
correct.  However, I no longer have access to the documentation that
would let me verify that.  I would absolutely not trust an LLM to know
if that's the case, since it's a fairly arbitrary and specific detail
of an obscure CPU.  That said, that the previous code has an | rather
than &~ and presumably at least somewhat worked does suggest it's
read/clear rather than plain read/write.

> ---
>  drivers/net/ethernet/ibm/emac/mal.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/emac/mal.c b/drivers/net/ethernet/ibm/emac/mal.c
> index 4025bc36ae16..eab7a487bf08 100644
> --- a/drivers/net/ethernet/ibm/emac/mal.c
> +++ b/drivers/net/ethernet/ibm/emac/mal.c
> @@ -282,8 +282,7 @@ static irqreturn_t mal_txeob(int irq, void *dev_instance)
>  
>  #ifdef CONFIG_PPC_DCR_NATIVE
>  	if (mal_has_feature(mal, MAL_FTR_CLEAR_ICINTSTAT))
> -		mtdcri(SDR0, DCRN_SDR_ICINTSTAT,
> -				(mfdcri(SDR0, DCRN_SDR_ICINTSTAT) | ICINTSTAT_ICTX));
> +		mtdcri(SDR0, DCRN_SDR_ICINTSTAT, ICINTSTAT_ICTX);
>  #endif
>  
>  	return IRQ_HANDLED;
> @@ -302,8 +301,7 @@ static irqreturn_t mal_rxeob(int irq, void *dev_instance)
>  
>  #ifdef CONFIG_PPC_DCR_NATIVE
>  	if (mal_has_feature(mal, MAL_FTR_CLEAR_ICINTSTAT))
> -		mtdcri(SDR0, DCRN_SDR_ICINTSTAT,
> -				(mfdcri(SDR0, DCRN_SDR_ICINTSTAT) | ICINTSTAT_ICRX));
> +		mtdcri(SDR0, DCRN_SDR_ICINTSTAT, ICINTSTAT_ICRX);
>  #endif
>  
>  	return IRQ_HANDLED;
> -- 
> 2.55.0
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson