Re: [PATCH net] net: emac: mal: fix W1C write race in ICINTSTAT clearing
From: David Gibson <hidden>
Date: 2026-07-03 03:24:48
Also in:
lkml
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 <redacted>
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.
quoted hunk ↗ jump to hunk
--- 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