From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtpout-03.galae.net (smtpout-03.galae.net [185.246.85.4]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id ED322379EE7 for ; Wed, 15 Apr 2026 14:07:53 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.246.85.4 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776262076; cv=none; b=CdpEiu9hGLYbmX/3RBZG5CcHPyRIhRdByeIyZ14iwcWv8hBGud5RSjbf8zUNjvdUZoUNzX2cxnNhc8lMf5TmFhxwE7MyFOSPaNEs1Zt80l39SQNj753B+DGp1jr5yh1M4l8OhLPM6/LRBcza4YihvGKlTUlNwPDi021yACqhRhQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776262076; c=relaxed/simple; bh=Fs4ebNmCBSRCtIeM3x+VpFjjj5xTJ3v3L14xv0j/0Dg=; h=Mime-Version:Content-Type:Date:Message-Id:From:Subject:Cc:To: References:In-Reply-To; b=QLvc5XZTPnIDB1LIh6YoPiYD1X0kbtmJde87P74z6C2rkopPi1LVM0dpUuZ2AfKWU/54im9AEiy8CvS3adsu71Rv9kxuUUIN2zLUfK8fDWhu/wfFb6BkPR9SZUjTWHFDYgdcDkc2P8Be5J7bOAsauAkBUbVcI1geaCF/w3tVPiQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com; spf=pass smtp.mailfrom=bootlin.com; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b=kkAcffNN; arc=none smtp.client-ip=185.246.85.4 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=bootlin.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b="kkAcffNN" Received: from smtpout-01.galae.net (smtpout-01.galae.net [212.83.139.233]) by smtpout-03.galae.net (Postfix) with ESMTPS id 5C4A34E42A02; Wed, 15 Apr 2026 14:07:52 +0000 (UTC) Received: from mail.galae.net (mail.galae.net [212.83.136.155]) by smtpout-01.galae.net (Postfix) with ESMTPS id 20C0B60420; Wed, 15 Apr 2026 14:07:52 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) by localhost (Mailerdaemon) with ESMTPSA id C05E410451B9E; Wed, 15 Apr 2026 16:07:48 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=dkim; t=1776262070; h=from:subject:date:message-id:to:cc:mime-version:content-type: content-transfer-encoding:in-reply-to:references; bh=oK2yz944i89PT8aKyVdZj6EXcfPBWWdS0h1p8cICzKI=; b=kkAcffNNoaNgw26VT6CGmEzg2VPwt5MN1jVQaMzxrRckOuSqUwa2xdLUakCwJoQRLN1FoU 9Qf8TYKwKswy8b3SHEkkSpsshj4zts1oEH28e1ud8aKCTGXmlh7QPyNT1/7b7ONAsUfgUI H/tWRnqD9lMTKOAG2PUIDXiJILhBB0tIyX7QDz8INNNCr+TiKBvKko2xKhUhRYKRLFiX5i piNwzTVAdg+nDzuPqJxI8YwmCupJ3ecdflansC3ntmm9nyFlSX+q4gsAnH5uNN5n5KC8ax PbrCSN8cqHSTxM+IvP2P69RPLg2v0OnId/YVzH1KmLdQeCThMYSdrvkmhjzfWg== Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 15 Apr 2026 16:07:48 +0200 Message-Id: From: =?utf-8?q?Th=C3=A9o_Lebrun?= Subject: Re: [PATCH net-next v2 12/14] net: macb: re-read ISR inside IRQ handler locked section Cc: , , , , , , , , , , , , , , , , , , To: "Jakub Kicinski" X-Mailer: aerc 0.21.0-0-g5549850facc2 References: <20260410-macb-context-v2-12-af39f71d40b6@bootlin.com> <20260414005244.3758889-1-kuba@kernel.org> In-Reply-To: <20260414005244.3758889-1-kuba@kernel.org> X-Last-TLS-Session-Version: TLSv1.3 Hello Jakub's LLM, On Tue Apr 14, 2026 at 2:52 AM CEST, Jakub Kicinski wrote: > This is an AI-generated review of your patch. The human sending this > email has considered the AI review valid, or at least plausible. > --- > net: macb: re-read ISR inside IRQ handler locked section > > This commit adds a second read of the ISR register inside the lock in > macb_interrupt() to handle a race condition where the status might have > been updated by concurrent hardware resets. > >> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethe= rnet/cadence/macb_main.c >> index 2633234012bae..3deab87c565b5 100644 >> --- a/drivers/net/ethernet/cadence/macb_main.c >> +++ b/drivers/net/ethernet/cadence/macb_main.c >> @@ -2190,6 +2190,13 @@ static irqreturn_t macb_interrupt(int irq, void *= dev_id) >> =20 >> spin_lock(&bp->lock); >> =20 >> + /* `status` stack variable might be stalled =3D> re-read it */ >> + status =3D queue_readl(queue, ISR); > > Will this second read discard interrupt events on older MACB variants tha= t > use clear-on-read for the ISR? > > If the hardware does not have the MACB_CAPS_ISR_CLEAR_ON_WRITE capability= , > the first queue_readl call before the lock already cleared the active bit= s. > > Overwriting the local status variable here sets it to 0, which permanentl= y > drops the initially captured Rx/Tx completions and stalls the network > interface. The AI is correct. For R1C, we must check ISR inside the bp->lock critical section. For W1C, we can check it outside to detect spurious interrupt without grabbing bp->lock and then we check again in bp->lock. So: static irqreturn_t macb_interrupt(int irq, void *dev_id) { // ... /* detect spurious interrupts without grabbing bp->lock */ if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE) { status =3D queue_readl(queue, ISR); if (unlikely(!status)) return IRQ_NONE; } spin_lock(&bp->lock); status =3D queue_readl(queue, ISR); if (unlikely(!status)) { spin_unlock(&bp->lock); return IRQ_NONE; } // ... } Or we could drop the readl(ISR) from outside bp->lock. >> + if (unlikely(!status)) { >> + spin_unlock(&bp->lock); >> + return IRQ_NONE; > > If the function reaches this point, the initial status read before the lo= ck > was non-zero, meaning the device legitimately asserted the interrupt. > > If a concurrent operation cleared the hardware state while this thread > waited for the lock, will returning IRQ_NONE falsely flag this as a > spurious interrupt? > > Since the interrupt was actually raised by the device, doing this repeate= dly > can cause the core IRQ subsystem to permanently disable the IRQ line. > > Could this return IRQ_HANDLED instead? Disagreed on this one. This codepath, if it happens, is expected to be infrequent and to protect against races by the swap operation disabling the device. There is no chance we often land here and end up disabling the IRQ line. Thanks, -- Th=C3=A9o Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com