[PATCH net-next v3] net: stmmac: drop the queue-0 pinning for GSO frames

[PATCH net-next v3] net: stmmac: drop the queue-0 pinning for GSO frames

[Lorenzo Bianconi]

stmmac_select_queue() forces every GSO frame (TCPv4/TCPv6/UDP-L4) onto
TX queue 0, relying on the legacy assumption that at least queue 0 is
always TSO/USO capable. This is no longer needed since
stmmac_features_check() already validates each GSO frame against the
capabilities of the queue it is mapped to, clearing the checksum and GSO
features for queues that cannot perform TSO (e.g. TBS queues or queues
without TX checksum offload support).

Remove stmmac_select_queue() so GSO frames are selected through the
regular queue selection path (XPS, flow hashing and priority mapping)
and can be spread across all TX queues. This also fixes the case where
queue 0 is not TSO-capable while other queues are, which previously
forced all TSO traffic into software GSO. Non-GSO traffic is unaffected,
since the core already falls back to netdev_pick_tx() when ndo_select_queue
is not set.

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
Changes in v3:
- Drop ndo_select_queue() callback and rely on network stack to select
  the proper TX queue.
- Link to v2: https://lore.kernel.org/r/20260808-stmmac_select_queue-tso-fix-v2-1-67175b29772e@oss.qualcomm.com

Changes in v2:
- Take into account when checksum offload is not supported.
- Link to v1: https://lore.kernel.org/r/20260807-stmmac_select_queue-tso-fix-v1-1-4c4787bad02b@oss.qualcomm.com
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index f2fc89176654..2eb1c42a7c27 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -6426,24 +6426,6 @@ static int stmmac_setup_tc(struct net_device *ndev, enum tc_setup_type type,
 	}
 }
 
