Re: [PATCH net v3 1/2] net: stmmac: propagate FPE preemption-class mapping errors

commenting on sashiko's report:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260911-stmmac-tc_setup_dwmac510_mqprio-error-path-v3-0-a76b1e2547c1%40oss.qualcomm.com

stmmac_fpe_map_preemption_class() dispatches through the
stmmac_do_void_callback() helper, which forces the callback's return
value to 0 whenever the op pointer is populated. As a result the
-EINVAL returned by dwmac5_fpe_map_preemption_class() (e.g. when a
preemptible TC owns more than one TXQ under SP scheduling) is silently
swallowed by every caller.

Switch the dispatch macro to stmmac_do_callback() so the callback's real
result is propagated, and honour it in the taprio and mqprio qdisc
offload.

Note that the taprio "if (ret)" check in tc_taprio_configure() used to
be dead code and now becomes live: a preemptible TC spanning more than
one TXQ under SP scheduling cannot be programmed in hardware, so a
taprio or mqprio configuration that previously returned success while
leaving the preemption-class register unprogrammed now fails with
-EINVAL. For taprio, the failure also runs the disable path, tearing
down the schedule that was just installed; this is the intended
behaviour.

- Is the described condition narrower than what the code actually rejects?
  In dwmac5_fpe_map_preemption_class() the SP rejection is not gated on the
  TC being preemptible:
  - I guess we can do a little improvement of the commit message if I need to
    repost.

Fixes: 195e4f409a40 ("net: stmmac: support fp parameter of tc-mqprio")
Signed-off-by: Lorenzo Bianconi <redacted>

- Can a failed TAPRIO_CMD_REPLACE now leave sch_taprio and the hardware out
  of sync?  In tc_taprio_configure() the FPE mapping is validated only after
  the gate control list has already been programmed and enabled:
  	ret = stmmac_est_configure(priv, priv, priv->est,
  				   priv->plat->clk_ptp_rate);
  	mutex_unlock(&priv->est_lock);
  	if (ret) {
  	...
  	ret = stmmac_fpe_map_preemption_class(priv, priv->dev, extack,
  					      qopt->mqprio.preemptible_tcs);
  	if (ret)
  		goto disable;
  That "goto disable" was unreachable for every MAC providing the op while
  the macro forced 0, and becomes live now.  The disable block sets
  priv->est->enable = false, reprograms EST, zeroes
  priv->xstats.max_sdu_txq_drop/mtl_est_txq_hlbf/mtl_est_txq_hlbs and clears
  the preemption class register, and the function returns -EINVAL.
  - I think this issue is not introduced by this patch, but it is already there
    and it should be fixed with a dedicated patch.

- Is this ternary reachable in any form other than "return ret;"?  Following
  the call with pclass == 0:
  - dwmac5_fpe_map_preemption_class() takes "if (!pclass) goto
    update_mapping;" and falls through to "return 0;"
  - dwxgmac3_fpe_map_preemption_class() has no error return at all

  - I think the current implementation is more robust for any future change
    even if in the current code if pclass is 0, it can't return an error code.

- Same question for the mqprio delete path: with pclass == 0 the value
  returned by stmmac_reset_tc_mqprio() can only encode "callback op pointer
  missing", after netdev_reset_tc() and netif_set_real_num_tx_queues() have
  already been applied.  Is propagating it useful here, or would keeping
  "return 0;" after the reset match what the hardware state actually is?
  - Same here, I think the current implementation is more robust for any
    future change.

Regards,
Lorenzo

-	}
+	if (!num_tc)
+		return stmmac_reset_tc_mqprio(ndev, extack);
 
 	err = netdev_set_num_tc(ndev, num_tc);
 	if (err)

-- 
2.55.0