Re: [PATCH net-next v2 3/3] net: dsa: yt921x: Add port qdisc tbf support

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: yt921x: Add port qdisc tbf support

This commit wires up the port_setup_tc DSA operation to offload a TBF
qdisc onto the yt921x per-port shaper, adding register definitions for
the shaper control block and a marker-transform helper that shares the
existing policer plumbing.

> diff --git a/drivers/net/dsa/yt921x.c b/drivers/net/dsa/yt921x.c
> index 42dca9617fb1..dc38e0dd22ee 100644
> --- a/drivers/net/dsa/yt921x.c
> +++ b/drivers/net/dsa/yt921x.c

[ ... ]

> @@ -1378,6 +1390,112 @@ yt921x_dsa_port_policer_add(struct dsa_switch *ds, int port,
>  	return res;
>  }
>  
> +static int
> +yt921x_tbf_validate(struct yt921x_priv *priv,
> +		    const struct tc_tbf_qopt_offload *qopt)
> +{
> +	struct netlink_ext_ack *extack = qopt->extack;
> +
> +	if (qopt->parent != TC_H_ROOT) {
> +		NL_SET_ERR_MSG_MOD(extack, "Parent should be \"root\"");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	switch (qopt->command) {
> +	case TC_TBF_REPLACE: {
> +		const struct tc_tbf_qopt_offload_replace_params *p;
> +
> +		p = &qopt->replace_params;
> +
> +		if (p->mtu || p->peak.rate_bytes_ps) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Offload not supported when mtu/peakrate is configured");
> +			return -EOPNOTSUPP;
> +		}

Is the p->mtu check overly broad here?  tbf_change() in
net/sched/sch_tbf.c sets q->mtu from the user-supplied qopt->mtu whenever
TCA_TBF_PBURST is absent, so p->mtu can be non-zero even when no peakrate
has been requested.  A user that explicitly passes the mtu parameter to
tc would then be denied offload with an error that attributes the failure
to mtu/peakrate together.

Would gating only on p->peak.rate_bytes_ps better reflect what the
hardware cannot do?

> +
> +		if (!p->rate.mpu) {
> +			NL_SET_ERR_MSG_MOD(extack, "Assuming mpu = 64");
> +		} else if (p->rate.mpu != 64) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Offload not supported when mpu is other than 64");
> +			return -EOPNOTSUPP;
> +		}

Is setting an extack message on the success path intentional?
psched_ratecfg_precompute() leaves mpu at 0 by default, so the common
tc qdisc add ... tbf rate X burst Y invocation will hit the first branch,
return 0, and iproute2 will then print:

    Warning: yt921x: Assuming mpu = 64.

on every TBF replace.

Also, is the assumption actually enforced anywhere?  yt921x_marker_tfm_shape()
and yt921x_marker_tfm() do not appear to compensate the rate computation
for a 64-byte MPU, so the message seems to document an assumption that is
not modelled by the transform.

> +
> +		break;
> +	}
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}

Would it be clearer to have yt921x_tbf_validate() return -EOPNOTSUPP for
commands the handler cannot service, so the supported command set is
stated once?  The default arm here returns 0 for every command other than
TC_TBF_REPLACE (including TC_TBF_STATS and TC_TBF_GRAFT) while
yt921x_dsa_port_setup_tc_tbf_port() below returns -EOPNOTSUPP for those
same commands.  There is no observable bug today, but if the validator is
ever used on its own the two lists can drift.

> +
> +static int
> +yt921x_dsa_port_setup_tc_tbf_port(struct dsa_switch *ds, int port,
> +				  const struct tc_tbf_qopt_offload *qopt)
> +{
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +	struct netlink_ext_ack *extack = qopt->extack;
> +	u32 ctrls[2];
> +	int res;
> +
> +	switch (qopt->command) {
> +	case TC_TBF_DESTROY:
> +		ctrls[0] = 0;
> +		ctrls[1] = 0;
> +		break;

Is there a concern about the write ordering on teardown?
yt921x_reg64_write() -> yt921x_regs_write() performs two independent
register writes (reg+0 then reg+4).  On TC_TBF_DESTROY the first write
sets CIR=0 and CBS=0 while the previously programmed EN bit in ctrls[1]
is still set; only the second write clears EN.  The REPLACE path writes
configuration before arming EN, but the inverse is not followed here.

Would writing ctrls[1]=0 first (or clearing EN via a read-modify-write
before zeroing CIR/CBS) be safer for the hardware state machine?

> +	case TC_TBF_REPLACE: {
[ ... ]
> +		ctrls[0] = YT921X_PORT_SHAPE_CTRLa_CIR(marker.cir) |
> +			   YT921X_PORT_SHAPE_CTRLa_CBS(marker.cbs);
> +		ctrls[1] = YT921X_PORT_SHAPE_CTRLb_UNIT(marker.unit) |
> +			   YT921X_PORT_SHAPE_CTRLb_EN;
> +		break;
> +	}
> +	default:
> +		return -EOPNOTSUPP;
> +	}

Does returning -EOPNOTSUPP for TC_TBF_STATS defeat the hw offload
indicator?  qdisc_offload_dump_helper() in net/sched/sch_api.c first
clears TCQ_F_OFFLOADED, then calls into the driver, and only re-sets the
flag on a zero return:

    sch->flags &= ~TCQ_F_OFFLOADED;
    if (!tc_skip_hw(sch->flags))
        err = qdisc_offload(sch, type, type_data, extack);
    if (err == -EOPNOTSUPP)
        return 0;
    if (err)
        return err;
    sch->flags |= TCQ_F_OFFLOADED;

tbf_dump() invokes this path on every dump, so even after a successful
TC_TBF_REPLACE, tc -s qdisc show would keep reporting offload false and
tc_fill_qdisc() would emit TCA_HW_OFFLOAD=0.

Would a stub TC_TBF_STATS case that returns 0 (leaving bstats/qstats
untouched) preserve the flag here?  mlxsw does this in its
spectrum_qdisc.c.

[ ... ]