Re: [PATCH net-next] net: stmmac: fix mqprio and cbs combination

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

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

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · 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] The commit message says the strict-priority scheduler selected
>   by the channel-mode mqprio offload and the per-queue CBS shaper "are…
> 
> --
> 
> net: stmmac: fix mqprio and cbs combination
> 
> This patch removes the checks that stopped the channel-mode mqprio offload
> and the CBS offload from being enabled together. It drops the AVB queue
> rejection in tc_mqprio_validate_chan_mode(), the prio_offload check in
> tc_setup_cbs(), and the matching AVB check in stmmac_reinit_queues().
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > index ea1f97d6f0bd5..175236ba379d0 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> 
> [ ... ]
> 
> > @@ -1350,18 +1346,6 @@ static int tc_mqprio_validate_chan_mode(struct stmmac_priv *priv,
> >  		return -EOPNOTSUPP;
> >  	}
> >  
> > -	/* 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) {
> > -			NL_SET_ERR_MSG_MOD(mqprio->extack,
> > -					   "SP conflicts with AVB queues");
> > -			return -EOPNOTSUPP;
> > -		}
> > -	}
> > -
> 
> [Severity: High]
> 
> The commit message says:
> 
>   The strict-priority scheduler selected by the channel-mode mqprio
>   offload and the per-queue CBS shaper are independent MTL settings and do
>   not conflict
> 
> Is that true on XGMAC/XLGMAC cores? They use dwmac510_tc_ops, so both
> tc_setup_dwmac510_mqprio() and tc_setup_cbs() can be reached on them.
> 
> On XGMAC, SP and CBS look like two values of the same per-TC TSA field.
> dwxgmac2_prog_mtl_tx_algorithms() rewrites that field on every TC:
> 
> drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:dwxgmac2_prog_mtl_tx_algorithms() {
>     ...
> 	for (i = 0; i < MTL_MAX_TX_QUEUES; i++) {
> 		value = readl(ioaddr + XGMAC_MTL_TCx_ETS_CONTROL(i));
> 		value &= ~XGMAC_TSA;
> 		if (ets)
> 			value |= XGMAC_ETS;
> 		writel(value, ioaddr + XGMAC_MTL_TCx_ETS_CONTROL(i));
> 	}
> }
> 
> dwxgmac2_config_cbs() turns CBS on by writing the same field:
> 
> drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:dwxgmac2_config_cbs() {
>     ...
> 	value = readl(ioaddr + XGMAC_MTL_TCx_ETS_CONTROL(queue));
> 	value &= ~XGMAC_TSA;
> 	value |= XGMAC_CC | XGMAC_CBS;
> 	writel(value, ioaddr + XGMAC_MTL_TCx_ETS_CONTROL(queue));
> }
> 
> With this check gone, what happens if a TX queue is already in AVB mode?
> One way to get there is a DT mtl-tx-config queue with snps,avb-algorithm.
> In that case stmmac_open()->stmmac_mtl_configuration() programs CBS for the
> queue when the device is opened.

I guess the only case that is not covered yet is if the user configures CBS
via the device-tree and then applies mqprio via tc on top of it.
I will fix in v2.

Regards,
Lorenzo

> 
> After that, "tc qdisc replace ... mqprio ... mode channel hw 1" now
> succeeds and runs:
> 
> tc_setup_dwmac510_mqprio()
>   tc_mqprio_validate_chan_mode()
>   tc_mqprio_config_queue_prio()
>     stmmac_prog_mtl_tx_algorithms(priv, priv->hw, MTL_TX_ALGORITHM_SP)
>       dwxgmac2_prog_mtl_tx_algorithms()
> 
> Would this also clear TSA on the AVB queue and turn CBS off in hardware?
> Nothing calls stmmac_configure_cbs() again afterwards. Meanwhile
> tx_queues_cfg[].mode_to_use still says MTL_QUEUE_AVB, so the driver state
> and the hardware would disagree until the next ndo_open.
> 
> Removing mqprio seems to cause the same problem. prio_offload can now be
> true while AVB queues exist, and stmmac_reset_tc_mqprio() then does:
> 
> 	stmmac_prog_mtl_tx_algorithms(priv, priv->hw,
> 				      priv->plat->tx_sched_algorithm);
> 
> On XGMAC this rewrites TSA on every TC again (to ETS or SP). That would
> wipe CBS on any AVB queue and not restore it.
> 
> The example in the commit message (mqprio first, then the cbs children)
> works because config_cbs runs after the SP write.
> 
> On dwmac4/EQoS the claim does seem to hold.
> dwmac4_prog_mtl_tx_algorithms() only touches MTL_OPERATION_SCHALG_MASK, and
> CBS is enabled separately through MTL_ETS_CTRL_AVALG.
> 
> The commit in the Fixes: tag added these checks on purpose:
> 
>   Forcing strict priority conflicts with the CBS algorithm of AVB queues
> 
> Should the checks be kept for XGMAC? Or could the mqprio set and reset
> paths call stmmac_configure_cbs() again after reprogramming the TX
> algorithm?
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-stmmac-mqprio-cbs-combo-v1-1-831e323eb4ef%40oss.qualcomm.com

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