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

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

[David Yang]

v1: https://lore.kernel.org/r/20260502215314.917687-1-mmyangfl@gmail.com
  - remove queue related register definiations
  - add missing extack param during tbf setup
v0: https://lore.kernel.org/r/20260409171209.2575583-1-mmyangfl@gmail.com
  - picked from old series
  - add extack to the offload struct
  - add all params to the offload struct

David Yang (3):
  net: sched: tbf: add extack to offload params
  net: sched: tbf: pass all params to offload users
  net: dsa: yt921x: Add port qdisc tbf support

 drivers/net/dsa/yt921x.c | 126 +++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/yt921x.h |  18 ++++++
 include/net/pkt_cls.h    |   7 ++-
 net/sched/sch_tbf.c      |  15 ++++-
 4 files changed, 162 insertions(+), 4 deletions(-)

-- 
2.53.0

[PATCH net-next v2 1/3] net: sched: tbf: add extack to offload params

[David Yang]

Drivers might have error messages to propagate to user space. Propagate
the netlink extack so that they can inform user space in a verbal way of
their limitations.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 include/net/pkt_cls.h | 1 +
 net/sched/sch_tbf.c   | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 99ac747b7906..3bd08d7f39c1 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -1046,6 +1046,7 @@ struct tc_tbf_qopt_offload_replace_params {
 };
 
 struct tc_tbf_qopt_offload {
+	struct netlink_ext_ack *extack;
 	enum tc_tbf_command command;
 	u32 handle;
 	u32 parent;
diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
index f2340164f579..4576111fe075 100644
--- a/net/sched/sch_tbf.c
+++ b/net/sched/sch_tbf.c
@@ -139,7 +139,8 @@ static u64 psched_ns_t2l(const struct psched_ratecfg *r,
 	return len;
 }
 
-static void tbf_offload_change(struct Qdisc *sch)
+static void tbf_offload_change(struct Qdisc *sch,
+			       struct netlink_ext_ack *extack)
 {
 	struct tbf_sched_data *q = qdisc_priv(sch);
 	struct net_device *dev = qdisc_dev(sch);
@@ -148,6 +149,7 @@ static void tbf_offload_change(struct Qdisc *sch)
 	if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
 		return;
 
+	qopt.extack = extack;
 	qopt.command = TC_TBF_REPLACE;
 	qopt.handle = sch->handle;
 	qopt.parent = sch->parent;
@@ -166,6 +168,7 @@ static void tbf_offload_destroy(struct Qdisc *sch)
 	if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
 		return;
 
+	qopt.extack = NULL;
 	qopt.command = TC_TBF_DESTROY;
 	qopt.handle = sch->handle;
 	qopt.parent = sch->parent;
@@ -176,6 +179,7 @@ static int tbf_offload_dump(struct Qdisc *sch)
 {
 	struct tc_tbf_qopt_offload qopt;
 
+	qopt.extack = NULL;
 	qopt.command = TC_TBF_STATS;
 	qopt.handle = sch->handle;
 	qopt.parent = sch->parent;
@@ -193,6 +197,7 @@ static void tbf_offload_graft(struct Qdisc *sch, struct Qdisc *new,
 		.parent		= sch->parent,
 		.child_handle	= new->handle,
 		.command	= TC_TBF_GRAFT,
+		.extack		= extack,
 	};
 
 	qdisc_offload_graft_helper(qdisc_dev(sch), sch, new, old,
@@ -477,7 +482,7 @@ static int tbf_change(struct Qdisc *sch, struct nlattr *opt,
 	qdisc_put(old);
 	err = 0;
 
-	tbf_offload_change(sch);
+	tbf_offload_change(sch, extack);
 done:
 	return err;
 }
-- 
2.53.0

[PATCH net-next v2 2/3] net: sched: tbf: pass all params to offload users

[David Yang]

Drivers might have fine-grained control over queues. Pass all available
params so they can make use of them.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 include/net/pkt_cls.h | 6 +++++-
 net/sched/sch_tbf.c   | 6 +++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 3bd08d7f39c1..d73c04e3d919 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -1040,8 +1040,12 @@ enum tc_tbf_command {
 };
 
 struct tc_tbf_qopt_offload_replace_params {
+	u32		limit;
+	u32		max_size;
+	s64		buffer;
+	s64		mtu;
 	struct psched_ratecfg rate;
-	u32 max_size;
+	struct psched_ratecfg peak;
 	struct gnet_stats_queue *qstats;
 };
 
diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
index 4576111fe075..8a1110d59dec 100644
--- a/net/sched/sch_tbf.c
+++ b/net/sched/sch_tbf.c
@@ -153,8 +153,12 @@ static void tbf_offload_change(struct Qdisc *sch,
 	qopt.command = TC_TBF_REPLACE;
 	qopt.handle = sch->handle;
 	qopt.parent = sch->parent;
-	qopt.replace_params.rate = q->rate;
+	qopt.replace_params.limit = q->limit;
 	qopt.replace_params.max_size = q->max_size;
+	qopt.replace_params.buffer = q->buffer;
+	qopt.replace_params.mtu = q->mtu;
+	qopt.replace_params.rate = q->rate;
+	qopt.replace_params.peak = q->peak;
 	qopt.replace_params.qstats = &sch->qstats;
 
 	dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TBF, &qopt);
-- 
2.53.0

Re: [PATCH net-next v2 2/3] net: sched: tbf: pass all params to offload users

[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: sched: tbf: pass all params to offload users

This commit exposes additional TBF parameters (limit, buffer, mtu, and peak
rate) to hardware offload drivers so they have finer-grained control over
queue configuration.

> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 3bd08d7f39c1..d73c04e3d919 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -1040,8 +1040,12 @@ enum tc_tbf_command {
>  };
>  
>  struct tc_tbf_qopt_offload_replace_params {
> +	u32		limit;
> +	u32		max_size;
> +	s64		buffer;
> +	s64		mtu;

Are buffer and mtu the most clear variable names for the offload API? 

In sch_tbf.c, these fields store token bucket depths converted to nanoseconds,
rather than raw configuration bytes. Exposing time values under names that
typically imply byte sizes could mislead driver authors.

>  	struct psched_ratecfg rate;
> -	u32 max_size;
> +	struct psched_ratecfg peak;
>  	struct gnet_stats_queue *qstats;
>  };

[ ... ]

> diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
> index 4576111fe075..8a1110d59dec 100644
> --- a/net/sched/sch_tbf.c
> +++ b/net/sched/sch_tbf.c
> @@ -153,8 +153,12 @@ static void tbf_offload_change(struct Qdisc *sch,
>  	qopt.command = TC_TBF_REPLACE;
>  	qopt.handle = sch->handle;
>  	qopt.parent = sch->parent;
> -	qopt.replace_params.rate = q->rate;
> +	qopt.replace_params.limit = q->limit;
>  	qopt.replace_params.max_size = q->max_size;

Does passing q->max_size here provide the driver with the original burst size?

The software TBF overwrites q->max_size with min(burst, pburst) when a peak
rate is configured. As a result, the offload API only passes this overwritten
value, meaning a driver attempting to offload a dual-rate TBF is not given
the original burst size in bytes and would have to reverse-engineer it from
the nanosecond buffer value.

> +	qopt.replace_params.buffer = q->buffer;
> +	qopt.replace_params.mtu = q->mtu;

Will this cause the yt921x offload (introduced in the next patch) to
unconditionally reject all offloads?

The yt921x driver attempts to reject dual-rate offloads by checking if p->mtu
or p->peak.rate_bytes_ps are set. However, the tc userspace tool always
populates the mtu parameter with a default value (like 2047 bytes) for legacy
rate table compatibility.

Since tc always sets mtu, q->mtu will be non-zero, causing the yt921x driver
to return -EOPNOTSUPP and reject the configuration. 

Should the offload API explicitly pass the raw burst and pburst parameters in
bytes instead?

> +	qopt.replace_params.rate = q->rate;
> +	qopt.replace_params.peak = q->peak;
>  	qopt.replace_params.qstats = &sch->qstats;
>  
>  	dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TBF, &qopt);

Re: [PATCH net-next v2 2/3] net: sched: tbf: pass all params to offload users

[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: sched: tbf: pass all params to offload users

This change extends struct tc_tbf_qopt_offload_replace_params with
limit, max_size, buffer, mtu and a peak ratecfg, and populates them
from tbf_sched_data in tbf_offload_change() so offload drivers can see
all the tbf parameters.

> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 3bd08d7f39c1..d73c04e3d919 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -1040,8 +1040,12 @@ enum tc_tbf_command {
>  };
>  
>  struct tc_tbf_qopt_offload_replace_params {
> +	u32		limit;
> +	u32		max_size;
> +	s64		buffer;
> +	s64		mtu;

The buffer and mtu fields are stored in tbf_sched_data in nanoseconds
(see tbf_change() in net/sched/sch_tbf.c where they are derived via
PSCHED_TICKS2NS(qopt->buffer) and psched_l2t_ns()), but they are
exposed here as bare s64 buffer / s64 mtu right next to max_size
which is a byte count.

Would it be worth renaming these to buffer_ns / mtu_ns, or adding
kerneldoc to describe their unit?

A driver author reading this struct and seeing mtu adjacent to
max_size might reasonably assume mtu is a byte MTU and program
hardware accordingly.

>  	struct psched_ratecfg rate;
> -	u32 max_size;
> +	struct psched_ratecfg peak;

tbf_change() does memset(&peak, 0, sizeof(peak)) when no peakrate is
configured, and the qdisc internally uses tbf_peak_present(q) which
checks q->peak.rate_bytes_ps != 0 to distinguish absent from present.

With peak exposed to offload drivers, every driver now has to
reproduce this convention implicitly (e.g. by checking
p->peak.rate_bytes_ps).

Would an explicit indicator such as a bool peak_present field, or at
least kerneldoc spelling out the rate_bytes_ps == 0 means absent
contract, make this safer for future offload consumers?

>  	struct gnet_stats_queue *qstats;
>  };
>  

[ ... ]
-- 
pw-bot: cr

Re: [PATCH net-next v2 2/3] net: sched: tbf: pass all params to offload users

[David Yang]

On Thu, May 7, 2026 at 9:23 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >       qopt.command = TC_TBF_REPLACE;
> >       qopt.handle = sch->handle;
> >       qopt.parent = sch->parent;
> > -     qopt.replace_params.rate = q->rate;
> > +     qopt.replace_params.limit = q->limit;
> >       qopt.replace_params.max_size = q->max_size;
>
> Does passing q->max_size here provide the driver with the original burst size?
>
> The software TBF overwrites q->max_size with min(burst, pburst) when a peak
> rate is configured. As a result, the offload API only passes this overwritten
> value, meaning a driver attempting to offload a dual-rate TBF is not given
> the original burst size in bytes and would have to reverse-engineer it from
> the nanosecond buffer value.

I'm not an expert on TBF so I don't know if it's fine to expose the
internal of schedulers to the drivers, also this would require changes
on struct tbf_sched_data.

> >  struct tc_tbf_qopt_offload_replace_params {
> > +     u32             limit;
> > +     u32             max_size;
> > +     s64             buffer;
> > +     s64             mtu;
>
> The buffer and mtu fields are stored in tbf_sched_data in nanoseconds
> (see tbf_change() in net/sched/sch_tbf.c where they are derived via
> PSCHED_TICKS2NS(qopt->buffer) and psched_l2t_ns()), but they are
> exposed here as bare s64 buffer / s64 mtu right next to max_size
> which is a byte count.
>
> Would it be worth renaming these to buffer_ns / mtu_ns, or adding
> kerneldoc to describe their unit?
>
> A driver author reading this struct and seeing mtu adjacent to
> max_size might reasonably assume mtu is a byte MTU and program
> hardware accordingly.

These are carbon copies of struct tbf_sched_data, I see no reason to
rename just here.

> >       struct psched_ratecfg rate;
> > -     u32 max_size;
> > +     struct psched_ratecfg peak;
>
> tbf_change() does memset(&peak, 0, sizeof(peak)) when no peakrate is
> configured, and the qdisc internally uses tbf_peak_present(q) which
> checks q->peak.rate_bytes_ps != 0 to distinguish absent from present.
>
> With peak exposed to offload drivers, every driver now has to
> reproduce this convention implicitly (e.g. by checking
> p->peak.rate_bytes_ps).
>
> Would an explicit indicator such as a bool peak_present field, or at
> least kerneldoc spelling out the rate_bytes_ps == 0 means absent
> contract, make this safer for future offload consumers?

No similar logic is found for struct flow_action_police.

Re: [PATCH net-next v2 2/3] net: sched: tbf: pass all params to offload users

[Jakub Kicinski]

On Thu, 7 May 2026 11:11:58 +0800 David Yang wrote:
> > >  struct tc_tbf_qopt_offload_replace_params {
> > > +     u32             limit;
> > > +     u32             max_size;
> > > +     s64             buffer;
> > > +     s64             mtu;  
> >
> > The buffer and mtu fields are stored in tbf_sched_data in nanoseconds
> > (see tbf_change() in net/sched/sch_tbf.c where they are derived via
> > PSCHED_TICKS2NS(qopt->buffer) and psched_l2t_ns()), but they are
> > exposed here as bare s64 buffer / s64 mtu right next to max_size
> > which is a byte count.
> >
> > Would it be worth renaming these to buffer_ns / mtu_ns, or adding
> > kerneldoc to describe their unit?
> >
> > A driver author reading this struct and seeing mtu adjacent to
> > max_size might reasonably assume mtu is a byte MTU and program
> > hardware accordingly.  
> 
> These are carbon copies of struct tbf_sched_data, I see no reason to
> rename just here.

Driver API has broader exposure and more potential for
misunderstandings. AI's naming suggestion makes sense to me.

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

[David Yang]

Enable port shaping and support limiting the rate of outgoing traffic.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/yt921x.c | 126 +++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/yt921x.h |  18 ++++++
 2 files changed, 144 insertions(+)

diff --git a/drivers/net/dsa/yt921x.c b/drivers/net/dsa/yt921x.c
index fd1fdcd5f9a3..fd77e1ef53bb 100644
--- a/drivers/net/dsa/yt921x.c
+++ b/drivers/net/dsa/yt921x.c
@@ -24,6 +24,7 @@
 #include <net/dsa.h>
 #include <net/dscp.h>
 #include <net/ieee8021q.h>
+#include <net/pkt_cls.h>
 
 #include "yt921x.h"
 
@@ -1272,6 +1273,17 @@ yt921x_marker_tfm_police(struct yt921x_marker *marker,
 				 priv, port, extack);
 }
 
+static int
+yt921x_marker_tfm_shape(struct yt921x_marker *marker, u64 rate, u64 burst,
+			unsigned int flags, struct yt921x_priv *priv, int port,
+			struct netlink_ext_ack *extack)
+{
+	return yt921x_marker_tfm(marker, rate, burst, flags,
+				 priv->port_shape_slot_ns, YT921X_SHAPE_CIR_MAX,
+				 YT921X_SHAPE_CBS_MAX, YT921X_SHAPE_UNIT_MAX,
+				 priv, port, extack);
+}
+
 static int
 yt921x_police_validate(const struct flow_action_police *police,
 		       const struct flow_action *action,
@@ -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;
+		}
+
+		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;
+		}
+
+		break;
+	}
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+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;
+	case TC_TBF_REPLACE: {
+		const struct tc_tbf_qopt_offload_replace_params *p;
+		struct yt921x_marker marker;
+
+		p = &qopt->replace_params;
+
+		res = yt921x_marker_tfm_shape(&marker, p->rate.rate_bytes_ps,
+					      p->max_size,
+					      YT921X_MARKER_SINGLE_BUCKET,
+					      priv, port, extack);
+		if (res)
+			return res;
+
+		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;
+	}
+
+	mutex_lock(&priv->reg_lock);
+	res = yt921x_reg64_write(priv, YT921X_PORTn_SHAPE_CTRL(port), ctrls);
+	mutex_unlock(&priv->reg_lock);
+
+	return res;
+}
+
+static int
+yt921x_dsa_port_setup_tc(struct dsa_switch *ds, int port,
+			 enum tc_setup_type type, void *type_data)
+{
+	struct yt921x_priv *priv = to_yt921x_priv(ds);
+	int res;
+
+	switch (type) {
+	case TC_SETUP_QDISC_TBF: {
+		const struct tc_tbf_qopt_offload *qopt = type_data;
+
+		res = yt921x_tbf_validate(priv, qopt);
+		if (res)
+			return res;
+
+		return yt921x_dsa_port_setup_tc_tbf_port(ds, port, qopt);
+	}
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static int
 yt921x_mirror_del(struct yt921x_priv *priv, int port, bool ingress)
 {
@@ -3524,6 +3642,13 @@ static int yt921x_chip_setup_tc(struct yt921x_priv *priv)
 		return res;
 	priv->meter_slot_ns = ctrl * op_ns;
 
+	ctrl = max(priv->port_shape_slot_ns / op_ns,
+		   YT921X_PORT_SHAPE_SLOT_MIN);
+	res = yt921x_reg_write(priv, YT921X_PORT_SHAPE_SLOT, ctrl);
+	if (res)
+		return res;
+	priv->port_shape_slot_ns = ctrl * op_ns;
+
 	return 0;
 }
 
@@ -3680,6 +3805,7 @@ static const struct dsa_switch_ops yt921x_dsa_switch_ops = {
 	/* rate */
 	.port_policer_del	= yt921x_dsa_port_policer_del,
 	.port_policer_add	= yt921x_dsa_port_policer_add,
+	.port_setup_tc		= yt921x_dsa_port_setup_tc,
 	/* hsr */
 	.port_hsr_leave		= dsa_port_simple_hsr_leave,
 	.port_hsr_join		= dsa_port_simple_hsr_join,
diff --git a/drivers/net/dsa/yt921x.h b/drivers/net/dsa/yt921x.h
index 546b12a8994a..70fa780c337f 100644
--- a/drivers/net/dsa/yt921x.h
+++ b/drivers/net/dsa/yt921x.h
@@ -531,6 +531,19 @@ enum yt921x_app_selector {
 #define  YT921X_MIRROR_PORT_M			GENMASK(3, 0)
 #define   YT921X_MIRROR_PORT(x)				FIELD_PREP(YT921X_MIRROR_PORT_M, (x))
 
+#define YT921X_PORT_SHAPE_SLOT		0x34000c
+#define  YT921X_PORT_SHAPE_SLOT_SLOT_M		GENMASK(11, 0)
+#define YT921X_PORTn_SHAPE_CTRL(port)	(0x354000 + 8 * (port))
+#define  YT921X_PORT_SHAPE_CTRLb_EN		BIT(4)
+#define  YT921X_PORT_SHAPE_CTRLb_PKT_MODE	BIT(3)	/* 0: byte rate mode */
+#define  YT921X_PORT_SHAPE_CTRLb_UNIT_M		GENMASK(2, 0)
+#define   YT921X_PORT_SHAPE_CTRLb_UNIT(x)		FIELD_PREP(YT921X_PORT_SHAPE_CTRLb_UNIT_M, (x))
+#define  YT921X_PORT_SHAPE_CTRLa_CBS_M		GENMASK(31, 18)
+#define   YT921X_PORT_SHAPE_CTRLa_CBS(x)		FIELD_PREP(YT921X_PORT_SHAPE_CTRLa_CBS_M, (x))
+#define  YT921X_PORT_SHAPE_CTRLa_CIR_M		GENMASK(17, 0)
+#define   YT921X_PORT_SHAPE_CTRLa_CIR(x)		FIELD_PREP(YT921X_PORT_SHAPE_CTRLa_CIR_M, (x))
+#define YT921X_PORTn_SHAPE_STAT(port)	(0x356000 + 4 * (port))
+
 #define YT921X_EDATA_EXTMODE	0xfb
 #define YT921X_EDATA_LEN	0x100
 
@@ -556,6 +569,10 @@ enum yt921x_fdb_entry_status {
 #define YT921X_METER_UNIT_MAX	((1 << 3) - 1)
 #define YT921X_METER_CIR_MAX	((1 << 18) - 1)
 #define YT921X_METER_CBS_MAX	((1 << 16) - 1)
+#define YT921X_PORT_SHAPE_SLOT_MIN	80
+#define YT921X_SHAPE_UNIT_MAX	((1 << 3) - 1)
+#define YT921X_SHAPE_CIR_MAX	((1 << 18) - 1)
+#define YT921X_SHAPE_CBS_MAX	((1 << 14) - 1)
 
 #define YT921X_LAG_NUM		2
 #define YT921X_LAG_PORT_NUM	4
@@ -652,6 +669,7 @@ struct yt921x_priv {
 
 	const struct yt921x_info *info;
 	unsigned int meter_slot_ns;
+	unsigned int port_shape_slot_ns;
 	/* cache of dsa_cpu_ports(ds) */
 	u16 cpu_ports_mask;
 	unsigned char cycle_ns;
-- 
2.53.0

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