-static u16 stmmac_select_queue(struct net_device *dev, struct sk_buff *skb,
-			       struct net_device *sb_dev)
-{
-	int gso = skb_shinfo(skb)->gso_type;
-
-	if (gso & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6 | SKB_GSO_UDP_L4)) {
-		/*
-		 * There is no way to determine the number of TSO/USO
-		 * capable Queues. Let's use always the Queue 0
-		 * because if TSO/USO is supported then at least this
-		 * one will be capable.
-		 */
-		return 0;
-	}
-
-	return netdev_pick_tx(dev, skb, NULL) % dev->real_num_tx_queues;
-}
-
 static int stmmac_set_mac_address(struct net_device *ndev, void *addr)
 {
 	struct stmmac_priv *priv = netdev_priv(ndev);
@@ -7298,7 +7280,6 @@ static const struct net_device_ops stmmac_netdev_ops = {
 	.ndo_eth_ioctl = stmmac_ioctl,
 	.ndo_get_stats64 = stmmac_get_stats64,
 	.ndo_setup_tc = stmmac_setup_tc,
-	.ndo_select_queue = stmmac_select_queue,
 	.ndo_set_mac_address = stmmac_set_mac_address,
 	.ndo_vlan_rx_add_vid = stmmac_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid = stmmac_vlan_rx_kill_vid,

---
base-commit: c4a839e06d0036c9a4af756d24688e0d34a18ca2
change-id: 20260807-stmmac_select_queue-tso-fix-06cc586d4022

Best regards,
-- 
Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

Re: [net-next,v3] net: stmmac: drop the queue-0 pinning for GSO frames

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 2 · Low: 1

- [High] Removing .ndo_select_queue silently disables hardware TSO/USO
  for most GSO flows on all in-tree stmmac platforms that mark every TX…
- [Medium] The patch's stated safety property — 'stmmac_features_check()
  already validates each GSO frame against the capabilities of the queue…
- [Medium] The patch deletes the hardware fact that the removed comment
  carried ('There is no way to determine the number of TSO/USO capable…
- [Low] The commit claims to fix a real user-visible offload defect
  ('This also fixes the case where queue 0 is not TSO-capable while…

--

net: stmmac: drop the queue-0 pinning for GSO frames

This drops stmmac_select_queue() and its .ndo_select_queue entry so GSO
frames are no longer pinned to TX queue 0. The changelog argues the pinning
is redundant because stmmac_features_check() already validates each GSO
frame against the capabilities of the queue it is mapped to. Non-GSO
traffic keeps going through netdev_pick_tx().

Link: https://lore.kernel.org/r/20260808-stmmac_select_queue-tso-fix-v2-1-67175b29772e@oss.qualcomm.com
Link: https://lore.kernel.org/r/20260807-stmmac_select_queue-tso-fix-v1-1-4c4787bad02b@oss.qualcomm.com

>From the changelog:

> This also fixes the case where queue 0 is not TSO-capable while other
> queues are, which previously forced all TSO traffic into software GSO.

[Severity: Low]
Since this is described as a fix, should it carry a Fixes tag? The queue-0
pinning being removed came in with commit 4993e5b37e8b ("net: stmmac:
Re-work the queue selection for TSO packets"), which in turn fixed
c5acdbee22a1 ("net: stmmac: Send TSO packets always from Queue 0").

The removal also depends on the per-queue checks in
stmmac_features_check() added by commit b55dfb173ce8 ("net: stmmac: move
check for hardware checksum supported") and the surrounding series. Without
those, backporting this alone removes the only thing that steered GSO
frames to a capable queue.

Could the changelog name those prerequisites so stable triage has
something to work from?

The claimed defect itself looks describable from DT, via
stmmac_mtl_setup():

	plat->tx_queues_cfg[queue].coe_unsupported =
		of_property_read_bool(q_node, "snps,coe-unsupported");

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 66ae365e1c89f..25121d6d84995 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6428,24 +6428,6 @@ static int stmmac_setup_tc(struct net_device *ndev, enum tc_setup_type type,
>  	}
>  }
>  
> -static u16 stmmac_select_queue(struct net_device *dev, struct sk_buff *skb,
> -			       struct net_device *sb_dev)
> -{
> -	int gso = skb_shinfo(skb)->gso_type;
> -
> -	if (gso & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6 | SKB_GSO_UDP_L4)) {
> -		/*
> -		 * There is no way to determine the number of TSO/USO
> -		 * capable Queues. Let's use always the Queue 0
> -		 * because if TSO/USO is supported then at least this
> -		 * one will be capable.
> -		 */
> -		return 0;
> -	}

[Severity: High]
Does this lose hardware TSO/USO for most GSO flows on the in-tree
platforms that mark every TX queue above queue 0 as TBS?

dwmac-intel.c, dwmac-imx.c, dwmac-mediatek.c, dwmac-qcom-ethqos.c and
stmmac_pci.c all do the equivalent of intel_mgbe_common_data():

	/* Default TX Q0 to use TSO and rest TXQ for TBS */
	if (i > 0)
		plat->tx_queues_cfg[i].tbs_en = 1;

That becomes STMMAC_TBS_AVAIL at open time in __stmmac_setup_dma_desc(),
with no ETF qdisc involved:

	int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;

	/* Setup per-TXQ tbs flag before TX descriptor alloc */
	tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;

and stmmac_tso_channel_permitted() then returns false for those channels:

	/* TSO and TBS cannot co-exist */
	return !(priv->dma_conf.tx_queue[chan].tbs & STMMAC_TBS_AVAIL);

The commit message says stmmac_features_check() "validates each GSO frame
against the capabilities of the queue it is mapped to", but the check only
demotes the frame, it does not move it:

	if (skb_is_gso(skb)) {
		if (!stmmac_tso_channel_permitted(priv, queue) ||
		    !stmmac_tso_valid_packet(skb))
			features &= ~NETIF_F_GSO_MASK;

So with the selector gone, netdev_pick_tx() hashes GSO flows over all N
queues and roughly (N-1)/N of TCP/UDP-GSO flows on these boards end up
segmented in software by validate_xmit_skb() rather than by the MAC. Is
that intended, and if so could the changelog mention the throughput and
CPU cost instead of describing the change as removing something that is
"no longer needed"?

Would a capability-aware queue selection (or a per-queue TSO map consulted
by the selector) be needed to keep the "Q0 is the TSO queue" contract those
platforms document?

> -
> -	return netdev_pick_tx(dev, skb, NULL) % dev->real_num_tx_queues;
> -}
> -

[Severity: Medium]
The deleted comment records a hardware fact: there is no way to determine
which TX DMA channels were synthesized with the TSO data path. DWMAC and
XGMAC only expose a single global TSOEN bit, surfaced as
priv->dma_cap.tsoen, and stmmac_hw_setup() programs TSE from it for every
non-TBS channel without any readback:

	if (priv->dma_cap.tsoen && priv->plat->flags & STMMAC_FLAG_TSO_EN) {
		for (chan = 0; chan < tx_cnt; chan++) {
			if (!stmmac_tso_channel_permitted(priv, chan))
				continue;

			stmmac_enable_tso(priv, priv->ioaddr, 1, chan);
		}
	}

stmmac_features_check() only knows about tx_queues_cfg[queue].coe_unsupported
and TBS, so on a part where only a subset of channels implement TSO it
cannot clear NETIF_F_GSO_MASK, and stmmac_xmit() then hands the frame to
stmmac_tso_xmit() on a channel whose DMA does not implement TSE. That
programs an MSS context descriptor plus TDES3 in TSE/THL/payload-length
form on a channel that decodes those bits as CIC and frame length.

Is there evidence that all channels are TSO-capable on the supported parts?
If not, would a per-queue tso capability consulted by
stmmac_features_check() be the replacement for the removed workaround?

>  static int stmmac_set_mac_address(struct net_device *ndev, void *addr)
>  {
>  	struct stmmac_priv *priv = netdev_priv(ndev);
> @@ -7302,7 +7284,6 @@ static const struct net_device_ops stmmac_netdev_ops = {
>  	.ndo_eth_ioctl = stmmac_ioctl,
>  	.ndo_get_stats64 = stmmac_get_stats64,
>  	.ndo_setup_tc = stmmac_setup_tc,
> -	.ndo_select_queue = stmmac_select_queue,

[Severity: Medium]
Does the "features_check validates the queue it is mapped to" argument hold
on the AF_PACKET PACKET_QDISC_BYPASS path? __dev_direct_xmit() runs the
feature validation before assigning the real queue:

net/core/dev.c:__dev_direct_xmit() {
	...
	skb = validate_xmit_skb_list(skb, dev, &again);
	if (skb != orig_skb)
		goto drop;

	skb_set_queue_mapping(skb, queue_id);
	...
}

and with .ndo_select_queue gone, packet_pick_tx_queue() takes the else
branch, so queue_id can be any queue:

net/packet/af_packet.c:packet_pick_tx_queue() {
	...
	skb_record_rx_queue(skb, cpu % dev->real_num_tx_queues);
	if (ops->ndo_select_queue) {
		queue_index = ops->ndo_select_queue(dev, skb, NULL);
		queue_index = netdev_cap_txqueue(dev, queue_index);
	} else {
		queue_index = netdev_pick_tx(dev, skb, NULL);
	}
	...
}

The mapping stmmac_features_check() reads via skb_get_queue_mapping() is
therefore (cpu % real_num_tx_queues) + 1, not queue_id. When the evaluated
queue is TSO-permitted but the queue actually used is not, NETIF_F_GSO_MASK
survives and stmmac_xmit() dispatches unconditionally:

	if (skb_is_gso(skb))
		return stmmac_tso_xmit(skb, dev);

Before this patch the same ordering existed but was harmless, because
packet_pick_tx_queue() consulted ndo_select_queue and GSO frames always
went out on queue 0. Is something needed to close that now?

Separately, that mapping can be real_num_tx_queues itself when
cpu % real_num_tx_queues == real_num_tx_queues - 1. Can this read past
priv->plat->tx_queues_cfg[] and priv->dma_conf.tx_queue[] in
stmmac_features_check() when real_num_tx_queues == MTL_MAX_TX_QUEUES?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-stmmac_select_queue-tso-fix-v3-1-d9c215fe7a85%40oss.qualcomm.com

Re: [net-next,v3] net: stmmac: drop the queue-0 pinning for GSO frames

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 9790 bytes --]

> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 1 · Medium: 2 · Low: 1
> 
> - [High] Removing .ndo_select_queue silently disables hardware TSO/USO
>   for most GSO flows on all in-tree stmmac platforms that mark every TX…
> - [Medium] The patch's stated safety property — 'stmmac_features_check()
>   already validates each GSO frame against the capabilities of the queue…
> - [Medium] The patch deletes the hardware fact that the removed comment
>   carried ('There is no way to determine the number of TSO/USO capable…
> - [Low] The commit claims to fix a real user-visible offload defect
>   ('This also fixes the case where queue 0 is not TSO-capable while…
> 
> --
> 
> net: stmmac: drop the queue-0 pinning for GSO frames
> 
> This drops stmmac_select_queue() and its .ndo_select_queue entry so GSO
> frames are no longer pinned to TX queue 0. The changelog argues the pinning
> is redundant because stmmac_features_check() already validates each GSO
> frame against the capabilities of the queue it is mapped to. Non-GSO
> traffic keeps going through netdev_pick_tx().
> 
> Link: https://lore.kernel.org/r/20260808-stmmac_select_queue-tso-fix-v2-1-67175b29772e@oss.qualcomm.com
> Link: https://lore.kernel.org/r/20260807-stmmac_select_queue-tso-fix-v1-1-4c4787bad02b@oss.qualcomm.com
> 
> >From the changelog:
> 
> > This also fixes the case where queue 0 is not TSO-capable while other
> > queues are, which previously forced all TSO traffic into software GSO.
> 
> [Severity: Low]
> Since this is described as a fix, should it carry a Fixes tag? The queue-0
> pinning being removed came in with commit 4993e5b37e8b ("net: stmmac:
> Re-work the queue selection for TSO packets"), which in turn fixed
> c5acdbee22a1 ("net: stmmac: Send TSO packets always from Queue 0").
> 
> The removal also depends on the per-queue checks in
> stmmac_features_check() added by commit b55dfb173ce8 ("net: stmmac: move
> check for hardware checksum supported") and the surrounding series. Without
> those, backporting this alone removes the only thing that steered GSO
> frames to a capable queue.
> 
> Could the changelog name those prerequisites so stable triage has
> something to work from?
> 
> The claimed defect itself looks describable from DT, via
> stmmac_mtl_setup():
> 
> 	plat->tx_queues_cfg[queue].coe_unsupported =
> 		of_property_read_bool(q_node, "snps,coe-unsupported");
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 66ae365e1c89f..25121d6d84995 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -6428,24 +6428,6 @@ static int stmmac_setup_tc(struct net_device *ndev, enum tc_setup_type type,
> >  	}
> >  }
> >  
> > -static u16 stmmac_select_queue(struct net_device *dev, struct sk_buff *skb,
> > -			       struct net_device *sb_dev)
> > -{
> > -	int gso = skb_shinfo(skb)->gso_type;
> > -
> > -	if (gso & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6 | SKB_GSO_UDP_L4)) {
> > -		/*
> > -		 * There is no way to determine the number of TSO/USO
> > -		 * capable Queues. Let's use always the Queue 0
> > -		 * because if TSO/USO is supported then at least this
> > -		 * one will be capable.
> > -		 */
> > -		return 0;
> > -	}
> 
> [Severity: High]
> Does this lose hardware TSO/USO for most GSO flows on the in-tree
> platforms that mark every TX queue above queue 0 as TBS?
> 
> dwmac-intel.c, dwmac-imx.c, dwmac-mediatek.c, dwmac-qcom-ethqos.c and
> stmmac_pci.c all do the equivalent of intel_mgbe_common_data():
> 
> 	/* Default TX Q0 to use TSO and rest TXQ for TBS */
> 	if (i > 0)
> 		plat->tx_queues_cfg[i].tbs_en = 1;
> 
> That becomes STMMAC_TBS_AVAIL at open time in __stmmac_setup_dma_desc(),
> with no ETF qdisc involved:
> 
> 	int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;
> 
> 	/* Setup per-TXQ tbs flag before TX descriptor alloc */
> 	tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
> 
> and stmmac_tso_channel_permitted() then returns false for those channels:
> 
> 	/* TSO and TBS cannot co-exist */
> 	return !(priv->dma_conf.tx_queue[chan].tbs & STMMAC_TBS_AVAIL);
> 
> The commit message says stmmac_features_check() "validates each GSO frame
> against the capabilities of the queue it is mapped to", but the check only
> demotes the frame, it does not move it:
> 
> 	if (skb_is_gso(skb)) {
> 		if (!stmmac_tso_channel_permitted(priv, queue) ||
> 		    !stmmac_tso_valid_packet(skb))
> 			features &= ~NETIF_F_GSO_MASK;
> 
> So with the selector gone, netdev_pick_tx() hashes GSO flows over all N
> queues and roughly (N-1)/N of TCP/UDP-GSO flows on these boards end up
> segmented in software by validate_xmit_skb() rather than by the MAC. Is
> that intended, and if so could the changelog mention the throughput and
> CPU cost instead of describing the change as removing something that is
> "no longer needed"?
> 
> Would a capability-aware queue selection (or a per-queue TSO map consulted
> by the selector) be needed to keep the "Q0 is the TSO queue" contract those
> platforms document?

We agreed in the previous disucssion to allow the networking stack to decide if
the selected queue should perform TSO running stmmac_features_check().
Moreover, there is no guarantee queue 0 will always support TSO for all the
platforms.

> 
> > -
> > -	return netdev_pick_tx(dev, skb, NULL) % dev->real_num_tx_queues;
> > -}
> > -
> 
> [Severity: Medium]
> The deleted comment records a hardware fact: there is no way to determine
> which TX DMA channels were synthesized with the TSO data path. DWMAC and
> XGMAC only expose a single global TSOEN bit, surfaced as
> priv->dma_cap.tsoen, and stmmac_hw_setup() programs TSE from it for every
> non-TBS channel without any readback:
> 
> 	if (priv->dma_cap.tsoen && priv->plat->flags & STMMAC_FLAG_TSO_EN) {
> 		for (chan = 0; chan < tx_cnt; chan++) {
> 			if (!stmmac_tso_channel_permitted(priv, chan))
> 				continue;
> 
> 			stmmac_enable_tso(priv, priv->ioaddr, 1, chan);
> 		}
> 	}
> 
> stmmac_features_check() only knows about tx_queues_cfg[queue].coe_unsupported
> and TBS, so on a part where only a subset of channels implement TSO it
> cannot clear NETIF_F_GSO_MASK, and stmmac_xmit() then hands the frame to
> stmmac_tso_xmit() on a channel whose DMA does not implement TSE. That
> programs an MSS context descriptor plus TDES3 in TSE/THL/payload-length
> form on a channel that decodes those bits as CIC and frame length.
> 
> Is there evidence that all channels are TSO-capable on the supported parts?
> If not, would a per-queue tso capability consulted by
> stmmac_features_check() be the replacement for the removed workaround?

As pointed out above I guess it is up to the networking stack to decide if hw
TSO can be used or the traffic should be segmented in sw. I do not think always
use queue 0 for TSO is the right approach.

> 
> >  static int stmmac_set_mac_address(struct net_device *ndev, void *addr)
> >  {
> >  	struct stmmac_priv *priv = netdev_priv(ndev);
> > @@ -7302,7 +7284,6 @@ static const struct net_device_ops stmmac_netdev_ops = {
> >  	.ndo_eth_ioctl = stmmac_ioctl,
> >  	.ndo_get_stats64 = stmmac_get_stats64,
> >  	.ndo_setup_tc = stmmac_setup_tc,
> > -	.ndo_select_queue = stmmac_select_queue,
> 
> [Severity: Medium]
> Does the "features_check validates the queue it is mapped to" argument hold
> on the AF_PACKET PACKET_QDISC_BYPASS path? __dev_direct_xmit() runs the
> feature validation before assigning the real queue:
> 
> net/core/dev.c:__dev_direct_xmit() {
> 	...
> 	skb = validate_xmit_skb_list(skb, dev, &again);
> 	if (skb != orig_skb)
> 		goto drop;
> 
> 	skb_set_queue_mapping(skb, queue_id);
> 	...
> }
> 
> and with .ndo_select_queue gone, packet_pick_tx_queue() takes the else
> branch, so queue_id can be any queue:
> 
> net/packet/af_packet.c:packet_pick_tx_queue() {
> 	...
> 	skb_record_rx_queue(skb, cpu % dev->real_num_tx_queues);
> 	if (ops->ndo_select_queue) {
> 		queue_index = ops->ndo_select_queue(dev, skb, NULL);
> 		queue_index = netdev_cap_txqueue(dev, queue_index);
> 	} else {
> 		queue_index = netdev_pick_tx(dev, skb, NULL);
> 	}
> 	...
> }
> 
> The mapping stmmac_features_check() reads via skb_get_queue_mapping() is
> therefore (cpu % real_num_tx_queues) + 1, not queue_id. When the evaluated
> queue is TSO-permitted but the queue actually used is not, NETIF_F_GSO_MASK
> survives and stmmac_xmit() dispatches unconditionally:
> 
> 	if (skb_is_gso(skb))
> 		return stmmac_tso_xmit(skb, dev);
> 
> Before this patch the same ordering existed but was harmless, because
> packet_pick_tx_queue() consulted ndo_select_queue and GSO frames always
> went out on queue 0. Is something needed to close that now?
> 
> Separately, that mapping can be real_num_tx_queues itself when
> cpu % real_num_tx_queues == real_num_tx_queues - 1. Can this read past
> priv->plat->tx_queues_cfg[] and priv->dma_conf.tx_queue[] in
> stmmac_features_check() when real_num_tx_queues == MTL_MAX_TX_QUEUES?

I guess this issue was already present even before this patch (e.g. for non-TSO
traffic transmitted by a AF_PACKET socket where PACKET_QDISC_BYPASS is
enabled). I guess we should fix it with a dedicated patch (e.g. verifying
queue is never greater than MTL_MAX_TX_QUEUES in stmmac_features_check()).

Regards,
Lorenzo

> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-stmmac_select_queue-tso-fix-v3-1-d9c215fe7a85%40oss.qualcomm.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [net-next,v3] net: stmmac: drop the queue-0 pinning for GSO frames

[Paolo Abeni]

On 9/2/26 8:48 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 1 · Medium: 2 · Low: 1
> 
> - [High] Removing .ndo_select_queue silently disables hardware TSO/USO
>   for most GSO flows on all in-tree stmmac platforms that mark every TX…
> - [Medium] The patch's stated safety property — 'stmmac_features_check()
>   already validates each GSO frame against the capabilities of the queue…
> - [Medium] The patch deletes the hardware fact that the removed comment
>   carried ('There is no way to determine the number of TSO/USO capable…
> - [Low] The commit claims to fix a real user-visible offload defect
>   ('This also fixes the case where queue 0 is not TSO-capable while…
> 
> --
> 
> net: stmmac: drop the queue-0 pinning for GSO frames
> 
> This drops stmmac_select_queue() and its .ndo_select_queue entry so GSO
> frames are no longer pinned to TX queue 0. The changelog argues the pinning
> is redundant because stmmac_features_check() already validates each GSO
> frame against the capabilities of the queue it is mapped to. Non-GSO
> traffic keeps going through netdev_pick_tx().
> 
> Link: https://lore.kernel.org/r/20260808-stmmac_select_queue-tso-fix-v2-1-67175b29772e@oss.qualcomm.com
> Link: https://lore.kernel.org/r/20260807-stmmac_select_queue-tso-fix-v1-1-4c4787bad02b@oss.qualcomm.com
> 
>>From the changelog:
> 
>> This also fixes the case where queue 0 is not TSO-capable while other
>> queues are, which previously forced all TSO traffic into software GSO.
> 
> [Severity: Low]
> Since this is described as a fix, should it carry a Fixes tag? The queue-0
> pinning being removed came in with commit 4993e5b37e8b ("net: stmmac:
> Re-work the queue selection for TSO packets"), which in turn fixed
> c5acdbee22a1 ("net: stmmac: Send TSO packets always from Queue 0").
> 
> The removal also depends on the per-queue checks in
> stmmac_features_check() added by commit b55dfb173ce8 ("net: stmmac: move
> check for hardware checksum supported") and the surrounding series. Without
> those, backporting this alone removes the only thing that steered GSO
> frames to a capable queue.
> 
> Could the changelog name those prerequisites so stable triage has
> something to work from?
> 
> The claimed defect itself looks describable from DT, via
> stmmac_mtl_setup():
> 
> 	plat->tx_queues_cfg[queue].coe_unsupported =
> 		of_property_read_bool(q_node, "snps,coe-unsupported");
> 
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 66ae365e1c89f..25121d6d84995 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -6428,24 +6428,6 @@ static int stmmac_setup_tc(struct net_device *ndev, enum tc_setup_type type,
>>  	}
>>  }
>>  
>> -static u16 stmmac_select_queue(struct net_device *dev, struct sk_buff *skb,
>> -			       struct net_device *sb_dev)
>> -{
>> -	int gso = skb_shinfo(skb)->gso_type;
>> -
>> -	if (gso & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6 | SKB_GSO_UDP_L4)) {
>> -		/*
>> -		 * There is no way to determine the number of TSO/USO
>> -		 * capable Queues. Let's use always the Queue 0
>> -		 * because if TSO/USO is supported then at least this
>> -		 * one will be capable.
>> -		 */
>> -		return 0;
>> -	}
> 
> [Severity: High]
> Does this lose hardware TSO/USO for most GSO flows on the in-tree
> platforms that mark every TX queue above queue 0 as TBS?
> 
> dwmac-intel.c, dwmac-imx.c, dwmac-mediatek.c, dwmac-qcom-ethqos.c and
> stmmac_pci.c all do the equivalent of intel_mgbe_common_data():
> 
> 	/* Default TX Q0 to use TSO and rest TXQ for TBS */
> 	if (i > 0)
> 		plat->tx_queues_cfg[i].tbs_en = 1;
> 
> That becomes STMMAC_TBS_AVAIL at open time in __stmmac_setup_dma_desc(),
> with no ETF qdisc involved:
> 
> 	int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;
> 
> 	/* Setup per-TXQ tbs flag before TX descriptor alloc */
> 	tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
> 
> and stmmac_tso_channel_permitted() then returns false for those channels:
> 
> 	/* TSO and TBS cannot co-exist */
> 	return !(priv->dma_conf.tx_queue[chan].tbs & STMMAC_TBS_AVAIL);
> 
> The commit message says stmmac_features_check() "validates each GSO frame
> against the capabilities of the queue it is mapped to", but the check only
> demotes the frame, it does not move it:
> 
> 	if (skb_is_gso(skb)) {
> 		if (!stmmac_tso_channel_permitted(priv, queue) ||
> 		    !stmmac_tso_valid_packet(skb))
> 			features &= ~NETIF_F_GSO_MASK;
> 
> So with the selector gone, netdev_pick_tx() hashes GSO flows over all N
> queues and roughly (N-1)/N of TCP/UDP-GSO flows on these boards end up
> segmented in software by validate_xmit_skb() rather than by the MAC. Is
> that intended, and if so could the changelog mention the throughput and
> CPU cost instead of describing the change as removing something that is
> "no longer needed"?
> 
> Would a capability-aware queue selection (or a per-queue TSO map consulted
> by the selector) be needed to keep the "Q0 is the TSO queue" contract those
> platforms document?
> 
>> -
>> -	return netdev_pick_tx(dev, skb, NULL) % dev->real_num_tx_queues;
>> -}
>> -
> 
> [Severity: Medium]
> The deleted comment records a hardware fact: there is no way to determine
> which TX DMA channels were synthesized with the TSO data path. DWMAC and
> XGMAC only expose a single global TSOEN bit, surfaced as
> priv->dma_cap.tsoen, and stmmac_hw_setup() programs TSE from it for every
> non-TBS channel without any readback:
> 
> 	if (priv->dma_cap.tsoen && priv->plat->flags & STMMAC_FLAG_TSO_EN) {
> 		for (chan = 0; chan < tx_cnt; chan++) {
> 			if (!stmmac_tso_channel_permitted(priv, chan))
> 				continue;
> 
> 			stmmac_enable_tso(priv, priv->ioaddr, 1, chan);
> 		}
> 	}
> 
> stmmac_features_check() only knows about tx_queues_cfg[queue].coe_unsupported
> and TBS, so on a part where only a subset of channels implement TSO it
> cannot clear NETIF_F_GSO_MASK, and stmmac_xmit() then hands the frame to
> stmmac_tso_xmit() on a channel whose DMA does not implement TSE. That
> programs an MSS context descriptor plus TDES3 in TSE/THL/payload-length
> form on a channel that decodes those bits as CIC and frame length.
> 
> Is there evidence that all channels are TSO-capable on the supported parts?
> If not, would a per-queue tso capability consulted by
> stmmac_features_check() be the replacement for the removed workaround?
> 
>>  static int stmmac_set_mac_address(struct net_device *ndev, void *addr)
>>  {
>>  	struct stmmac_priv *priv = netdev_priv(ndev);
>> @@ -7302,7 +7284,6 @@ static const struct net_device_ops stmmac_netdev_ops = {
>>  	.ndo_eth_ioctl = stmmac_ioctl,
>>  	.ndo_get_stats64 = stmmac_get_stats64,
>>  	.ndo_setup_tc = stmmac_setup_tc,
>> -	.ndo_select_queue = stmmac_select_queue,
> 
> [Severity: Medium]
> Does the "features_check validates the queue it is mapped to" argument hold
> on the AF_PACKET PACKET_QDISC_BYPASS path? __dev_direct_xmit() runs the
> feature validation before assigning the real queue:
> 
> net/core/dev.c:__dev_direct_xmit() {
> 	...
> 	skb = validate_xmit_skb_list(skb, dev, &again);
> 	if (skb != orig_skb)
> 		goto drop;
> 
> 	skb_set_queue_mapping(skb, queue_id);
> 	...
> }
> 
> and with .ndo_select_queue gone, packet_pick_tx_queue() takes the else
> branch, so queue_id can be any queue:
> 
> net/packet/af_packet.c:packet_pick_tx_queue() {
> 	...
> 	skb_record_rx_queue(skb, cpu % dev->real_num_tx_queues);
> 	if (ops->ndo_select_queue) {
> 		queue_index = ops->ndo_select_queue(dev, skb, NULL);
> 		queue_index = netdev_cap_txqueue(dev, queue_index);
> 	} else {
> 		queue_index = netdev_pick_tx(dev, skb, NULL);
> 	}
> 	...
> }
> 
> The mapping stmmac_features_check() reads via skb_get_queue_mapping() is
> therefore (cpu % real_num_tx_queues) + 1, not queue_id. When the evaluated
> queue is TSO-permitted but the queue actually used is not, NETIF_F_GSO_MASK
> survives and stmmac_xmit() dispatches unconditionally:
> 
> 	if (skb_is_gso(skb))
> 		return stmmac_tso_xmit(skb, dev);
> 
> Before this patch the same ordering existed but was harmless, because
> packet_pick_tx_queue() consulted ndo_select_queue and GSO frames always
> went out on queue 0. Is something needed to close that now?
> 
> Separately, that mapping can be real_num_tx_queues itself when
> cpu % real_num_tx_queues == real_num_tx_queues - 1. Can this read past
> priv->plat->tx_queues_cfg[] and priv->dma_conf.tx_queue[] in
> stmmac_features_check() when real_num_tx_queues == MTL_MAX_TX_QUEUES?
FTR the above looks like a giant slop to me. The main point is that the
pre-patch ndo_select_queue() is IMHO very bad, as e.g. it would cause
OoO and unfair queues load. Dropping it is the right thing to do.

/P

Re: [PATCH net-next v3] net: stmmac: drop the queue-0 pinning for GSO frames

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Mon, 31 Aug 2026 08:48:33 +0200 you wrote:
> stmmac_select_queue() forces every GSO frame (TCPv4/TCPv6/UDP-L4) onto
> TX queue 0, relying on the legacy assumption that at least queue 0 is
> always TSO/USO capable. This is no longer needed since
> stmmac_features_check() already validates each GSO frame against the
> capabilities of the queue it is mapped to, clearing the checksum and GSO
> features for queues that cannot perform TSO (e.g. TBS queues or queues
> without TX checksum offload support).
> 
> [...]

Here is the summary with links:
  - [net-next,v3] net: stmmac: drop the queue-0 pinning for GSO frames
    https://git.kernel.org/netdev/net-next/c/8e2faf945a13

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html