Re: [PATCH net-next v3 2/2] net: dsa: yt921x: Add port TBF support

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@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.
Full review at: https://sashiko.dev
---
net: dsa: yt921x: Add port TBF support

This commit adds support for configuring the egress shaper via the
TC_SETUP_QDISC_TBF offload in the yt921x driver.

> diff --git a/drivers/net/dsa/yt921x.c b/drivers/net/dsa/yt921x.c
> index 42dca9617fb17..c5af64ea0c326 100644
> --- a/drivers/net/dsa/yt921x.c
> +++ b/drivers/net/dsa/yt921x.c
[ ... ]
> +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;
> +
> +	if (qopt->parent != TC_H_ROOT)
> +		return -EOPNOTSUPP;
> +
> +	switch (qopt->command) {
> +	case TC_TBF_STATS:
> +		return 0;

Is it intended to return 0 here without populating the hardware statistics?

When the driver handles this command by returning 0 without modifying
qopt->stats, it incorrectly signals to the TC core that hardware statistics
were successfully fetched. As a result, the subsystem will report
unmodified (typically zeroed) statistics, which hides any packets shaped
or dropped by the hardware.

Should this read the hardware statistic registers (like the newly defined
YT921X_PORTn_SHAPE_STAT) to update qopt->stats, or alternatively return
-EOPNOTSUPP to indicate a lack of support?