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

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

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

[ ... ]

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

[David Yang]

On Thu, May 7, 2026 at 9:23 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > +                     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?

I didn't see it can be non-zero while testing with tc-tbf(8).

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

IIRC I have checked that with tc-tbf(8) (or I used the wrong series);
will check that later.

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

https://lore.kernel.org/all/20260413153248.0588690b@kernel.org/
...BTW we can use extack for "warnings" like this, too

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

tc-tbf(8): "For ethernet, no packet uses less than 64 bytes." I don't
see a need for rate computation unless I get it wrong.

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

The AI got confused on register operations; see comment for multi-word
registers.

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

No one else did that; mt753x_tc_setup_qdisc_tbf().