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

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

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

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 <lorenzo.bianconi@oss.qualcomm.com>

> ---
>  drivers/net/ethernet/stmicro/stmmac/hwif.h      |  2 +-
>  drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c | 19 +++++++++----------
>  2 files changed, 10 insertions(+), 11 deletions(-)
> 
> 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)
>  
>  /* PTP and HW Timer helpers */
>  struct stmmac_hwtimestamp {
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 14cabe76e53e..5398616fcdfe 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -970,7 +970,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>  	struct netlink_ext_ack *extack = qopt->mqprio.extack;
>  	struct timespec64 time, current_time, qopt_time;
>  	ktime_t current_time_ns;
> -	int i, ret = 0;
> +	int err, i, ret = 0;
>  	u64 ctr;
>  
>  	if (qopt->base_time < 0)
> @@ -1120,9 +1120,9 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>  		mutex_unlock(&priv->est_lock);
>  	}
>  
> -	stmmac_fpe_map_preemption_class(priv, priv->dev, extack, 0);
> +	err = stmmac_fpe_map_preemption_class(priv, priv->dev, extack, 0);

- 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.

>  
> -	return ret;
> +	return qopt->cmd == TAPRIO_CMD_DESTROY ? err : ret;
>  }
>  
>  static void tc_taprio_stats(struct stmmac_priv *priv,
> @@ -1237,14 +1237,15 @@ static int tc_query_caps(struct stmmac_priv *priv,
>  	}
>  }
>  
> -static void stmmac_reset_tc_mqprio(struct net_device *ndev,
> -				   struct netlink_ext_ack *extack)
> +static int stmmac_reset_tc_mqprio(struct net_device *ndev,
> +				  struct netlink_ext_ack *extack)
>  {
>  	struct stmmac_priv *priv = netdev_priv(ndev);
>  
>  	netdev_reset_tc(ndev);
>  	netif_set_real_num_tx_queues(ndev, priv->plat->tx_queues_to_use);
> -	stmmac_fpe_map_preemption_class(priv, ndev, extack, 0);
> +
> +	return stmmac_fpe_map_preemption_class(priv, ndev, extack, 0);
>  }
>  
>  static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
> @@ -1257,10 +1258,8 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
>  	u32 num_tc = qopt->num_tc;
>  	int err;
>  
> -	if (!num_tc) {
> -		stmmac_reset_tc_mqprio(ndev, extack);
> -		return 0;

- 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
> 

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