Re: [PATCH net-next v4 2/3] net: stmmac: align real_num_tx_queues to mqprio configuration

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

Critical: 0 · High: 1 · Medium: 0 (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).

- [High] At this commit, stmmac_hw_setup() (stmmac_main.c:3781) calls
  netif_set_real_num_tx_queues(dev, priv->xmit_qdisc.num_tx_queues) and…

Pre-existing issues:
- [Medium] After this patch, a device open (ifdown/ifup,
  stmmac_reinit_queues()/ethtool -L, resume) restores the software half…

--

net: stmmac: align real_num_tx_queues to mqprio configuration

This commit records the TX queue count of the current mqprio offload in
priv->xmit_qdisc.num_tx_queues. stmmac_hw_setup() then uses that value
for real_num_tx_queues on every open, so it stays consistent with the
netdev TC map. It also stops stmmac_reinit_queues() from lowering the TX
queue count below what an active mqprio offload needs.

[Severity: High]
Can this expose TX queues that have no rings allocated?

tc_setup_dwmac510_mqprio() sets num_tx_queues to max(offset + count)
from the user's qopt. The only range check is in
mqprio_validate_queue_counts(), and it compares against
dev->real_num_tx_queues, not plat->tx_queues_to_use:

net/sched/sch_mqprio_lib.c:mqprio_validate_queue_counts() {
    ...
	if (qopt->offset[i] >= dev->real_num_tx_queues ||
	    last > dev->real_num_tx_queues) {
    ...
}

While the interface is down, real_num_tx_queues can be larger than
tx_queues_to_use:

  - __stmmac_dvr_probe() allocates the netdev with MTL_MAX_TX_QUEUES and
    doesn't lower real_num_tx_queues before the first open.

  - stmmac_reinit_queues() on a down interface changes tx_queues_to_use
    but leaves real_num_tx_queues as it was.

stmmac_setup_tc() has no netif_running() check. Take tx_queues_to_use = 4
on a down interface and this sequence:

  tc qdisc add ... mqprio num_tc 2 ... queues 4@0 4@4 hw 1
  ip link set ... up

That stores xmit_qdisc.num_tx_queues = 8. On open,
alloc_dma_tx_desc_resources() only allocates rings for
queue < tx_queues_to_use, but stmmac_hw_setup() now publishes 8 real TX
queues.

Traffic mapped to TC1 then goes to queues 4-7, and stmmac_xmit() does:

	tx_q = &priv->dma_conf.tx_queue[queue];
	...
	WARN_ON(tx_q->tx_skbuff[first_entry]);

For those queues, tx_skbuff and the descriptor ring are NULL. Would this
cause a NULL pointer dereference in the TX path?

Before this patch, open always reset real_num_tx_queues to
tx_queues_to_use and netif_setup_tc() dropped the invalid TC map, so
this path couldn't be reached.

Later in the series, "net: stmmac: add tc mqprio hw queue priority
offload" adds a check that offset + count does not exceed
plat->tx_queues_to_use in tc_setup_dwmac510_mqprio(), which closes this.
However, this patch carries a Fixes: tag and the later patch doesn't.
Would a stable backport of this patch alone bring back the oops?

Should the tx_queues_to_use bound be part of this patch instead?

[ ... ]

[ ... ]

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. This
patch now restores the software side of the mqprio offload on every
open. Does anything restore the hardware side as well?

On open, stmmac_init_dma_engine() issues a DMA software reset. That
resets the MTL registers, including the FPE_MTL_PREEMPTION_CLASS field.

After that, stmmac_dma_operation_mode()->dwxgmac2_dma_tx_mode() rewrites
Q2TCMAP to the identity map:

	value |= FIELD_PREP(XGMAC_Q2TCMAP, channel);

The only function that programs the preemption class and the mqprio
Q2TCMAP is stmmac_fpe_map_preemption_class(). It is called only from the
taprio and mqprio setup and reset paths in stmmac_tc.c, never from open.

Consider ifdown/ifup, ethtool -L or resume. Afterwards real_num_tx_queues
and the netdev TC map still reflect the mqprio offload, and
xmit_qdisc.enabled stays true. The hardware, though, has no preemptible
queues and, on XGMAC, a 1:1 queue-to-TC map that doesn't match the qopt
offsets and counts.

mqprio->preemptible_tcs is not saved here, so the open path can't replay
it. The final patch in the series restores prio and algo from
xmit_qdisc in stmmac_mtl_configuration(). It still doesn't restore the
preemption class or Q2TCMAP.

Would it make sense to save preemptible_tcs in xmit_qdisc and reprogram
it from the open path?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001-stmmac-mqprio-prio-offload-v4-0-a74610d7f1e5%40oss.qualcomm.com