Re: [PATCH net-next] net: stmmac: add tc-ets qdisc offload

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

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

> On Tue, Aug 04, 2026 at 08:17:35PM +0200, Lorenzo Bianconi wrote:
> 
> [...]
> 
> > When all bands are strict priority, the MTL scheduler is programmed with
> > SP scheduling; 
> 
> ... and I replied too early _ sorry for the noise _ there is another small thing

thx for the review :)

> w.r.t. SP scheduling:
> 
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > @@ -1210,6 +1210,73 @@ static int tc_setup_etf(struct stmmac_priv *priv,
> >  	return 0;
> >  }
> >  
> > +static int tc_setup_sp_sched(struct stmmac_priv *priv)
> > +{
> > +	struct plat_stmmacenet_data *pdata = priv->plat;
> > +	int i;
> > +
> > +	for (i = 0; i < pdata->tx_queues_to_use; i++)
> > +		pdata->tx_queues_cfg[i].weight = 0;
> > +	stmmac_set_tx_queue_weight(priv);
> > +
> > +	pdata->tx_sched_algorithm = MTL_TX_ALGORITHM_SP;
> > +	stmmac_prog_mtl_tx_algorithms(priv, priv->hw,
> > +				      pdata->tx_sched_algorithm);
> > +
> > +	return 0;
> > +}
> 
> The above function configures the stmmac SP queues to a well-known state where all
> queues are SP. However,
> 
> [...]
> 
> > +
> > +static int tc_setup_ets_sched(struct stmmac_priv *priv,
> > +			      struct tc_ets_qopt_offload *qopt)
> > +{
> > +	struct tc_ets_qopt_offload_replace_params *p = &qopt->replace_params;
> > +	struct plat_stmmacenet_data *pdata = priv->plat;
> > +	int i, nstrict = 0;
> > +
> > +	if (p->bands > priv->plat->tx_queues_to_use)
> > +		return -EOPNOTSUPP;
> > +
> > +	for (i = 0; i < p->bands; i++) {
> > +		if (!p->quanta[i])
> > +			nstrict++;
> > +	}
> > +
> > +	if (p->bands && nstrict)
> > +		return -EOPNOTSUPP;
> > +
> > +	if (nstrict) {
> > +		tc_setup_sp_sched(priv);
> 
> This is not necessarily what the user wants when setting up the ETS qdisc.
> The driver should parse 'priomap' (like in [1] or [2]) so that only 'nstrict'
> queues are involved, and selected based on the packet priority. Otherwise, the
> offloaded ETS is going to behave very differently from its full-software
> implementation _ which is something we probably want to avoid.
> 
> [1] https://elixir.bootlin.com/linux/v7.1.5/source/drivers/net/ethernet/microchip/lan966x/lan966x_ets.c#L36
> [2] https://elixir.bootlin.com/linux/v7.1.5/source/drivers/net/ethernet/airoha/airoha_eth.c#L2281
> 	
> WDYT? Any feedback appreciated!

ack, I agree. Since the hw assigns increasing priority to the sp queues (prio_n+1 > prio_n),
we should check the priomap according to the following code:

	if (nstrict) {
		if (nstrict != p->bands)
			return -EINVAL;

		for (i = 0; i < p->bands; i++) {
			if (p->priomap[p->bands - i - 1] != i)
				return -EINVAL;
		}
		...
	}

Agree?

Regard,
Lorenzo

> 
> -- 
> davide
> 

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