Re: [PATCH net] net: stmmac: preserve real_num_tx_queues on mqprio setup failure

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

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

> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 1 · Medium: 1 · Low: 0
> 
> - [High] The patch silently changes the semantics of the shared dispatch
>   macro stmmac_fpe_map_preemption_class() in hwif.h from…
> - [Medium] The TC state that the new error path saves and restores
>   (num_tc via stmmac_set_ndev_tcs(), plus prio_tc_map) is immediately…
> 
> --
> 
> net: stmmac: preserve real_num_tx_queues on mqprio setup failure
> 
> This reworks tc_setup_dwmac510_mqprio() so that the previous num_tc,
> tc_to_txq and prio_tc_map values are saved and restored on failure instead
> of calling stmmac_reset_tc_mqprio(), and adds a stmmac_set_ndev_tcs()
> helper.  It also switches the stmmac_fpe_map_preemption_class() dispatch
> macro from stmmac_do_void_callback() to stmmac_do_callback().
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> > index 04dafec021b4..9314bcb85c22 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
> > +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> > @@ -494,7 +494,7 @@ struct stmmac_ops {
> >  #define stmmac_set_arp_offload(__priv, __args...) \
> >  	stmmac_do_void_callback(__priv, mac, set_arp_offload, __args)
> >  #define stmmac_fpe_map_preemption_class(__priv, __args...) \
> > -	stmmac_do_void_callback(__priv, mac, fpe_map_preemption_class, __args)
> > +	stmmac_do_callback(__priv, mac, fpe_map_preemption_class, __args)
> 
> [Severity: High]
> Should this macro conversion be called out in the commit message, and
> possibly split into its own patch?  It changes the semantics of a shared
> dispatch macro for all of its users, not only for the mqprio path.

I do not think so since this patch adds the only place where the
stmmac_fpe_map_preemption_class() return value is honored, all the other
stmmac_fpe_map_preemption_class() calls discard the error value.


[...]

> 
> Second, the same macro is used by tc_taprio_configure():
> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c:tc_taprio_configure() {
> 	...
> 	ret = stmmac_fpe_map_preemption_class(priv, priv->dev, extack,
> 					      qopt->mqprio.preemptible_tcs);
> 	if (ret)
> 		goto disable;
> 	...
> }
> 
> That "if (ret)" was dead code and becomes live here.  In
> dwmac5_fpe_map_preemption_class():
> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c {
> 	...
> 		if (priv->plat->tx_sched_algorithm == MTL_TX_ALGORITHM_SP) {
> 			NL_SET_ERR_MSG_MOD(extack, ALG_ERR_MSG);
> 			return -EINVAL;
> 		}
> 	...
> }
> 
> tx_sched_algorithm defaults to MTL_TX_ALGORITHM_SP for DT platforms in
> stmmac_platform.c, and the default per-queue weights differ (0x10 + queue),
> so a taprio or mqprio configuration with a preemptible TC owning more than
> one TXQ now gets -EINVAL where it previously returned success with the
> preemption-class register left unprogrammed.

I think it is better to return an error if the hw is not programmed properly
since otherwise the NIC is silently not working fine. The user can check and
understand what is the wrong configuration.

> 
> For taprio, the new error also takes the disable label, which clears
> priv->est->enable and reprograms EST, so the schedule that was just
> installed is torn down.  Is that intended for configurations that used to be
> accepted, for example:
> 
>   tc qdisc replace dev eth0 root taprio ... queues 2@0 2@2 ... fp E P
> 
> Also, the two remaining users that still ignore the now meaningful return
> value, the taprio disable path and stmmac_reset_tc_mqprio(), were not
> updated.  Should they be?
> 

[...]

> 
> [Severity: Medium]
> Can this restore of num_tc, tc_to_txq and prio_tc_map ever be observed?
> 
> tc_setup_dwmac510_mqprio() with num_tc != 0 is only reached from
> mqprio_enable_offload() inside mqprio_init(), and the error is returned
> before hw_offload is recorded:
> 
> net/sched/sch_mqprio.c:mqprio_enable_offload() {
> 	err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_MQPRIO,
> 					    &mqprio);
> 	if (err)
> 		return err;
> 
> 	priv->hw_offload = mqprio.qopt.hw;
> }
> 
> qdisc_create() then always runs ops->destroy() on init failure:
> 
> net/sched/sch_api.c:qdisc_create() {
> 	if (ops->init) {
> 		err = ops->init(sch, tca[TCA_OPTIONS], extack);
> 		if (err != 0)
> 			goto err_out4;
> 	}
> ...
> err_out4:
> 	if (ops->destroy)
> 		ops->destroy(sch);
> }
> 
> and with hw_offload still 0 the else branch is taken:
> 
> net/sched/sch_mqprio.c:mqprio_destroy() {
> 	if (priv->hw_offload && dev->netdev_ops->ndo_setup_tc)
> 		mqprio_disable_offload(sch);
> 	else
> 		netdev_set_num_tc(dev, 0);
> }
> 
> So dev->num_tc goes back to 0 immediately after the driver restored it, and
> skb_tx_hash() ignores tc_to_txq and prio_tc_map when num_tc is 0.  On a
> failing "tc qdisc replace" over an already offloaded mqprio, does the still
> grafted old qdisc lose its priority classification anyway?

I think in a error condition, the driver is supposed to restore the previous
overwritten values, then it is up to qdisc layer to manage the error condition.

Regards,
Lorenzo

> 
> If so, should the commit message text
> 
>   "the TC-to-queue and priority-to-TC mappings are restored to the
>    previously saved values"
> 
> be adjusted, since only the real_num_tx_queues part of the rollback appears
> to survive?
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903-stmmac-tc_setup_dwmac510_mqprio-error-path-v1-1-eb171bdd340a%40oss.qualcomm.com

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