From nobody Thu Sep 24 22:56:31 2026 Received: from smtpout-04.galae.net (smtpout-04.galae.net [185.171.202.116]) (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 174DE525A89; Fri, 18 Sep 2026 20:35:22 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.171.202.116 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789763724; cv=none; b=BiRAyVys8uQ0amY0MrLw+XewMWAUzqqvbIKRGr3aW4k1lCibGQW9M+oj9GIR39vxc9QCHQgFVsVbynbjJIDMgMLO3Sinj5UnJYF/JfVGEcjsIEAP3eC8EkGTBNQmq2a+8YEVJRsjTXhAmm2hjRfK5UciTrWvPClf1lJBOQkh9MQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789763724; c=relaxed/simple; bh=awCtQKR8zVzOsWS0w8h31o5Rv/jqZJtos1/c2hzAfLo=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=FYEpsO0ITUwXxq/LJUq3c0t/Vtj89q5uV0BAYQUGOgXfNQzThfD7J+zsiLe0ku8HYoRW0v1/e4w6ANwvgXAqpeBgRLltwwEv9g2xlWvMhaipInZl89stjmSRhFAjpzg7mqOfvJa0I1Z6GblCwdmA2g8w3y7pTn144/C6gJiGlkI= 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=Qa9xWzM/; arc=none smtp.client-ip=185.171.202.116 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="Qa9xWzM/" Received: from smtpout-01.galae.net (smtpout-01.galae.net [212.83.139.233]) by smtpout-04.galae.net (Postfix) with ESMTPS id 7382FC5846F; Fri, 18 Sep 2026 20:36:06 +0000 (UTC) Received: from mail.galae.net (mail.galae.net [212.83.136.155]) by smtpout-01.galae.net (Postfix) with ESMTPS id 7F2EC60649; Fri, 18 Sep 2026 20:35:21 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) by localhost (Mailerdaemon) with ESMTPSA id DE7741032907B; Fri, 18 Sep 2026 22:35:12 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=dkim; t=1789763715; h=from:subject:date:message-id:to:cc:mime-version:content-type: content-transfer-encoding:in-reply-to:references; bh=iUeXAsB+eMon3IGQKZXDkNH5ztxmcZZI6qhivhfoRbY=; b=Qa9xWzM/g/Cg3tIK9cnvxSMxBPBzM4NeshEJf3hbNP+0v5Ru0zWyX8+LMyiFcxUqUGc/mJ 4y9GQeiqQkmzsMYRtnNNz7t7VeHAl8GpOse0WNLtwhZU4I6LzQ+jmUbCKTzFNC2hNKW2+9 M4SzLmEbGFeYDuG3y8i4BDHpw4MtyLVy4ckHHX9Qpw756YpB7AjyNzMBJm0/imSTOAOmJm 805hQ0XpTF8JHK6sJqJcnyIUw1zRuxPRmv6eCrFowUvtwYZ8VtTPrsSa7XL6ROrfvwlo7s 1u6tI4waaxLiPFnR3umh2X75yP7meZtVH/oCJ30UABpPYMfgyOnGIO0YlCDKKQ== From: =?utf-8?q?Th=C3=A9o_Lebrun?= Date: Fri, 18 Sep 2026 22:35:04 +0200 Subject: [PATCH net 1/3] net: macb: never give hardware a NULL RX buffer Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Message-Id: <20260918-macb-close-v1-1-05e32ce98813@bootlin.com> References: <20260918-macb-close-v1-0-05e32ce98813@bootlin.com> In-Reply-To: <20260918-macb-close-v1-0-05e32ce98813@bootlin.com> To: Conor Dooley , Andrew Lunn , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Simon Horman , Nicolas Ferre , Sean Anderson , Antoine Tenart , Russell King Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Nicolai Buchwitz , Vladimir Kondratiev , Gregory CLEMENT , Tawfik Bayouk , Thomas Petazzoni , Maxime Chevallier , =?utf-8?q?Th=C3=A9o_Lebrun?= , stable@vger.kernel.org X-Mailer: b4 0.15.2 X-Last-TLS-Session-Version: TLSv1.3 The refill logic is simple: iterate over all pending rx slots, allocate SKB (& DMA map) if needed and hand it off the to hardware by clearing the RX_USED flag. If the refill operation fails mid-way, it early returns leaving the following slots untouched. In the normal case that is fine, because all slots have been properly initialised (and might have been already used by HW meaning they won't be reused). When slots have not been initialised however, we are in trouble. After dma_alloc_coherent() of the rx ring buffer, all slots have NULL pointers and RX_USED cleared meaning HW will try using them. Ensure this does not happen by setting the RX_USED flag on all slots before calling refill at buffer alloc, in gem_init_rx_ring(). That way even if refill fails on an alloc/dma_map, the HW won't try using NULL pointers as buffers. Theoretical bugfix, never encountered in practice. To reproduce, introduce memory pressure (less than 512 SKBs of free memory) and open the interface. Note that this codepath also hits at resume, on HRESP errors and on set_ringparam (while interface is running). Fixes: 4df95131ea80 ("net/macb: change RX path for GEM") Cc: stable@vger.kernel.org Signed-off-by: Th=C3=A9o Lebrun --- drivers/net/ethernet/cadence/macb_main.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/etherne= t/cadence/macb_main.c index b8234ac4b602..751fa9e68099 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -2785,9 +2785,14 @@ static int macb_alloc(struct macb *bp) =20 static void gem_init_rx_ring(struct macb_queue *queue) { + unsigned int i; + queue->rx_tail =3D 0; queue->rx_prepared_head =3D 0; =20 + for (i =3D 0; i < queue->bp->rx_ring_size; i++) + macb_rx_desc(queue, i)->addr |=3D MACB_BIT(RX_USED); + gem_rx_refill(queue); } =20 --=20 2.55.0 From nobody Thu Sep 24 22:56:31 2026 Received: from smtpout-02.galae.net (smtpout-02.galae.net [185.246.84.56]) (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 D528A524AE4; Fri, 18 Sep 2026 20:35:29 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.246.84.56 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789763731; cv=none; b=gOp0wn1ZoCbDA3YnQ/3H19Bc6G98OQaYc9Zt6A5EVbBXRgsvlcNLiXrI2fwR9vG3KStfhp/NnwrXHJeFiRKqrAHIFSR8jgek7Ki7urF+du4/5PopG73b2IOEYbr5hAbN4l1dOf7dMAbFQvxZE00OlcoaDnpaQkpoC/PoR70K8RM= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789763731; c=relaxed/simple; bh=qXeso4G9JOJSfSUyHMb0bEYqyspr0J0X6c8OuiOuc+E=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=GWHVSghAFx0nL40HaFrRos2Q1mnzbJqjZIuA4lKJZ4zkeq9wCfxU8MPsnPyBt+0PH33tdsGkWafXVcGrQFKvzwv7a0IbUMKVnnfX66V+LlIBHHekm2Es4yhxfFmEK5TVjwYvOyGnd/BC6jl9a1UIYypeqI83Aab1u6GWIIpMYys= 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=a1sbgYDQ; arc=none smtp.client-ip=185.246.84.56 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="a1sbgYDQ" Received: from smtpout-01.galae.net (smtpout-01.galae.net [212.83.139.233]) by smtpout-02.galae.net (Postfix) with ESMTPS id 74ECB1A099D; Fri, 18 Sep 2026 20:35:28 +0000 (UTC) Received: from mail.galae.net (mail.galae.net [212.83.136.155]) by smtpout-01.galae.net (Postfix) with ESMTPS id 488AE60649; Fri, 18 Sep 2026 20:35:28 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) by localhost (Mailerdaemon) with ESMTPSA id 309CD1032907C; Fri, 18 Sep 2026 22:35:20 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=dkim; t=1789763722; h=from:subject:date:message-id:to:cc:mime-version:content-type: content-transfer-encoding:in-reply-to:references; bh=/mAkVh9vqUyP4Y71/1fJoyfrZD8rk6C0eOjg+j/FQBw=; b=a1sbgYDQ21K61aUfm3x5mp3Om9ainMMaCdQQi3sMa+nRCON/lAcbVB2WbUZyZKcqdFyg/k +J1l6qdaHM0YCDD9s+uUVvtvGZDx9/wU7an2rKRTRAemcTsq0y31Vi/GNu1wkcK1gRXJWv /5Zn+Nl2BPYG92Ct2onOS2l3wErEUescHDKcXgSSEXaW/AmgcKWN4LGLUafelGAkFR1xdr o19vQ2P/f5FQtRDjG3QBuINMYZRGjwaYG2efq02BY3HBwB29BLlahp5SJuosiQrrIh1IE9 3Ijn/1jFTizvSxSVzZ/hx2ARyK/FJ1MIkI93kzKWGTpGJppfS0X9/4/VBORbZA== From: =?utf-8?q?Th=C3=A9o_Lebrun?= Date: Fri, 18 Sep 2026 22:35:05 +0200 Subject: [PATCH net 2/3] net: macb: propagate RX ring refill errors Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Message-Id: <20260918-macb-close-v1-2-05e32ce98813@bootlin.com> References: <20260918-macb-close-v1-0-05e32ce98813@bootlin.com> In-Reply-To: <20260918-macb-close-v1-0-05e32ce98813@bootlin.com> To: Conor Dooley , Andrew Lunn , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Simon Horman , Nicolas Ferre , Sean Anderson , Antoine Tenart , Russell King Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Nicolai Buchwitz , Vladimir Kondratiev , Gregory CLEMENT , Tawfik Bayouk , Thomas Petazzoni , Maxime Chevallier , =?utf-8?q?Th=C3=A9o_Lebrun?= , stable@vger.kernel.org X-Mailer: b4 0.15.2 X-Last-TLS-Session-Version: TLSv1.3 gem_rx_refill() is responsible for Rx SKB allocation, including at open, but its prototype indicates a void return value. Therefore we change the code to propagate allocation and DMA mapping errors back up the stack, making sure the open fails if it occurs. Change all those to return errno-style ints: - gem_rx_refill() - its parent gem_init_rx_ring() - its grand-parent gem_init_rings() - the macbgem_ops.mog_init_rings function pointer - its grand-uncle macb_init_rings() Theoretical bugfix, never encountered in practice. To reproduce, introduce memory pressure (less than 512 SKBs of free memory) and open the interface. I expect the last queue to be unuseable because it has zero usable rx buffers. Nothing will ever trigger a refill on that queue which only happens once a frame has been received. Note that other callers of refill (resume, HRESP error task, NAPI) cannot do anything useful with that error and keep their best-effort refill, hoping it will improve. Fixes: 4df95131ea80 ("net/macb: change RX path for GEM") Cc: stable@vger.kernel.org Signed-off-by: Th=C3=A9o Lebrun --- drivers/net/ethernet/cadence/macb.h | 2 +- drivers/net/ethernet/cadence/macb_main.c | 30 +++++++++++++++++++++++-----= -- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cad= ence/macb.h index d6931c41f39d..cfaa0ca49f1a 100644 --- a/drivers/net/ethernet/cadence/macb.h +++ b/drivers/net/ethernet/cadence/macb.h @@ -1197,7 +1197,7 @@ struct macb_queue; struct macb_or_gem_ops { int (*mog_alloc_rx_buffers)(struct macb *bp); void (*mog_free_rx_buffers)(struct macb *bp); - void (*mog_init_rings)(struct macb *bp); + int (*mog_init_rings)(struct macb *bp); int (*mog_rx)(struct macb_queue *queue, struct napi_struct *napi, int budget); }; diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/etherne= t/cadence/macb_main.c index 751fa9e68099..c418f859cc34 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -1486,13 +1486,14 @@ static int macb_tx_complete(struct macb_queue *queu= e, int budget) return packets; } =20 -static void gem_rx_refill(struct macb_queue *queue) +static int gem_rx_refill(struct macb_queue *queue) { struct macb *bp =3D queue->bp; struct macb_dma_desc *desc; struct sk_buff *skb; unsigned int entry; dma_addr_t paddr; + int err =3D 0; =20 while (CIRC_SPACE(queue->rx_prepared_head, queue->rx_tail, bp->rx_ring_size) > 0) { @@ -1509,6 +1510,7 @@ static void gem_rx_refill(struct macb_queue *queue) if (unlikely(!skb)) { netdev_err(bp->netdev, "Unable to allocate sk_buff\n"); + err =3D -ENOMEM; break; } =20 @@ -1518,6 +1520,7 @@ static void gem_rx_refill(struct macb_queue *queue) DMA_FROM_DEVICE); if (dma_mapping_error(&bp->pdev->dev, paddr)) { dev_kfree_skb(skb); + err =3D -ENOMEM; break; } =20 @@ -1558,6 +1561,8 @@ static void gem_rx_refill(struct macb_queue *queue) =20 netdev_vdbg(bp->netdev, "rx ring: queue: %p, prepared head %d, tail %d\n", queue, queue->rx_prepared_head, queue->rx_tail); + + return err; } =20 /* Mark DMA descriptors from begin up to and not including end as unused */ @@ -2783,7 +2788,7 @@ static int macb_alloc(struct macb *bp) return -ENOMEM; } =20 -static void gem_init_rx_ring(struct macb_queue *queue) +static int gem_init_rx_ring(struct macb_queue *queue) { unsigned int i; =20 @@ -2793,14 +2798,16 @@ static void gem_init_rx_ring(struct macb_queue *que= ue) for (i =3D 0; i < queue->bp->rx_ring_size; i++) macb_rx_desc(queue, i)->addr |=3D MACB_BIT(RX_USED); =20 - gem_rx_refill(queue); + return gem_rx_refill(queue); } =20 -static void gem_init_rings(struct macb *bp) +static int gem_init_rings(struct macb *bp) { struct macb_queue *queue; struct macb_dma_desc *desc =3D NULL; + int last_err =3D 0; unsigned int q; + int err; int i; =20 for (q =3D 0, queue =3D bp->queues; q < bp->num_queues; ++q, ++queue) { @@ -2813,11 +2820,15 @@ static void gem_init_rings(struct macb *bp) queue->tx_head =3D 0; queue->tx_tail =3D 0; =20 - gem_init_rx_ring(queue); + err =3D gem_init_rx_ring(queue); + if (err) + last_err =3D err; } + + return last_err; } =20 -static void macb_init_rings(struct macb *bp) +static int macb_init_rings(struct macb *bp) { int i; struct macb_dma_desc *desc =3D NULL; @@ -2832,6 +2843,8 @@ static void macb_init_rings(struct macb *bp) bp->queues[0].tx_head =3D 0; bp->queues[0].tx_tail =3D 0; desc->ctrl |=3D MACB_BIT(TX_WRAP); + + return 0; } =20 static void macb_reset_hw(struct macb *bp) @@ -3162,7 +3175,9 @@ static int macb_open(struct net_device *netdev) goto pm_exit; } =20 - bp->macbgem_ops.mog_init_rings(bp); + err =3D bp->macbgem_ops.mog_init_rings(bp); + if (err) + goto free_rings; macb_init_buffers(bp); =20 for (q =3D 0, queue =3D bp->queues; q < bp->num_queues; ++q, ++queue) { @@ -3200,6 +3215,7 @@ static int macb_open(struct net_device *netdev) napi_disable(&queue->napi_rx); napi_disable(&queue->napi_tx); } +free_rings: macb_free(bp); pm_exit: pm_runtime_put_sync(&bp->pdev->dev); --=20 2.55.0 From nobody Thu Sep 24 22:56:31 2026 Received: from smtpout-02.galae.net (smtpout-02.galae.net [185.246.84.56]) (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 18983525A98; Fri, 18 Sep 2026 20:35:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.246.84.56 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789763743; cv=none; b=WTE5SFQJE5SSnOZ8shK7zCbBu8dpdf8qH+WcU465Pt9+5quvQtU5uEqRkqrt3DlJjxnqjTVgZMsGAhuwiUnjUkX1X83cuzFVr9CIOs67cGCZMps59GhsUQaKeFS7/EMfr4/fT23dVQ5iLKR79604hQ39qVh7kEkfYwni976Fjtk= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789763743; c=relaxed/simple; bh=lYVIRWYpEJbrhQ/oBp9dfFrspCsE0CzlZ1NBrrCWUHY=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=jOUTuqIfMkEfii3yDZ98tENf+M4JpViepm4/g2s2f+oW2bJhdZkys1pb8TKSTNEDHjPjwD06vVhlOGpM/rDixUkuv5fH2p3s6hyJjr5a6VkhfuYgaoPilibG0UNpZxg2D+LKlQQms5lZUFQxN+E9cLkIgEqg3oMNnYB9+72VYOs= 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=vy3QbrTO; arc=none smtp.client-ip=185.246.84.56 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="vy3QbrTO" Received: from smtpout-01.galae.net (smtpout-01.galae.net [212.83.139.233]) by smtpout-02.galae.net (Postfix) with ESMTPS id DCE161A0664; Fri, 18 Sep 2026 20:35:34 +0000 (UTC) Received: from mail.galae.net (mail.galae.net [212.83.136.155]) by smtpout-01.galae.net (Postfix) with ESMTPS id A85D360649; Fri, 18 Sep 2026 20:35:34 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) by localhost (Mailerdaemon) with ESMTPSA id 1D26F10329075; Fri, 18 Sep 2026 22:35:26 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=dkim; t=1789763729; h=from:subject:date:message-id:to:cc:mime-version:content-type: content-transfer-encoding:in-reply-to:references; bh=KiYXgWSHGS4Z5hj8EXdVamYzUDB+odt7GHe4rRMG7F4=; b=vy3QbrTO9Wig4snJ1Qpm10xtVhOS1bVZh2/5ZLG3fWDcV5drqNg2MAJeePUsOxho0t7LaO h02ZViA2wAz7qFFwuYGy60fr+z4dkLWfIDUrYtKpEnO+BR/xSjjR9YV7nJKYZP0yJjW4ZF WOOqRN0PnJxESpOIueBtHQdtvK9DPrpssXO/muqp/Iyy9e33D0WIoAo/SuWPoswzTGxYC4 W1J7F5yVSjcA5QuxBVl16UB4QKTRog09gsL7j+iuY7v/X/NvNHGXch/TatrkLzkpwmXLlj mN0kWJyplLMEiBfwGkLTGEejJpaYWnHw8X7KqDxwdSg+sERfKSBREm9Yp+uxtw== From: =?utf-8?q?Th=C3=A9o_Lebrun?= Date: Fri, 18 Sep 2026 22:35:06 +0200 Subject: [PATCH net 3/3] net: macb: quiesce IRQs and drain BH on interface close Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Message-Id: <20260918-macb-close-v1-3-05e32ce98813@bootlin.com> References: <20260918-macb-close-v1-0-05e32ce98813@bootlin.com> In-Reply-To: <20260918-macb-close-v1-0-05e32ce98813@bootlin.com> To: Conor Dooley , Andrew Lunn , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Simon Horman , Nicolas Ferre , Sean Anderson , Antoine Tenart , Russell King Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Nicolai Buchwitz , Vladimir Kondratiev , Gregory CLEMENT , Tawfik Bayouk , Thomas Petazzoni , Maxime Chevallier , =?utf-8?q?Th=C3=A9o_Lebrun?= , stable@vger.kernel.org X-Mailer: b4 0.15.2 X-Last-TLS-Session-Version: TLSv1.3 The macb_close() operation is facing races as it disables IRQs late in its sequence and keeps BH primitives alive while shutdown. Non exhaustive list of races that could occur: - macb_tx_error_task() could be scheduled and access the buffers freed by macb_close(). - macb_tx_error_task() or macb_hresp_error_task() might re-enable interrupts after the IDR write in macb_close(). - macb_close() calls napi_disable() meaning that if macb_tx_error_task() occurs later, it will deadlock on napi_disable() that shouldn't be called if NAPI is already disabled. - macb_hresp_error_task() might reinit every RX/TX ring under macb_close()'s foot. - macb_interrupt() might re-enable NAPI just after it has been disabled by macb_close(). Instead, disable all our primitives one by one: - (1) mask and sync on IRQ handlers, - (2) drain any scheduled bp->hresp_err_bh_work, - (3) drain any scheduled queue->tx_error_task, - (4) drain queue->napi_rx/napi_tx, - (5) drain bp->tx_lpi_work. Careful! Ordering is important because our scheduling primitives can wake each other up. Recap table: | | enable/disable | schedule | | |----|-------|-------|------|----|----|--------|-----| | |IRQs|napi_tx|napi_rx|tx_lpi|napi|napi|tx_error|hresp| | Context | | | | task | rx | tx | task |task | |=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D|=3D=3D=3D=3D|=3D=3D=3D=3D=3D= =3D=3D|=3D=3D=3D=3D=3D=3D=3D|=3D=3D=3D=3D=3D=3D|=3D=3D=3D=3D|=3D=3D=3D=3D|= =3D=3D=3D=3D=3D=3D=3D=3D|=3D=3D=3D=3D=3D| | open | X | X | X | | | | | | | link_up | X | | | | | | | | | link_down | X | | | | | | | | | close | X | X | X | | | | | | | enable_tx_lpi | | | | X | | | | | | swap | X | X | X | X | | | | | | suspend | X | X | X | | | | | | | resume | X | X | X | | | | | | |---------------|----|-------|-------|------|----|----|--------|-----| | irq & netpoll | X | | | | X | X | X | X | |---------------|----|-------|-------|------|----|----|--------|-----| | napi_rx | X | | | | X | | | | | napi_tx | X | | | X | | X | | | |---------------|----|-------|-------|------|----|----|--------|-----| | tx_error_task | X | X | | | | | | | | hresp task | X | | | | | | | | As example, one ordering constraint that can be deduced from the table: napi_tx can schedule tx_lpi_task meaning napi_tx must be disabled before tx_lpi_task, else we risk napi_tx re-enabling tx_lpi_task after it has been disabled by macb_close(). We do *not* use IDR masking to shutdown IRQs because that risks conflicting with BH primitives we have not disabled yet. For example if we writel(IDR) in macb_close() and napi_rx is pending then IRQs might be unmasked by the NAPI poll. As for why we do not use disable_irq(): we will have situations where we are quiesced but want to listen to some IRQs and (minor reason) we register shared IRQ handlers so we shouldn't disable the full IRQ line. Instead we introduce a bool that tells macb_interrupt() to self-disarm. Its default value is true as we start closed. It gets set to false while interface is active. Reading into my crystal ball, we'll reuse that flag in suspend/WOL, set_ringparam and change_mtu (context swap). Note that old IRQ handler tried preventing a race with close by self-disarming based on netif_running(). This might work, but it does not prevent a race with the error codepath of macb_open() which needs to run with IRQs dis-armed but netif_running() returns true during that time. Fixes: e86cd53afc59 ("net/macb: better manage tx errors") Cc: stable@vger.kernel.org Signed-off-by: Th=C3=A9o Lebrun --- drivers/net/ethernet/cadence/macb.h | 5 ++ drivers/net/ethernet/cadence/macb_main.c | 84 ++++++++++++++++++++++++----= ---- 2 files changed, 69 insertions(+), 20 deletions(-) diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cad= ence/macb.h index cfaa0ca49f1a..1cb2778fe49e 100644 --- a/drivers/net/ethernet/cadence/macb.h +++ b/drivers/net/ethernet/cadence/macb.h @@ -1382,6 +1382,11 @@ struct macb { struct delayed_work tx_lpi_work; u32 tx_lpi_timer; =20 + /* ISR must not drive NAPI & BH mechanisms. True when the interface + * is closed. Protected by bp->lock. + */ + bool irq_quiesced; + int rx_bd_rd_prefetch; int tx_bd_rd_prefetch; =20 diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/etherne= t/cadence/macb_main.c index c418f859cc34..75bbde81d62c 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -1995,6 +1995,53 @@ static int macb_tx_poll(struct napi_struct *napi, in= t budget) return work_done; } =20 +static void macb_quiesce_start(struct macb *bp) +{ + struct macb_queue *queue; + unsigned long flags; + unsigned int q; + + spin_lock_irqsave(&bp->lock, flags); + bp->irq_quiesced =3D true; + spin_unlock_irqrestore(&bp->lock, flags); + + for (q =3D 0, queue =3D bp->queues; q < bp->num_queues; ++q, ++queue) + synchronize_irq(queue->irq); + + cancel_work_sync(&bp->hresp_err_bh_work); + + for (q =3D 0, queue =3D bp->queues; q < bp->num_queues; ++q, ++queue) { + /* Must be done before NAPI is disabled: the task ends with a + * napi_enable() call. + */ + cancel_work_sync(&queue->tx_error_task); + + napi_disable(&queue->napi_rx); + napi_disable(&queue->napi_tx); + } + + /* Must be done after napi_tx is disabled: its completion re-arms + * the LPI timer. + */ + cancel_delayed_work_sync(&bp->tx_lpi_work); +} + +static void macb_quiesce_end(struct macb *bp) +{ + struct macb_queue *queue; + unsigned long flags; + unsigned int q; + + for (q =3D 0, queue =3D bp->queues; q < bp->num_queues; ++q, ++queue) { + napi_enable(&queue->napi_rx); + napi_enable(&queue->napi_tx); + } + + spin_lock_irqsave(&bp->lock, flags); + bp->irq_quiesced =3D false; + spin_unlock_irqrestore(&bp->lock, flags); +} + static void macb_hresp_error_task(struct work_struct *work) { struct macb *bp =3D from_work(bp, work, hresp_err_bh_work); @@ -2137,8 +2184,8 @@ static irqreturn_t macb_interrupt(int irq, void *dev_= id) spin_lock(&bp->lock); =20 while (status) { - /* close possible race with dev_close */ - if (unlikely(!netif_running(netdev))) { + /* self-disarm while the netdev is closed */ + if (unlikely(bp->irq_quiesced)) { queue_writel(queue, IDR, -1); macb_queue_isr_clear(bp, queue, -1); break; @@ -3155,8 +3202,6 @@ static int macb_open(struct net_device *netdev) { size_t bufsz =3D netdev->mtu + ETH_HLEN + ETH_FCS_LEN + NET_IP_ALIGN; struct macb *bp =3D netdev_priv(netdev); - struct macb_queue *queue; - unsigned int q; int err; =20 netdev_dbg(bp->netdev, "open\n"); @@ -3180,10 +3225,7 @@ static int macb_open(struct net_device *netdev) goto free_rings; macb_init_buffers(bp); =20 - for (q =3D 0, queue =3D bp->queues; q < bp->num_queues; ++q, ++queue) { - napi_enable(&queue->napi_rx); - napi_enable(&queue->napi_tx); - } + macb_quiesce_end(bp); =20 macb_init_hw(bp); =20 @@ -3210,11 +3252,10 @@ static int macb_open(struct net_device *netdev) phy_power_off(bp->phy); =20 reset_hw: + /* The netdev stays down: quiesce and drain, as macb_close() does. */ + macb_quiesce_start(bp); + macb_reset_hw(bp); - for (q =3D 0, queue =3D bp->queues; q < bp->num_queues; ++q, ++queue) { - napi_disable(&queue->napi_rx); - napi_disable(&queue->napi_tx); - } free_rings: macb_free(bp); pm_exit: @@ -3225,19 +3266,17 @@ static int macb_open(struct net_device *netdev) static int macb_close(struct net_device *netdev) { struct macb *bp =3D netdev_priv(netdev); - struct macb_queue *queue; unsigned long flags; unsigned int q; =20 + macb_quiesce_start(bp); + + /* Drain the BH contexts before stopping the queues: NAPI completion + * and tx_error_task wake them up. + */ netif_tx_stop_all_queues(netdev); - - for (q =3D 0, queue =3D bp->queues; q < bp->num_queues; ++q, ++queue) { - napi_disable(&queue->napi_rx); - napi_disable(&queue->napi_tx); + for (q =3D 0; q < bp->num_queues; ++q) netdev_tx_reset_queue(netdev_get_tx_queue(netdev, q)); - } - - cancel_delayed_work_sync(&bp->tx_lpi_work); =20 phylink_stop(bp->phylink); phylink_disconnect_phy(bp->phylink); @@ -4759,6 +4798,11 @@ static int macb_init_dflt(struct platform_device *pd= ev) bp->tx_ring_size =3D DEFAULT_TX_RING_SIZE; bp->rx_ring_size =3D DEFAULT_RX_RING_SIZE; =20 + /* No locking needed because the IRQs are not requested yet. The + * flag is cleared by macb_open() and re-armed by macb_close(). + */ + bp->irq_quiesced =3D true; + /* set the queue register mapping once for all: queue0 has a special * register mapping but we don't want to test the queue index then * compute the corresponding register offset at run time. --=20 2.55.0