[PATCH net v3 1/2] net: stmmac: ethtool: ignore inapplicable per-queue coalesce fields

[PATCH net v3 1/2] net: stmmac: ethtool: ignore inapplicable per-queue coalesce fields

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

rx_queues_to_use and tx_queues_to_use are independent: dwmac-intel uses
6 RX and 4 TX channels on TGL and EHL, and snps,rx-queues-to-use and
snps,tx-queues-to-use are parsed separately, so a queue index can exist
in one direction only. include/linux/ethtool.h documents the parameters
of the missing side as inapplicable and asks set_per_queue_coalesce() to
ignore them, but __stmmac_set_coalesce() validates both directions for
every index while __stmmac_get_coalesce() reports the inapplicable ones
as zero.

ethtool implements --per-queue by reading the current settings of every
queue in the mask and sending them back with only the requested fields
overwritten, so the zeroed fields are fed straight back into the setter:

  - on an RX-only index the test for both TX fields being zero rejects
    the request, so per-queue RX coalescing cannot be changed at all;
  - on a TX-only index rx_coalesce_usecs is zero, stmmac_usec2riwt()
    returns zero and the MIN_DMA_RIWT test rejects the request, so
    per-queue TX coalescing cannot be changed at all.

On an RX-only index the TX test rejects the request after the RX block
has already called stmmac_rx_watchdog() and stored rx_riwt[] and
rx_coal_frames[], so the driver reports -EINVAL with the hardware half
reprogrammed.  The roll-back that ethtool_set_per_queue_coalesce() runs
for the queues it has already changed then trips over the same test and
cannot restore them.  On a TX-only index the rx_riwt range check fails
before anything is written, but the request is still rejected and the
TX settings are never stored.

Validate and apply only the side that the queue index actually has, and
move the TX checks in front of the RX block so that a request is either
applied completely or rejected without touching the device.

Fixes: db2f2842e6f5 ("net: stmmac: add per-queue TX & RX coalesce ethtool support")
Cc: stable@vger.kernel.org
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
Changes in v2:
- Trim the comment in front of the TX checks to one line; the reasoning
  belongs in the changelog. (Andrew Lunn)
- Link: https://lore.kernel.org/all/20260920015647.1783574-1-xiaolinkui@126.com/

Changes in v3:
- Only run the TX checks when the index carries a TX ring, and the rx_riwt
  range check only when it carries an RX ring. Moving the checks alone
  rejected per-queue RX coalescing on an RX-only index and broke the
  ethtool core rollback for it. (Sashiko review)
