[PATCH net-next] net: stmmac: add tc-mqprio qdisc offload
Lorenzo Bianconi
lorenzo.bianconi at oss.qualcomm.com
Wed Sep 23 09:54:40 PDT 2026
> Thank you for your contribution! Sashiko AI review found 6 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 6 (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] One-directional AVB/CBS rejection: tc_setup_dwmac510_mqprio()
> rejects the mqprio offload if any TX queue is already MTL_QUEUE_AVB…
> - [Medium] The patch narrows an mqprio offload that already existed,
> breaking configurations the driver accepted before, and the commit…
> - [Medium] The new per-TC offset validation bounds qopt->offset[i]
> against qopt->num_tc (stmmac_tc.c:1340-1343, extack 'TX queue range…
> - [Medium] Replacing an offloaded mqprio qdisc makes the old qdisc's
> teardown wipe the replacement's just-installed hardware state.
> - [Medium] The new mqprio setup and teardown paths perform read-modify-
> write MMIO from ndo_setup_tc without a runtime-PM reference and…
> - [Medium] XGMAC index mismatch: the loop at stmmac_tc.c:1367-1390
> resolves, for each TX queue index i, the traffic class j that owns…
>
> Pre-existing issues:
> - [Medium] The mqprio offload accepts TC_MQPRIO_MODE_CHANNEL with
> TC_MQPRIO_SHAPER_BW_RATE and returns success while ignoring…
>
> --
>
> net: stmmac: add tc-mqprio qdisc offload
>
> The patch implements tc-mqprio offload in stmmac by switching the MTL TX
> scheduler to strict priority and programming each TX queue's PSTQX/PSTC
> priority bitmask from qopt->prio_tc_map. The per-queue masks and the
> scheduling algorithm are cached in the new priv->xmit_qdisc so they can be
> re-applied on reopen, and teardown restores the devicetree configuration.
>
> A few questions below about the new validation and register programming.
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
> > index c889204a7aa5..b6b5ef7c8fc4 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
> > @@ -230,7 +230,7 @@ int dwmac5_fpe_map_preemption_class(struct net_device *ndev,
> > if (count == 1)
> > continue;
> >
> > - if (priv->plat->tx_sched_algorithm == MTL_TX_ALGORITHM_SP) {
> > + if (priv->xmit_qdisc.algo == MTL_TX_ALGORITHM_SP) {
> > NL_SET_ERR_MSG_MOD(extack, ALG_ERR_MSG);
> > return -EINVAL;
> > }
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index bf9e7e4cb1c3..0c9083f8846e 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>
> [ ... ]
>
> > @@ -3604,7 +3598,7 @@ static void stmmac_mtl_configuration(struct stmmac_priv *priv)
> > /* Configure MTL TX algorithms */
> > if (tx_queues_count > 1)
> > stmmac_prog_mtl_tx_algorithms(priv, priv->hw,
> > - priv->plat->tx_sched_algorithm);
> > + priv->xmit_qdisc.algo);
> >
> > /* Configure CBS in AVB TX queues */
> > if (tx_queues_count > 1)
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > index 42a00446e9b4..a2a9680da5c0 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > @@ -1265,6 +1265,23 @@ static int stmmac_reset_tc_mqprio(struct net_device *ndev,
> > struct netlink_ext_ack *extack)
> > {
> > struct stmmac_priv *priv = netdev_priv(ndev);
> > + int i;
> > +
> > + for (i = 0; i < priv->plat->tx_queues_to_use; i++) {
> > + u32 prio;
> > +
> > + if (priv->plat->tx_queues_cfg[i].use_prio)
> > + prio = priv->plat->tx_queues_cfg[i].prio;
> > + else
> > + prio = 0;
> > +
> > + stmmac_tx_queue_prio(priv, priv->hw, prio, i);
> > + priv->xmit_qdisc.prio[i] = prio;
> > + }
> > +
> > + stmmac_prog_mtl_tx_algorithms(priv, priv->hw,
> > + priv->plat->tx_sched_algorithm);
> > + priv->xmit_qdisc.algo = priv->plat->tx_sched_algorithm;
> >
> > netdev_reset_tc(ndev);
> > netif_set_real_num_tx_queues(ndev, priv->plat->tx_queues_to_use);
>
> [Severity: Medium]
> Can a qdisc replace end up undoing the offload that was just installed?
>
> For "tc qdisc replace dev X root mqprio ... hw 1" on top of an already
> offloaded mqprio, the core creates and offloads the new qdisc first, and
> only then destroys the old one:
>
> net/sched/sch_api.c:qdisc_graft() {
> ...
> notify_and_destroy(net, skb, n, classid, old, new, extack);
>
> if (new && new->ops->attach)
> new->ops->attach(new);
> }
>
> qdisc_put(old) -> mqprio_destroy() -> mqprio_disable_offload() issues
> num_tc == 0, which tc_setup_dwmac510_mqprio() forwards to
> stmmac_reset_tc_mqprio(). That now rewrites the PSTQX priorities and the
> MTL scheduling algorithm back to the devicetree values, and also resets
> priv->xmit_qdisc, while mqprio_attach() does not re-apply the offload.
>
> The result looks like the replacement qdisc reporting hw offload while the
> MAC is back on the devicetree scheduler and priorities, and the stale
> cached state is re-applied on the next open. Should the reset path check
> whether an offloaded mqprio is still installed before reprogramming?
I guess this issue is already present and it has not been introduced by
this patch. I think the problem should be fixed with a dedicated patch.
Moreover, the fix would require to change sch_mqprio in order to provide
the handle of the offloaded qdisc.
>
> [Severity: Medium]
> Is it safe to touch these registers from ndo_setup_tc without a runtime PM
> reference?
>
> stmmac_reset_tc_mqprio() and the setup path both call
> stmmac_tx_queue_prio() and stmmac_prog_mtl_tx_algorithms() unconditionally,
> and those are read-modify-write accesses, e.g.:
>
> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:dwmac4_tx_queue_priority() {
> value = readl(ioaddr + base_register);
>
> value &= ~GMAC_TXQCTRL_PSTQX_MASK(queue);
> value |= (prio << GMAC_TXQCTRL_PSTQX_SHIFT(queue)) & ...;
>
> writel(value, ioaddr + base_register);
> }
>
> The driver drops its PM reference at the end of probe and in
> stmmac_release(), and stmmac_runtime_suspend() gates the bus/CSR clocks via
> stmmac_bus_clks_config(priv, false). Adding or deleting an mqprio qdisc on
> a down interface therefore reaches these accesses with the MAC clock
> gated, where the readl can return an undefined value that is then written
> back over the other queues' PSTQ fields and the other MTL_Operation_Mode
> bits.
I think this issue is not introduce by this patch and it is already present (so
it needs to be fixed with a dedicated patch). There is already a patch upstream
to resolve the problem:
https://lore.kernel.org/netdev/20260827-stmmac-setup-tc-enable-pm-v2-1-a9b8a5948f41@oss.qualcomm.com/
>
> Other stmmac control paths that touch registers outside open (set_mac_address,
> VLAN add/del) wrap the access in pm_runtime_get_sync()/pm_runtime_put().
> Would a similar guard, or a netif_running() check, be appropriate here?
> These writes also look redundant while the device is down, since
> stmmac_mtl_configuration() re-applies priv->xmit_qdisc on the next open.
>
> > @@ -1290,6 +1308,17 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
> > if (qopt->num_tc > ARRAY_SIZE(tc_to_txq))
> > return -EINVAL;
> >
> > + if (!priv->dma_cap.dcben)
> > + return -EOPNOTSUPP;
> > +
>
> [Severity: Medium]
> This is a pre-existing issue and not introduced by this patch, but since
> the entry validation is being extended here: should mqprio->mode,
> mqprio->shaper, mqprio->min_rate[] and mqprio->max_rate[] be rejected?
As pointed out by sashiko, this issue is already preset and it should be fixed
with a dedicated patch.
>
> net/sched/sch_mqprio.c:mqprio_enable_offload() copies those into
> tc_mqprio_qopt_offload for TC_MQPRIO_MODE_CHANNEL and relies on the driver
> to refuse what it cannot do:
>
> case TC_MQPRIO_MODE_CHANNEL:
> mqprio.flags = priv->flags;
> ...
> if (priv->flags & TC_MQPRIO_F_MAX_RATE)
> for (i = 0; i < mqprio.qopt.num_tc; i++)
> mqprio.max_rate[i] = priv->max_rate[i];
>
> tc_setup_dwmac510_mqprio() only consumes qopt and preemptible_tcs, so
> "mode channel shaper bw_rlimit max_rate ... hw 1" returns success and the
> rate limits are echoed back to userspace without any shaping being
> programmed.
>
> [Severity: Medium]
> Does this narrow an offload that already worked?
>
> The baseline tc_setup_dwmac510_mqprio() validated only num_tc == 0 and
> num_tc > ARRAY_SIZE(tc_to_txq). The new !priv->dma_cap.dcben check, the
> AVB-queue check below, and the count[i] > 1 check make configurations that
> previously succeeded return -EOPNOTSUPP, including the FPE preemptible-TC
> mapping part of the offload which is unrelated to the new priority
> programming.
>
> The count > 1 case looks like it was intentionally supported:
>
> drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c:dwmac5_fpe_map_preemption_class() {
> ...
> if (count == 1)
> continue;
>
> if (priv->xmit_qdisc.algo == MTL_TX_ALGORITHM_SP) {
> NL_SET_ERR_MSG_MOD(extack, ALG_ERR_MSG);
> return -EINVAL;
> }
>
> queue_weight = priv->plat->tx_queues_cfg[offset].weight;
> ...
> }
>
> That weight-checking branch becomes unreachable from the mqprio path now
> that the algorithm is forced to SP before it runs. Configurations such as
> "mqprio ... queues 2 at 0 2 at 2 fp E P hw 1" lose the offload.
I think the new sanity checks are required in order to properly support SP
priority offloading based on the hw capabilities. There were not necessary
before since this hw capability was not available.
>
> Additionally, forcing MTL_TX_ALGORITHM_SP overrides the devicetree
> tx-sched-algorithm and the devicetree per-queue priority masks for every
> mqprio user that is still accepted. Should the commit message mention
> these behaviour changes for existing users rather than describing them only
> as offload requirements?
>
> > + /* Forcing strict priority conflicts with the CBS algorithm of AVB
> > + * queues, so reject the offload when any queue is configured as AVB.
> > + */
> > + for (i = 0; i < pdata->tx_queues_to_use; i++) {
> > + if (pdata->tx_queues_cfg[i].mode_to_use == MTL_QUEUE_AVB)
> > + return -EOPNOTSUPP;
> > + }
> > +
>
> [Severity: Medium]
> Is this rejection effective in only one direction?
>
> tc_setup_cbs() in the same file has no reciprocal check. It validates the
> queue index, priv->dma_cap.av and the slope parameters, then converts a DCB
> queue into an AVB queue and programs the shaper:
>
> mode_to_use = priv->plat->tx_queues_cfg[queue].mode_to_use;
> if (mode_to_use == MTL_QUEUE_DCB && qopt->enable) {
> ret = stmmac_dma_qmode(priv, priv->ioaddr, queue, MTL_QUEUE_AVB);
> ...
> priv->plat->tx_queues_cfg[queue].mode_to_use = MTL_QUEUE_AVB;
>
> Nothing there consults priv->xmit_qdisc.algo or netdev_get_num_tc(), so the
> usual TSN order
>
> tc qdisc add dev X root mqprio ... hw 1
> tc qdisc replace dev X parent <mqprio child> cbs ... offload 1
>
> succeeds and leaves the MAC in strict priority with a CBS/AVB queue, which
> is the combination this new check and the commit message declare
> unsupported.
>
> The mismatch also survives a reopen, since stmmac_mtl_configuration()
> programs the cached SP algorithm and then re-applies CBS:
>
> stmmac_prog_mtl_tx_algorithms(priv, priv->hw, priv->xmit_qdisc.algo);
> /* Configure CBS in AVB TX queues */
> if (tx_queues_count > 1)
> stmmac_configure_cbs(priv);
>
> Should tc_setup_cbs() carry the symmetric check on priv->xmit_qdisc.algo,
> or is the mqprio-side rejection not needed?
I think even this one is an issue already present since the user can provide
queue priorities via device tree and at the same time create a cbs qdisc
offloaded in hw. I think we should provide a fix that needs to be backported
here.
>
> > @@ -1299,6 +1328,20 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
> > ndev_prio_tc_map[i] = READ_ONCE(ndev->prio_tc_map[i]);
> >
> > for (i = 0; i < qopt->num_tc; i++) {
> > + /* The offload switches the MTL scheduler to strict priority,
> > + * which only supports a 1:1 TC to TX queue mapping.
> > + */
> > + if (qopt->count[i] > 1) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "SP sched requires 1:1 TXQ map");
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + if (qopt->offset[i] >= qopt->num_tc) {
> > + NL_SET_ERR_MSG_MOD(extack, "TX queue range exceeded");
> > + return -EINVAL;
> > + }
> > +
>
> [Severity: Medium]
> Should this bound be priv->plat->tx_queues_to_use rather than
> qopt->num_tc?
>
> num_tc is only limited by ARRAY_SIZE(tc_to_txq) here and by the core:
>
> 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) {
> ...
> }
>
> stmmac allocates the netdev with MTL_MAX_TX_QUEUES queues and only clamps
> real_num_tx_queues to plat->tx_queues_to_use in stmmac_hw_setup(), so
> before the first open a request like "mqprio num_tc 4 ... queues 1 at 0 1 at 1
> 1 at 2 1 at 3 hw 1" on a device with tx_queues_to_use == 2 passes both checks.
>
> The priority programming loop below is bounded by pdata->tx_queues_to_use,
> so the TCs with offset >= tx_queues_to_use never get a PSTQX/PSTC mask
> while the offload still returns success, and netif_setup_tc() remaps those
> priorities to TC0 at the next open.
>
> The same check also rejects a sparse but otherwise legal map, e.g.
> num_tc 2 with "queues 1 at 0 1 at 2". Is that intentional?
As the previous ones, even this issue is reachable even without this patch
applied. I think we should address it in net with a proper fixes tag.
>
> > @@ -1315,14 +1358,44 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
> > if (err)
> > goto error_reset_tc;
> >
> > + priv->xmit_qdisc.algo = MTL_TX_ALGORITHM_SP;
> > err = stmmac_fpe_map_preemption_class(priv, ndev, extack,
> > mqprio->preemptible_tcs);
> > if (err)
> > - goto error_reset_num_tx_queues;
> > + goto error_reset_xmit_algo;
> > +
> > + for (i = 0; i < pdata->tx_queues_to_use; i++) {
> > + u32 prio = 0;
> > + int j;
> > +
> > + for (j = 0; j < qopt->num_tc; j++) {
> > + int p;
> > +
> > + if (qopt->offset[j] != i)
> > + continue;
> > +
> > + /* The PSTQX/PSTC priority map is 8 bits wide, so only
> > + * priorities 0-7 can be represented in hardware.
> > + * Priorities 8-15 are handled in software by the
> > + * kernel through the netdev prio_tc_map.
> > + */
> > + for (p = 0; p < 8; p++) {
> > + if (qopt->prio_tc_map[p] == j)
> > + prio |= BIT(p);
> > + }
> > + break;
> > + }
> > +
> > + stmmac_tx_queue_prio(priv, priv->hw, prio, i);
>
> [Severity: Medium]
> Is the last argument the right index on XGMAC?
>
> The mask computed here belongs to traffic class j, but the index passed to
> stmmac_tx_queue_prio() is the queue index i. For XGMAC the callback is:
>
> drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:dwxgmac2_tx_queue_prio() {
> reg = (queue < 4) ? XGMAC_TC_PRTY_MAP0 : XGMAC_TC_PRTY_MAP1;
> ...
> value &= ~XGMAC_PSTC(queue);
> value |= (prio << XGMAC_PSTC_SHIFT(queue)) & XGMAC_PSTC(queue);
> }
>
> XGMAC_PSTC() selects a traffic-class field in MTL_TC_Prty_Map0/1, while the
> queue to TC association is programmed separately in XGMAC_Q2TCMAP:
>
> drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c:dwxgmac3_fpe_map_preemption_class() {
> for (u32 i = 0; i < count; i++) {
> val = readl(priv->ioaddr + XGMAC_MTL_TXQ_OPMODE(offset + i));
> writel(u32_replace_bits(val, tc, XGMAC_Q2TCMAP), ...);
> }
>
> Both dwxgmac210 and dwxlgmac2 use dwmac510_tc_ops, and a permuted map such
> as "mqprio num_tc 2 map ... queues 1 at 1 1 at 0 hw 1" passes both
> intervals_overlap() in the core and the new offset < num_tc check. In that
> case TC0's mask would be written into the PSTC field of TC1 and vice versa.
> Should the XGMAC path program PSTC using the TC index, or should
> non-identity offsets be rejected?
I think the issue is real since in dwmac4_tx_queue_priority(, queue) queue
index is the real hw queue, while in dwxgmac2_tx_queue_prio(, queue) queue
index is actually a tc index (even if it called queue).
Moreover, the dts path just assumes the tc index is equal to queue index.
I guess we should just forbid this configuration in order to keep dts and tc
paths aligned. What do you think?
Regards,
Lorenzo
>
> > + priv->xmit_qdisc.prio[i] = prio;
> > + }
> > +
> > + stmmac_prog_mtl_tx_algorithms(priv, priv->hw, MTL_TX_ALGORITHM_SP);
> >
> > return 0;
> >
> > -error_reset_num_tx_queues:
> > +error_reset_xmit_algo:
> > + priv->xmit_qdisc.algo = priv->plat->tx_sched_algorithm;
> > if (netif_set_real_num_tx_queues(ndev, ndev_num_tx_queues))
> > netdev_warn(ndev, "Failed to restore %u TX queues\n",
> > ndev_num_tx_queues);
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918-stmmac-mqprio-prio-offload-v1-1-5328157fcb58%40oss.qualcomm.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20260923/1d412c77/attachment-0001.sig>
More information about the linux-arm-kernel
mailing list