[PATCH net v3 1/2] net: stmmac: ethtool: ignore inapplicable per-queue coalesce fields
netdev-bot+sashiko at kernel.org
netdev-bot+sashiko at kernel.org
Tue Sep 22 21:58:02 PDT 2026
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
More information about the linux-arm-kernel
mailing list