- Reword the subject and changelog to describe the direction-aware
  validation.

 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  | 35 +++++++++++++------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 154cc0c7623d..fed648a9f784 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -837,6 +837,8 @@ static int __stmmac_set_coalesce(struct net_device *dev,
 	struct stmmac_priv *priv = netdev_priv(dev);
 	bool all_queues = false;
 	unsigned int rx_riwt;
+	bool has_rx;
+	bool has_tx;
 	u32 max_cnt;
 	u32 rx_cnt;
 	u32 tx_cnt;
@@ -850,7 +852,20 @@ static int __stmmac_set_coalesce(struct net_device *dev,
 	else if (queue >= max_cnt)
 		return -EINVAL;
 
-	if (priv->use_riwt) {
+	has_rx = all_queues || queue < rx_cnt;
+	has_tx = all_queues || queue < tx_cnt;
+
+	/* An index can be RX-only or TX-only; ignore the missing side. */
+	if (has_tx &&
+	    ec->tx_coalesce_usecs == 0 && ec->tx_max_coalesced_frames == 0)
+		return -EINVAL;
+
+	if (has_tx &&
+	    (ec->tx_coalesce_usecs > STMMAC_MAX_COAL_TX_TICK ||
+	     ec->tx_max_coalesced_frames > STMMAC_TX_MAX_FRAMES))
+		return -EINVAL;
+
+	if (has_rx && priv->use_riwt) {
 		rx_riwt = stmmac_usec2riwt(ec->rx_coalesce_usecs, priv);
 
 		if ((rx_riwt > MAX_DMA_RIWT) || (rx_riwt < MIN_DMA_RIWT))
@@ -866,7 +881,7 @@ static int __stmmac_set_coalesce(struct net_device *dev,
 				priv->rx_coal_frames[i] =
 					ec->rx_max_coalesced_frames;
 			}
-		} else if (queue < rx_cnt) {
+		} else {
 			priv->rx_riwt[queue] = rx_riwt;
 			stmmac_rx_watchdog(priv, priv->ioaddr,
 					   rx_riwt, queue);
@@ -875,14 +890,6 @@ static int __stmmac_set_coalesce(struct net_device *dev,
 		}
 	}
 
-	if ((ec->tx_coalesce_usecs == 0) &&
-	    (ec->tx_max_coalesced_frames == 0))
-		return -EINVAL;
-
-	if ((ec->tx_coalesce_usecs > STMMAC_MAX_COAL_TX_TICK) ||
-	    (ec->tx_max_coalesced_frames > STMMAC_TX_MAX_FRAMES))
-		return -EINVAL;
-
 	if (all_queues) {
 		int i;
 
@@ -892,7 +899,7 @@ static int __stmmac_set_coalesce(struct net_device *dev,
 			priv->tx_coal_timer[i] =
 				ec->tx_coalesce_usecs;
 		}
-	} else if (queue < tx_cnt) {
+	} else if (has_tx) {
 		priv->tx_coal_frames[queue] =
 			ec->tx_max_coalesced_frames;
 		priv->tx_coal_timer[queue] =
-- 
2.25.1

[PATCH net v3 2/2] net: stmmac: fix a divide by zero in stmmac_xdp_xmit_xdpf()

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

tx-frames 0 is a valid coalescing request: it only stops the frame count
from raising TX completion interrupts, while the coalescing timer armed
by stmmac_tx_timer_arm() keeps reclaiming the descriptors. That is why
stmmac_xmit(), stmmac_tso_xmit() and stmmac_xdp_xmit_zc() all test
priv->tx_coal_frames[queue] before taking the modulo, and why
__stmmac_set_coalesce() rejects the request only when tx-usecs is zero
as well, since then nothing would complete the transmissions.

stmmac_xdp_xmit_xdpf() is the one transmit path that takes the modulo
without the test, so

	ethtool -C eth0 tx-usecs 10 tx-frames 0

followed by an XDP_TX or an ndo_xdp_xmit frame divides by zero, which
oopses in softirq context on the architectures that trap on a zero
divisor.

Add the missing test, which leaves set_ic false exactly like the other
transmit paths do.

Fixes: be8b38a722e6 ("net: stmmac: Add support for XDP_TX action")
Cc: stable@vger.kernel.org
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
v3:
- New patch. stmmac_xdp_xmit_xdpf() is the only transmit path that divides
  by tx_coal_frames[queue] without a zero test; add the missing test the
  way stmmac_xdp_xmit_zc() has it. (Sashiko review)

 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index e2e680dd980c..276187f50ee3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5256,7 +5256,9 @@ static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,
 
 	tx_q->tx_count_frames++;
 
-	if (tx_q->tx_count_frames % priv->tx_coal_frames[queue] == 0)
+	if (!priv->tx_coal_frames[queue])
+		set_ic = false;
+	else if (tx_q->tx_count_frames % priv->tx_coal_frames[queue] == 0)
 		set_ic = true;
 	else
 		set_ic = false;
-- 
2.25.1

Re: [PATCH net v3 2/2] net: stmmac: fix a divide by zero in stmmac_xdp_xmit_xdpf()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The zero guard added to stmmac_xdp_xmit_xdpf() performs two
  separate, unsynchronized plain loads of priv->tx_coal_frames[queue]:…

Pre-existing issues:
- [Medium] With `ethtool -C ... tx-frames 0`, the new branch in
  stmmac_xdp_xmit_xdpf()…

--

net: stmmac: fix a divide by zero in stmmac_xdp_xmit_xdpf()

This adds the missing zero test on priv->tx_coal_frames[queue] before the
modulo in stmmac_xdp_xmit_xdpf(), so that "ethtool -C eth0 tx-usecs 10
tx-frames 0" followed by an XDP_TX or ndo_xdp_xmit frame no longer divides
by zero. The other transmit paths already have this test.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 1fb5f804ea234..f9846250458a4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5253,7 +5253,9 @@ static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,
>  
>  	tx_q->tx_count_frames++;
>  
> -	if (tx_q->tx_count_frames % priv->tx_coal_frames[queue] == 0)
> +	if (!priv->tx_coal_frames[queue])
> +		set_ic = false;
> +	else if (tx_q->tx_count_frames % priv->tx_coal_frames[queue] == 0)
>  		set_ic = true;
>  	else
>  		set_ic = false;

[Severity: Medium]
Is one sample of priv->tx_coal_frames[queue] needed here instead of two?

The guard and the divisor are two separate plain loads of the same
concurrently written field, with no barrier or lock between them, so the
compiler is free to reload it for the modulo.

The writer side in __stmmac_set_coalesce() (stmmac_ethtool.c) stores it with
plain stores from process context, with no driver lock, no queue stop and no
NAPI quiescing:

	if (all_queues) {
		int i;

		for (i = 0; i < tx_cnt; i++) {
			priv->tx_coal_frames[i] =
				ec->tx_max_coalesced_frames;
			priv->tx_coal_timer[i] =
				ec->tx_coalesce_usecs;
		}
	} else if (has_tx) {
		priv->tx_coal_frames[queue] =
			ec->tx_max_coalesced_frames;

The reader in stmmac_xdp_xmit_xdpf() runs in softirq, reached from
stmmac_rx()/stmmac_rx_zc() via stmmac_xdp_xmit_back(), or from
stmmac_xdp_xmit(). If the guard load observes the old non-zero value and the
divisor load observes the freshly stored 0, the modulo still divides by zero
in softirq, which is the failure this commit is closing.

Would something like this be more robust?

	u32 coal = READ_ONCE(priv->tx_coal_frames[queue]);

	set_ic = coal && (tx_q->tx_count_frames % coal == 0);

The same double-read shape exists in stmmac_xmit(), stmmac_tso_xmit() and
stmmac_xdp_xmit_zc(), so a READ_ONCE() there too would keep the paths
consistent.

[Severity: Medium]
This isn't a bug introduced by this patch, but the commit message states
that with tx-frames 0 "the coalescing timer armed by stmmac_tx_timer_arm()
keeps reclaiming the descriptors". Does that hold for the AF_XDP zero-copy
XDP_TX path?

stmmac_finalize_xdp_rx() is the only arming attempt on that path:

	if (xdp_status & STMMAC_XDP_TX)
		stmmac_tx_timer_arm(priv, queue);

and stmmac_tx_timer_arm() picks the rxtx NAPI for an xsk_pool-backed queue,
which is the instance currently polling:

	napi = tx_q->xsk_pool ? &ch->rxtx_napi : &ch->tx_napi;
	...
	if (unlikely(!napi_is_scheduled(napi))) {
		...hrtimer_start(...)
	} else {
		hrtimer_try_to_cancel(&tx_q->txtimer);
	}

so napi_is_scheduled() is true and the timer is cancelled rather than armed.

The end-of-poll fallback in stmmac_napi_poll_rxtx() does not cover it
either, since tx_pending_packets is sampled before the XDP_TX descriptors
are produced:

	tx_done = stmmac_tx_clean(priv, budget, chan, &tx_pending_packets);
	tx_done = min(tx_done, budget);

	rx_done = stmmac_rx_zc(priv, budget, chan);
	...
	if (tx_pending_packets)
		stmmac_tx_timer_arm(priv, chan);

With tx-frames 0 no descriptor carries the interrupt-on-completion bit
either, so tx_q->dirty_tx stays behind tx_q->cur_tx and the dma_map_single()
mappings and xdp_frames of the last batch are held until unrelated traffic
causes another poll on that channel.

This window is reachable today with the defaults, since stmmac_init_coalesce()
sets

		priv->tx_coal_frames[chan] = STMMAC_TX_FRAMES;

and a zero-copy XDP_TX burst shorter than 25 frames likewise leaves set_ic
false while the timer is cancelled, so the patch only widens an existing
gap rather than creating one. Would it be worth dropping or softening the
claim that the timer always keeps reclaiming, or arming the timer after
stmmac_rx_zc() in stmmac_napi_poll_rxtx()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921135716.218420-1-xiaolinkui%40126.com

Re: [PATCH net v3 1/2] net: stmmac: ethtool: ignore inapplicable per-queue coalesce fields

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Commit message inaccuracy: the changelog states "The roll-back
  that ethtool_set_per_queue_coalesce() runs for the queues it has…

Pre-existing issues:
- [Medium] On a probed-but-never-opened stmmac interface, every coalesce
  set request is rejected with -EINVAL, including requests that only…
- [Medium] __stmmac_set_coalesce() updates priv->rx_riwt[],
  priv->rx_coal_frames[], priv->tx_coal_frames[] and…
- [Low] Documented ABI vs implementation mismatch in the paired getter:
  include/linux/ethtool.h (kernel-doc for…

--

net: stmmac: ethtool: ignore inapplicable per-queue coalesce fields

The patch makes __stmmac_set_coalesce() derive from the queue index whether
that index has an RX ring, a TX ring or both, and validates and applies only
the directions that exist.  The TX range checks are also moved ahead of the
RX apply block so a rejected request no longer leaves the RX half
reprogrammed.

Link: https://lore.kernel.org/all/20260920015647.1783574-1-xiaolinkui@126.com/

A few questions below, mostly about the changelog and about pre-existing
behaviour that sits next to the modified code.

The changelog says:

  "The roll-back that ethtool_set_per_queue_coalesce() runs
   for the queues it has already changed then trips over the same test and
   cannot restore them."

Is that the mechanism for the read-modify-write flow the changelog itself
describes?  In net/ethtool/ioctl.c the rollback is:

  roll_back:
	if (ret != 0) {
		tmp = backup;
		for_each_set_bit(i, queue_mask, bit) {
			dev->ethtool_ops->set_per_queue_coalesce(dev, i, tmp);

The loop stops at bit, so the failing index is never restored at all.  For
the dwmac-intel EHL example (rx_cnt = 6, tx_cnt = 4, mask 0x3f) indices 0-3
carry both directions, so their backups have non-zero TX fields from
stmmac_init_coalesce() and their restore succeeds.  The half-applied RX state
of index 4 survives because index 4 is excluded from the loop, not because
the rollback hits the TX zero test.

[Severity: Low]
Could that sentence be reworded to match what the rollback loop actually
does?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index 1be5310ca766c..daec9cf9eba16 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c

[ ... ]

> @@ -850,7 +852,20 @@ static int __stmmac_set_coalesce(struct net_device *dev,
>  	else if (queue >= max_cnt)
>  		return -EINVAL;
>  
> -	if (priv->use_riwt) {
> +	has_rx = all_queues || queue < rx_cnt;
> +	has_tx = all_queues || queue < tx_cnt;
> +
> +	/* An index can be RX-only or TX-only; ignore the missing side. */
> +	if (has_tx &&
> +	    ec->tx_coalesce_usecs == 0 && ec->tx_max_coalesced_frames == 0)
> +		return -EINVAL;
> +
> +	if (has_tx &&
> +	    (ec->tx_coalesce_usecs > STMMAC_MAX_COAL_TX_TICK ||
> +	     ec->tx_max_coalesced_frames > STMMAC_TX_MAX_FRAMES))
> +		return -EINVAL;

[Severity: Low]
This is a pre-existing issue and not introduced by this patch, but should the
paired getter be brought in line at the same time?  The kernel-doc for
ethtool_ops::get_per_queue_coalesce in include/linux/ethtool.h says:

 *	queue has this number, set the inapplicable fields to ~0 and return 0.

__stmmac_get_coalesce() writes 0 instead:

  drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c:__stmmac_get_coalesce() {
	if (queue < tx_cnt) {
		...
	} else {
		ec->tx_coalesce_usecs = 0;
		ec->tx_max_coalesced_frames = 0;
	}
	...
	} else {
		ec->rx_max_coalesced_frames = 0;
		ec->rx_coalesce_usecs = 0;
	}
  }

ethtool_get_per_queue_coalesce() copies the driver reply straight to
userspace without translating the sentinel, so ethtool -c --per-queue on an
RX-only index of a 6 RX / 4 TX dwmac-intel part prints tx-usecs: 0 and
tx-frames: 0, and userspace cannot tell "no TX ring at this index" from
"coalescing is 0".  Both halves come from the same commit named in the
Fixes: tag.

> +
> +	if (has_rx && priv->use_riwt) {
>  		rx_riwt = stmmac_usec2riwt(ec->rx_coalesce_usecs, priv);
>  
>  		if ((rx_riwt > MAX_DMA_RIWT) || (rx_riwt < MIN_DMA_RIWT))
> 			return -EINVAL;

[Severity: Medium]
This isn't introduced by this patch either, but does the same "usec 0 ->
riwt 0 -> -EINVAL" failure remain for indices that do have an RX ring before
the first open?

priv->use_riwt is set to 1 in stmmac_hw_init() at probe time, while
priv->rx_riwt[] only gets its DEF_DMA_RIWT default in stmmac_hw_setup():

  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_hw_setup() {
	...
			if (!priv->rx_riwt[queue])
				priv->rx_riwt[queue] = DEF_DMA_RIWT;
	...
  }

Between probe and the first ndo_open, rx_riwt[queue] is 0, so
__stmmac_get_coalesce() reports rx_coalesce_usecs = stmmac_riwt2usec(0) = 0.
__ethnl_set_coalesce() calls ops->get_coalesce and then overwrites only the
attributes present in the request before calling ops->set_coalesce, so that 0
is echoed back and this block computes rx_riwt = stmmac_usec2riwt(0) = 0 and
returns -EINVAL on rx_riwt < MIN_DMA_RIWT, discarding the TX part of the
request as well.

So ethtool -C eth0 tx-usecs 100, or ethtool --per-queue eth0 queue_mask 0x1
--coalesce tx-frames 10, fails until ip link set eth0 up has run once.  The
new has_rx gating removes this only for indices without an RX ring; queue 0
and the all-queues setter still take the path above.

> @@ -866,7 +881,7 @@ static int __stmmac_set_coalesce(struct net_device *dev,
>  				priv->rx_coal_frames[i] =
>  					ec->rx_max_coalesced_frames;
>  			}
> -		} else if (queue < rx_cnt) {
> +		} else {
>  			priv->rx_riwt[queue] = rx_riwt;
>  			stmmac_rx_watchdog(priv, priv->ioaddr,
>  					   rx_riwt, queue);

[ ... ]

> @@ -892,7 +899,7 @@ static int __stmmac_set_coalesce(struct net_device *dev,
>  			priv->tx_coal_timer[i] =
>  				ec->tx_coalesce_usecs;
>  		}
> -	} else if (queue < tx_cnt) {
> +	} else if (has_tx) {
>  		priv->tx_coal_frames[queue] =
>  			ec->tx_max_coalesced_frames;
>  		priv->tx_coal_timer[queue] =

[Severity: Medium]
This is also pre-existing and the changelog does not claim to address it, but
since this hunk newly makes the per-queue TX store reachable for TX-only
indices, is the lack of any serialization here worth a look?

These stores happen in process context under rtnl with no lock and no
WRITE_ONCE(), while the transmit paths read the same fields from softirq
context holding nothing in common:

  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_xmit() {
	...
	else if (!priv->tx_coal_frames[queue])
		set_ic = false;
	...
	else if ((tx_q->tx_count_frames %
		  priv->tx_coal_frames[queue]) < tx_packets)
	...
  }

The zero test and the modulo divisor are two separate plain loads of a value
the setter can change in between, and tx_coal_frames may legally be stored as
0 whenever tx_coalesce_usecs is non-zero.  The same check-then-reload appears
in stmmac_tso_xmit() and stmmac_xdp_xmit_zc(), and stmmac_tx_timer_arm()
reads priv->tx_coal_timer[queue] the same way, so a concurrent xmit can also
see the new frame threshold with the old timer.  Note the later
"net: stmmac: fix a divide by zero in stmmac_xdp_xmit_xdpf()" change adds the
missing zero test there but keeps the same unannotated pattern.

Would READ_ONCE()/WRITE_ONCE() on these fields, or reading the divisor once
into a local, be appropriate here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921135716.218420-1-xiaolinkui%40126.com