Re: [PATCHv2 net] net: emac: mal: fix W1C write race in ICINTSTAT clearing
From: David Gibson <hidden>
Date: 2026-07-06 05:09:59
Also in:
lkml
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.
quoted hunk ↗ jump to hunk
Fixes: 1d3bb996481e ("Device tree aware EMAC driver") Assisted-by: opencode:big-pickle Signed-off-by: Rosen Penev <redacted> --- 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
Attachments
- signature.asc [application/pgp-signature] 833 bytes