From mboxrd@z Thu Jan 1 00:00:00 1970 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 3232B1DFDA1; Mon, 13 Jul 2026 16:41:29 +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=1783960892; cv=none; b=m/iQvfppSQAmf2uyBsMMQ1KIny8u7GranuzI7bgxeSfosb/gSMtRHL5Bejq759/8KBPo38p3EaRcj5/7FbCMgcZCYRxZJIefDzVCb6z7Pvkbm2Ja1dWNcTjRH+36nzpUZUtnojMVe0ZKDg6XfJtRzERAVApmFt+KHtuU1sLeH2Q= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783960892; c=relaxed/simple; bh=bcg6tFwoVL3YWWFogN2mO0WDKz5TBe7Q+dyN50fZm7E=; h=Mime-Version:Content-Type:Date:Message-Id:Subject:Cc:To:From: References:In-Reply-To; b=tUTGozscq2dbokmqUEA7cajsntaaLrCv0PU2RzyHytc1W1BLf1XImj8/PGjV7ME0HuoNhYBOZEtBWH9L2B/k+pjAbs7jJo36QdRe+HOvF3nO7WKAmMprko86biqJ77TEuh3M899N+fAQHSywEaQ/UZmFSmmOe3c9WMfdIZWumFg= 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=weK7KLPX; 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="weK7KLPX" Received: from smtpout-01.galae.net (smtpout-01.galae.net [212.83.139.233]) by smtpout-04.galae.net (Postfix) with ESMTPS id 72D1FC2BB14; Mon, 13 Jul 2026 16:41:43 +0000 (UTC) Received: from mail.galae.net (mail.galae.net [212.83.136.155]) by smtpout-01.galae.net (Postfix) with ESMTPS id 3A45660345; Mon, 13 Jul 2026 16:41:27 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) by localhost (Mailerdaemon) with ESMTPSA id 7AE3B11BD3A20; Mon, 13 Jul 2026 18:41:20 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=dkim; t=1783960886; h=from:subject:date:message-id:to:cc:mime-version:content-type: content-transfer-encoding:in-reply-to:references; bh=kznHa91E/ci3RkkQNuYqHNzjA7ZryX0m9Z2e76kwoqU=; b=weK7KLPX2TutxqjG5P8AQyQnhAhvywxiqfdPL0pmCXnz+GEJVlq9/KdH5xb5JnCpUjL5I3 G5thWT74FUIk0QlxtAbUGKI0Ph3suqw7gQwRNcRM93uh/GNNJI4Zx2EGb6EBtE/8LzI1pa vkTraxZkNeNctFUbmLlE7tvTeR38v//agVtwQ9ZC9kN6SYzSzYvmMZRdl7upQdhEkgbNbU TII0dwimSTMS2+inKMMtUJXHI7kZABAz33BOYkeEDWq1ZSCEiub94gvRkn+m/R64hlawNI Sssi0+X1VXsQwYqjJoshTxSZ4++bHnNZPm4ih4kIf38Cw/S1E8dnikzREAe1tg== Precedence: bulk X-Mailing-List: netdev@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: Mon, 13 Jul 2026 18:41:19 +0200 Message-Id: Subject: Re: [PATCH net-next v3 14/15] net: macb: use context swapping in .set_ringparam() Cc: "Conor Dooley" , "Andrew Lunn" , "David S. Miller" , "Eric Dumazet" , "Jakub Kicinski" , "Paolo Abeni" , "Richard Cochran" , "Russell King" , , , "Nicolas Ferre" , "Claudiu Beznea" , "Paolo Valerio" , "Vladimir Kondratiev" , "Gregory CLEMENT" , =?utf-8?q?Beno=C3=AEt_Monin?= , "Tawfik Bayouk" , "Thomas Petazzoni" , "Maxime Chevallier" To: "Nicolai Buchwitz" From: =?utf-8?q?Th=C3=A9o_Lebrun?= X-Mailer: aerc 0.21.0-0-g5549850facc2 References: <20260701-macb-context-v3-0-00268d5b1502@bootlin.com> <20260701-macb-context-v3-14-00268d5b1502@bootlin.com> In-Reply-To: X-Last-TLS-Session-Version: TLSv1.3 On Thu Jul 2, 2026 at 12:37 PM CEST, Nicolai Buchwitz wrote: > On 1.7.2026 17:59, Th=C3=A9o Lebrun wrote: >> ethtool_ops.set_ringparam() is implemented using the primitive close / >> update ring size / reopen sequence. Under memory pressure this does not >> fly: we free our buffers at close and cannot reallocate new ones at >> open. Also, it triggers a slow PHY reinit. >>=20 >> Instead, exploit the new context mechanism and improve our sequence to: >> - allocate a new context (including buffers) first >> - if it fails, early return without any impact to the interface >> - stop interface >> - update global state (bp, netdev, etc) >> - pass buffer pointers to the hardware >> - start interface >> - free old context. >>=20 >> The HW disable sequence is inspired by macb_reset_hw() but avoids >> (1) setting NCR bit CLRSTAT and (2) clearing register PBUFRXCUT. >>=20 >> The HW re-enable sequence is inspired by macb_mac_link_up(), skipping >> over register writes which would be redundant (because values have not >> changed). >>=20 >> The generic context swapping parts are isolated into helper functions >> macb_context_swap_start|end(), reusable by other operations=20 >> (change_mtu, >> set_channels, etc). >>=20 >> Introduce a new locking primitive (mac_cfg_lock mutex) to serialise=20 >> swap >> with phylink MAC callbacks. Avoid stopping phylink to avoid a slow PHY >> retrain. Those callbacks grab phydev->lock if it exists so we could >> imagine grabbing that from the swap op, but phydev->lock doesn't exist >> in the SFP case. >>=20 >> AT91 EMAC is handled differently as their buffer management is separate >> and they don't do NAPI. We refuse them (-EBUSY) to avoid implementing >> context swapping for them. >>=20 >> Signed-off-by: Th=C3=A9o Lebrun >> --- >> drivers/net/ethernet/cadence/macb.h | 2 + >> drivers/net/ethernet/cadence/macb_main.c | 142=20 >> +++++++++++++++++++++++++++++-- > >> [...] > >> +static void macb_context_swap_start(struct macb *bp) >> +{ >> + struct macb_queue *queue; >> + unsigned long flags; >> + unsigned int q; >> + u32 ctrl; >> + >> + mutex_lock(&bp->mac_cfg_lock); >> + >> + /* Mask interrupts before disabling BH features. */ >> + spin_lock_irqsave(&bp->lock, flags); >> + for (q =3D 0, queue =3D bp->queues; q < bp->num_queues; ++q, ++queue) = { >> + queue_writel(queue, IDR, -1); >> + queue_readl(queue, ISR); >> + macb_queue_isr_clear(bp, queue, -1); >> + } >> + spin_unlock_irqrestore(&bp->lock, flags); >> + >> + /* Drain BH features. HW is still active and usable at this point. */ >> + >> + cancel_work_sync(&bp->hresp_err_bh_work); >> + cancel_delayed_work_sync(&bp->tx_lpi_work); >> + >> + for (q =3D 0, queue =3D bp->queues; q < bp->num_queues; ++q, ++queue) = { >> + napi_disable(&queue->napi_rx); >> + napi_disable(&queue->napi_tx); >> + cancel_work_sync(&queue->tx_error_task); >> + netdev_tx_reset_queue(netdev_get_tx_queue(bp->netdev, q)); >> + } > > Can this deadlock against a pending tx_error_task? > > AFAIU macb_tx_error_task() does napi_disable(&queue->napi_tx) and later > napi_enable() on the same napi, and it can already be queued=20 > (macb_interrupt() > schedules it on a TX error) by the time the swap runs: > > swap_start: napi_disable(napi_tx) /* sets SCHED, returns */ > worker: tx_error_task: napi_disable(napi_tx) /* spins on SCHED */ > swap_start: cancel_work_sync(tx_error_task) /* waits on worker= =20 > */ > > napi_disable() spins until napi_enable() clears SCHED, but here the swap= =20 > won't > re-enable until macb_context_swap_end(), and cancel_work_sync() is=20 > what's > holding it up. Nothing clears it. > > Maybe cancel_work_sync() before the napi_disable() calls would work=20 > instead? IRQs > are masked just above, so AFAICT nothing can reschedule tx_error_task by= =20 > then. Ah yes, good catch. macb_tx_error_task() should never enter if NAPI is disabled, else its napi_disable() will hang. The fix sounds easy enough, as you indicated. We'll move the cancel_work_sync(queue->tx_error_task) call to be above our swap start napi_disable(). Thanks, -- Th=C3=A9o Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com