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

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

[Rosen Penev]

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.

Fixes: 1d3bb996481e ("Device tree aware EMAC driver")
Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 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

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

[David Gibson]

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

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

[Andrew Lunn]

On Fri, Jul 03, 2026 at 01:06:40PM +1000, David Gibson wrote:
> 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.

I agree. If this is pure LLM, we need some form of verification.

  Andrew

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

[Rosen Penev]

On Fri, Jul 3, 2026 at 9:37 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Fri, Jul 03, 2026 at 01:06:40PM +1000, David Gibson wrote:
> > 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.
>
> I agree. If this is pure LLM, we need some form of verification.
Only verification I have is the hardware and whether or not something breaks.

Sashiko reports:

This isn't a bug introduced by this patch, but does this read-modify-write
pattern on a write-1-to-clear interrupt status register cause lost
interrupts?
If another interrupt occurs and its bit is set before the mfdcri() read, the
read value will include that bit as a 1. The bitwise OR retains this 1, and
the subsequent mtdcri() will write it back, inadvertently clearing the newly
pending interrupt without processing it.

This is a pre-existing issue, but does this suffer from the same
write-1-to-clear read-modify-write problem as mal_txeob() where concurrent
interrupts might be accidentally cleared?
>
>   Andrew

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

[Andrew Lunn]

> Only verification I have is the hardware and whether or not
> something breaks.

We get so many patches, generated by LLM, which never get anything
more than compile testing, it is now actually useful to comment you
tested it on real hardware. Which is sad.

       Andrew

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

[Rosen Penev]

On Fri, Jul 3, 2026 at 12:11 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Only verification I have is the hardware and whether or not
> > something breaks.
>
> We get so many patches, generated by LLM, which never get anything
> more than compile testing, it is now actually useful to comment you
> tested it on real hardware. Which is sad.
OK. I'll resubmit mentioning that.
>
>        Andrew