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

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

[-- Attachment #1: Type: text/plain, Size: 2698 bytes --]

On Fri, Jul 03, 2026 at 08:21:46PM -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.
> 
> Tested on Cisco Meraki MX60. No issues seen.

Can you elaborate on what that means?  On the face it sounds like you
made the change and nothing blew up dramatically.  That's much better
than nothing, but since the original version also apparently worked
well enough to get by for years with no-one noticing, it doesn't
(alone) give a lot of confidence that the new version is an
improvement.

Or, did you mean that you observed dropped interrupts before, but
didn't after the change?  That would be much stronger evidence that
the change is beneficial.

> Fixes: 1d3bb996481e ("Device tree aware EMAC driver")
> Assisted-by: opencode:big-pickle
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  v2: mention that this was tested.
>  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 74526002d52b..e88e4a1ddcd4 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]