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

[Jakub Kicinski]

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 patch enables port shaping and adds support for limiting the rate of
outgoing traffic using the TBF qdisc offload.

> 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)
> +{
[ ... ]
> +	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) {

The userspace tc utility unconditionally sets a default MTU (typically 2047)
when adding a TBF qdisc, which is passed to the driver via p->mtu. Since
p->mtu is never zero, will this unconditionally reject all offload requests?

> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Offload not supported when mtu/peakrate is configured");
> +			return -EOPNOTSUPP;
> +		}
> +
> +		if (!p->rate.mpu) {

Does this code miss validation for p->rate.overhead?

The TC TBF layer allows configuring overhead to account for additional
headers. If the hardware relies on fixed wire frame sizes and the driver
ignores p->rate.overhead, could this lead to a silent misconfiguration
where the hardware shapes traffic at a different effective rate than
requested?

> +			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;
> +		}