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@xxxxxxxxxx> 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@xxxxxxxxxx/
...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().