[PATCH net-next v2 0/4] net: macb: Add TSN MQPRIO and CBS traffic-class offload

[PATCH net-next v2 0/4] net: macb: Add TSN MQPRIO and CBS traffic-class offload

[Vineeth Karumanchi]

This series adds TSN traffic-class hardware offload support to the
Cadence macb/GEM driver, building on the existing TAPRIO/Qbv
infrastructure.

GEM IP versions that implement Qbv (e.g. Versal GEM) also support
additional TSN clauses. The series first generalises the existing Qbv
capability flag into a traffic-class offload flag and centralises the
common ndo_setup_tc() preconditions, then adds two new offloads:

 - MQPRIO: traffic-class to hardware-queue mapping, delegating queue
   count and overlap validation to the mqprio core.

 - CBS (Credit-Based Shaper, IEEE 802.1Qav): per-queue idleslope
   programming on the two highest-priority queues, with the idleslope
   register value derived from the current link speed (full 32-bit
   range for high-speed GEM, port-rate factor for standard GEM).

Patches 1 and 2 are preparatory refactors with no functional change.
Patches 3 and 4 add the MQPRIO and CBS offloads respectively.

Changes in v2:
- CBS: macb_cbs_get_queue_params() now returns the idleslope register
  offset instead of a bool flag, and the idleslope is programmed via
  bp->macb_reg_writel(), dropping the per-queue if/else (patch 4).
- CBS: zero-initialize kset and expand the idleslope conversion
  comment (patch 4).
- Rebased on net-next, which renamed the struct net_device pointer to
  "netdev" (was "dev"/"ndev"); no functional change in patches 1-3.

v1: https://lore.kernel.org/netdev/20260807095012.640223-1-vineeth.karumanchi@amd.com/

Vineeth Karumanchi (4):
  net: macb: Rename MACB_CAPS_QBV to MACB_CAPS_TC
  net: macb: Move TC capability and PM checks to macb_setup_tc()
  net: macb: Add MQPRIO qdisc hardware offload support
  net: macb: Add TSN CBS TC offload support

 drivers/net/ethernet/cadence/macb.h      |  11 +-
 drivers/net/ethernet/cadence/macb_main.c | 204 +++++++++++++++++++++--
 2 files changed, 201 insertions(+), 14 deletions(-)


base-commit: 548b86839f7fb819a4d6c83b71c73ec378d24275
-- 
2.43.0

[PATCH net-next v2 1/4] net: macb: Rename MACB_CAPS_QBV to MACB_CAPS_TC

[Vineeth Karumanchi]

The MACB_CAPS_QBV capability flag was originally introduced to
gate TAPRIO/QBV support. However, GEM IP versions that support
QBV also implement multiple TSN clauses.

Replace this with a generic capability flag that can be reused
by other TSN features. Rename MACB_CAPS_QBV to MACB_CAPS_TC to
better reflect its role as a general traffic-class offload capability.

The supported TSN clauses are handled through macb_setup_tc().

Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
Changes in v2:
- No functional change; rebased on net-next.

 drivers/net/ethernet/cadence/macb.h      | 2 +-
 drivers/net/ethernet/cadence/macb_main.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index d6931c41f39d..f5359549f3d4 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -768,7 +768,7 @@
 #define MACB_CAPS_MIIONRGMII			BIT(9)
 #define MACB_CAPS_NEED_TSUCLK			BIT(10)
 #define MACB_CAPS_QUEUE_DISABLE			BIT(11)
-#define MACB_CAPS_QBV				BIT(12)
+#define MACB_CAPS_TC				BIT(12)
 #define MACB_CAPS_PCS				BIT(13)
 #define MACB_CAPS_HIGH_SPEED			BIT(14)
 #define MACB_CAPS_CLK_HW_CHG			BIT(15)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index a427a5b9d894..8223ad81215b 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4823,8 +4823,8 @@ static int macb_init_dflt(struct platform_device *pdev)
 		netdev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
 	if (bp->caps & MACB_CAPS_SG_DISABLED)
 		netdev->hw_features &= ~NETIF_F_SG;
-	/* Enable HW_TC if hardware supports QBV */
-	if (bp->caps & MACB_CAPS_QBV)
+	/* Enable TC offload for TSN-capable hardware */
+	if (bp->caps & MACB_CAPS_TC)
 		netdev->hw_features |= NETIF_F_HW_TC;
 
 	netdev->features = netdev->hw_features;
@@ -5693,7 +5693,7 @@ static const struct macb_config versal_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
 		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
 		MACB_CAPS_NEED_TSUCLK | MACB_CAPS_QUEUE_DISABLE |
-		MACB_CAPS_QBV |
+		MACB_CAPS_TC |
 		MACB_CAPS_USRIO_HAS_MII,
 	.dma_burst_length = 16,
 	.init = init_reset_optional,
-- 
2.43.0

[PATCH net-next v2 2/4] net: macb: Move TC capability and PM checks to macb_setup_tc()

[Vineeth Karumanchi]

Move the NETIF_F_HW_TC capability check and the runtime PM suspend
guard from macb_setup_taprio() into the common macb_setup_tc()
entry point.

These preconditions apply to all hardware TC offload paths routed
through ndo_setup_tc(), not just TAPRIO. Centralizing them avoids
duplicating the same checks as new TC qdisc types are added.

Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
Changes in v2:
- No functional change; rebased on net-next, which renamed the
  struct net_device pointer to "netdev" (was "dev").

 drivers/net/ethernet/cadence/macb_main.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 8223ad81215b..ff7e02d3fab8 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4476,18 +4476,8 @@ static void macb_taprio_destroy(struct net_device *netdev)
 static int macb_setup_taprio(struct net_device *netdev,
 			     struct tc_taprio_qopt_offload *taprio)
 {
-	struct macb *bp = netdev_priv(netdev);
 	int err = 0;
 
-	if (unlikely(!(netdev->hw_features & NETIF_F_HW_TC)))
-		return -EOPNOTSUPP;
-
-	/* Check if Device is in runtime suspend */
-	if (unlikely(pm_runtime_suspended(&bp->pdev->dev))) {
-		netdev_err(netdev, "Device is in runtime suspend\n");
-		return -EOPNOTSUPP;
-	}
-
 	switch (taprio->cmd) {
 	case TAPRIO_CMD_REPLACE:
 		err = macb_taprio_setup_replace(netdev, taprio);
@@ -4505,9 +4495,22 @@ static int macb_setup_taprio(struct net_device *netdev,
 static int macb_setup_tc(struct net_device *netdev, enum tc_setup_type type,
 			 void *type_data)
 {
+	struct macb *bp;
+
 	if (!netdev || !type_data)
 		return -EINVAL;
 
+	bp = netdev_priv(netdev);
+
+	if (unlikely(!(netdev->hw_features & NETIF_F_HW_TC)))
+		return -EOPNOTSUPP;
+
+	/* Check if Device is in runtime suspend */
+	if (unlikely(pm_runtime_suspended(&bp->pdev->dev))) {
+		netdev_err(netdev, "Device is in runtime suspend\n");
+		return -EOPNOTSUPP;
+	}
+
 	switch (type) {
 	case TC_SETUP_QDISC_TAPRIO:
 		return macb_setup_taprio(netdev, type_data);
-- 
2.43.0

Re: [PATCH net-next v2 2/4] net: macb: Move TC capability and PM checks to macb_setup_tc()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 1 · Low: 0 (1)

- [High] macb_setup_tc() only samples the runtime-PM state with
  pm_runtime_suspended() (macb_main.c:4509) and never acquires a…
- [Medium] macb_setup_tc()
  (drivers/net/ethernet/cadence/macb_main.c:4494-4519) now runs the…

Pre-existing issues:
- [Low] The relocated gate in macb_setup_tc() tests the immutable
  capability mask (`netdev->hw_features & NETIF_F_HW_TC`,…

--

net: macb: Move TC capability and PM checks to macb_setup_tc()

This moves the NETIF_F_HW_TC capability test and the runtime PM suspend
guard out of macb_setup_taprio() and into the shared macb_setup_tc()
entry point so future TC qdisc types do not have to repeat them.

A few questions about the relocated checks below.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 8223ad81215b..ff7e02d3fab8 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4476,18 +4476,8 @@ static void macb_taprio_destroy(struct net_device *netdev)
>  static int macb_setup_taprio(struct net_device *netdev,
>  			     struct tc_taprio_qopt_offload *taprio)
>  {
> -	struct macb *bp = netdev_priv(netdev);
>  	int err = 0;
>  
> -	if (unlikely(!(netdev->hw_features & NETIF_F_HW_TC)))
> -		return -EOPNOTSUPP;
> -
> -	/* Check if Device is in runtime suspend */
> -	if (unlikely(pm_runtime_suspended(&bp->pdev->dev))) {
> -		netdev_err(netdev, "Device is in runtime suspend\n");
> -		return -EOPNOTSUPP;
> -	}
> -
>  	switch (taprio->cmd) {
>  	case TAPRIO_CMD_REPLACE:
>  		err = macb_taprio_setup_replace(netdev, taprio);

[ ... ]

> @@ -4505,9 +4495,22 @@ static int macb_setup_taprio(struct net_device *netdev,
>  static int macb_setup_tc(struct net_device *netdev, enum tc_setup_type type,
>  			 void *type_data)
>  {
> +	struct macb *bp;
> +
>  	if (!netdev || !type_data)
>  		return -EINVAL;
>  
> +	bp = netdev_priv(netdev);
> +
> +	if (unlikely(!(netdev->hw_features & NETIF_F_HW_TC)))
> +		return -EOPNOTSUPP;

[Severity: Low]
This isn't a bug introduced by this patch, since the test is only being
relocated here, but should this gate look at netdev->features rather than
netdev->hw_features?

macb_probe() sets:

	if (bp->caps & MACB_CAPS_TC)
		netdev->hw_features |= NETIF_F_HW_TC;

	netdev->features = netdev->hw_features;

hw_features is never touched again, so this condition reduces to a
MACB_CAPS_TC capability test. macb_set_features() only reacts to
NETIF_F_HW_CSUM, NETIF_F_RXCSUM and NETIF_F_NTUPLE and returns 0 for a
NETIF_F_HW_TC change, so after

	ethtool -K ethX hw-tc-offload off

does a subsequent taprio replace still get accepted and programmed into
the ENST registers, and does an already programmed configuration stay
active?  The usual spelling for this gate elsewhere is
dev->features & NETIF_F_HW_TC (see tc_can_offload()).

> +
> +	/* Check if Device is in runtime suspend */
> +	if (unlikely(pm_runtime_suspended(&bp->pdev->dev))) {
> +		netdev_err(netdev, "Device is in runtime suspend\n");
> +		return -EOPNOTSUPP;
> +	}

[Severity: High]
Since this is now presented as the precondition for all ndo_setup_tc
paths, is a plain status read enough to keep the register clocks on for
the duration of the offload?

pm_runtime_suspended() only reports runtime_status == RPM_SUSPENDED, so it
returns false both when the device is RPM_ACTIVE with a pending
autosuspend and when it is RPM_SUSPENDING. macb_close() drops the
reference asynchronously:

	pm_runtime_put(&bp->pdev->dev);

which arms the MACB_PM_TIMEOUT (100 ms) autosuspend. Can the timer then
expire right after the check passes, so that

macb_runtime_suspend()
    macb_clks_disable(bp->pclk, bp->hclk, bp->tx_clk, bp->rx_clk, bp->tsu_clk);

runs while macb_taprio_setup_replace() or macb_taprio_destroy() is still
writing ENST_CONTROL and the per-queue ENST_START_TIME / ENST_ON_TIME /
ENST_OFF_TIME registers?  macb_taprio_setup_replace() does sleeping work
(kcalloc(GFP_KERNEL), phylink_ethtool_ksettings_get()) before it reaches
the register block, so the window is not small, and bp->lock does not
exclude the PM core.

Every other register-touching path in this driver brackets access with a
usage reference instead, for example macb_mdio_read()/macb_mdio_write(),
macb_open() and at91ether_open() use
pm_runtime_resume_and_get() ... pm_runtime_put_autosuspend(). Would
replacing the check with that pairing here be preferable to relocating
it, given the MQPRIO and CBS handlers added later in the series inherit
this precondition too?

> +
>  	switch (type) {
>  	case TC_SETUP_QDISC_TAPRIO:
>  		return macb_setup_taprio(netdev, type_data);

[Severity: Medium]
The guard above now runs before the switch, so it also fires for
tc_setup_type values this driver never handled. Is the netdev_err() on
those paths intended?

net/sched/cls_api.c:tcf_block_offload_cmd() calls ndo_setup_tc
unconditionally with non-NULL type_data:

	if (dev->netdev_ops->ndo_setup_tc) {
		int err;

		err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_BLOCK, &bo);
		if (err < 0) {
			if (err != -EOPNOTSUPP)
				NL_SET_ERR_MSG(extack, "Driver ndo_setup_tc failed");

and -EOPNOTSUPP is a normal outcome there (tcf_block_offload_bind() falls
back to no_offload_dev_inc() and returns 0).

So on MACB_CAPS_TC hardware whose interface is down and has autosuspended,
does every clsact/ingress qdisc add and delete now print
"Device is in runtime suspend" at error level while the command itself
succeeds?  The message is not rate limited, so repeating the operation
would repeat the log line. At this commit the same applies to
qdisc_offload_query_caps():

	if (ops->ndo_setup_tc)
		ops->ndo_setup_tc(dev, TC_QUERY_CAPS, &base);

The TC_QUERY_CAPS case is later moved ahead of the guard by the following
patch in the series ("net: macb: Add MQPRIO qdisc hardware offload
support"), but TC_SETUP_BLOCK still reaches it at the end of the series.

Before this patch these types went straight to the default case and
returned -EOPNOTSUPP with no PM inspection and no log output. Could the
commit message mention this, since it is described as centralizing
existing preconditions?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909142056.1433875-1-vineeth.karumanchi%40amd.com

[PATCH net-next v2 3/4] net: macb: Add MQPRIO qdisc hardware offload support

[Vineeth Karumanchi]

Add support for TC_SETUP_QDISC_MQPRIO hardware offload, allowing
traffic class to queue mapping via the mqprio qdisc.

Implement macb_setup_mqprio() which configures TC-to-queue mappings
through netdev_set_num_tc() and netdev_set_tc_queue(), and resets
them when num_tc is zero. The driver advertises TC_MQPRIO_HW_OFFLOAD_TCS
offload level.

Add macb_tc_query_caps() to report mqprio capabilities. Setting
validate_queue_counts to true delegates queue count and overlap
validation to the mqprio core via mqprio_validate_queue_counts(),
avoiding redundant checks in the driver.

Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
Changes in v2:
- No functional change; rebased on net-next, which renamed the
  struct net_device pointer to "netdev" (was "dev"/"ndev").

 drivers/net/ethernet/cadence/macb_main.c | 59 ++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index ff7e02d3fab8..67150ff03066 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4492,6 +4492,60 @@ static int macb_setup_taprio(struct net_device *netdev,
 	return err;
 }
 
+static int macb_setup_mqprio(struct net_device *netdev,
+			     struct tc_mqprio_qopt_offload *mqprio)
+{
+	struct tc_mqprio_qopt *qopt = &mqprio->qopt;
+	u8 num_tc = qopt->num_tc;
+	int err;
+	u8 i;
+
+	/* Handle reset case early */
+	if (!num_tc) {
+		netdev_reset_tc(netdev);
+		return 0;
+	}
+
+	/* Configure traffic classes */
+	qopt->hw = TC_MQPRIO_HW_OFFLOAD_TCS;
+
+	err = netdev_set_num_tc(netdev, num_tc);
+	if (err)
+		return err;
+
+	for (i = 0; i < num_tc; i++) {
+		err = netdev_set_tc_queue(netdev, i, qopt->count[i],
+					  qopt->offset[i]);
+		if (err)
+			goto err_reset_tc;
+
+		netdev_dbg(netdev, "MQPRIO: TC%d -> queue %u (count=%u)\n",
+			   i, qopt->offset[i], qopt->count[i]);
+	}
+
+	return 0;
+
+err_reset_tc:
+	netdev_reset_tc(netdev);
+	return err;
+}
+
+static int macb_tc_query_caps(struct net_device *netdev,
+			      struct tc_query_caps_base *base)
+{
+	switch (base->type) {
+	case TC_SETUP_QDISC_MQPRIO: {
+		struct tc_mqprio_caps *caps = base->caps;
+
+		caps->validate_queue_counts = true;
+
+		return 0;
+	}
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static int macb_setup_tc(struct net_device *netdev, enum tc_setup_type type,
 			 void *type_data)
 {
@@ -4500,6 +4554,9 @@ static int macb_setup_tc(struct net_device *netdev, enum tc_setup_type type,
 	if (!netdev || !type_data)
 		return -EINVAL;
 
+	if (type == TC_QUERY_CAPS)
+		return macb_tc_query_caps(netdev, type_data);
+
 	bp = netdev_priv(netdev);
 
 	if (unlikely(!(netdev->hw_features & NETIF_F_HW_TC)))
@@ -4512,6 +4569,8 @@ static int macb_setup_tc(struct net_device *netdev, enum tc_setup_type type,
 	}
 
 	switch (type) {
+	case TC_SETUP_QDISC_MQPRIO:
+		return macb_setup_mqprio(netdev, type_data);
 	case TC_SETUP_QDISC_TAPRIO:
 		return macb_setup_taprio(netdev, type_data);
 	default:
-- 
2.43.0

Re: [PATCH net-next v2 3/4] net: macb: Add MQPRIO qdisc hardware offload support

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 (1) · Medium: 2 · Low: 1

- [Medium] macb_setup_mqprio()
  (drivers/net/ethernet/cadence/macb_main.c:4495) consumes only…
- [Medium] The mqprio offload teardown path is unreachable while the MAC
  is runtime suspended, leaving the netdev traffic-class state installed…
- [Low] Concern (judged a false positive at validation): mqprio offload
  is only usable on MACB_CAPS_TC (TSN) parts and only while the device…

Pre-existing issues:
- [High] The new TC_QUERY_CAPS handler macb_tc_query_caps()
  (macb_main.c:4533) answers only TC_SETUP_QDISC_MQPRIO and returns…

--

net: macb: Add MQPRIO qdisc hardware offload support

This adds a TC_SETUP_QDISC_MQPRIO handler that installs the traffic class
to queue mapping with netdev_set_num_tc()/netdev_set_tc_queue() and resets
it when num_tc is zero.  It also adds a TC_QUERY_CAPS handler that reports
validate_queue_counts for mqprio.

A few questions below.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index ff7e02d3fab8..67150ff03066 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4492,6 +4492,60 @@ static int macb_setup_taprio(struct net_device *netdev,
>  	return err;
>  }
>  
> +static int macb_setup_mqprio(struct net_device *netdev,
> +			     struct tc_mqprio_qopt_offload *mqprio)
> +{
> +	struct tc_mqprio_qopt *qopt = &mqprio->qopt;
> +	u8 num_tc = qopt->num_tc;
> +	int err;
> +	u8 i;
> +
> +	/* Handle reset case early */
> +	if (!num_tc) {
> +		netdev_reset_tc(netdev);
> +		return 0;
> +	}
> +
> +	/* Configure traffic classes */
> +	qopt->hw = TC_MQPRIO_HW_OFFLOAD_TCS;

[Severity: Medium]
Should macb_setup_mqprio() reject the offload parameters it does not
implement instead of reporting the request as offloaded?

net/sched/sch_mqprio.c:mqprio_enable_offload() fills in more than qopt
before calling into the driver:

	case TC_MQPRIO_MODE_CHANNEL:
		mqprio.flags = priv->flags;
		if (priv->flags & TC_MQPRIO_F_MODE)
			mqprio.mode = priv->mode;
	...
	err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_MQPRIO, &mqprio);
	if (err)
		return err;
	priv->hw_offload = mqprio.qopt.hw;

macb_setup_mqprio() looks only at mqprio->qopt, so a request like

	tc qdisc add dev eth0 root mqprio num_tc 2 map ... queues 1@0 1@1 \
	    hw 1 mode channel shaper bw_rlimit max_rate 100Mbit 200Mbit

returns 0 with qopt->hw set, and tc reports hardware offload, while
mqprio->mode, mqprio->shaper, mqprio->min_rate[] and mqprio->max_rate[] are
not referenced anywhere in macb_main.c.

Other drivers reject these up front, for example cpsw_priv.c does

	if (mqprio->mode != TC_MQPRIO_MODE_DCB)
		return -EINVAL;

and mlx5 en_main.c / cxgb4_tc_mqprio.c return -EOPNOTSUPP for modes and
shapers they cannot honour.

Would it be worth also passing the reason back through mqprio->extack?

> +
> +	err = netdev_set_num_tc(netdev, num_tc);
> +	if (err)
> +		return err;
> +
> +	for (i = 0; i < num_tc; i++) {
> +		err = netdev_set_tc_queue(netdev, i, qopt->count[i],
> +					  qopt->offset[i]);
> +		if (err)
> +			goto err_reset_tc;
> +
> +		netdev_dbg(netdev, "MQPRIO: TC%d -> queue %u (count=%u)\n",
> +			   i, qopt->offset[i], qopt->count[i]);
> +	}
> +
> +	return 0;
> +
> +err_reset_tc:
> +	netdev_reset_tc(netdev);
> +	return err;
> +}
> +
> +static int macb_tc_query_caps(struct net_device *netdev,
> +			      struct tc_query_caps_base *base)
> +{
> +	switch (base->type) {
> +	case TC_SETUP_QDISC_MQPRIO: {
> +		struct tc_mqprio_caps *caps = base->caps;
> +
> +		caps->validate_queue_counts = true;
> +
> +		return 0;
> +	}
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}

[Severity: High]
This is a pre-existing issue and not introduced by this patch, but since
this is the callback where it would be declared: should
TC_SETUP_QDISC_TAPRIO also be answered here with
tc_taprio_caps.gate_mask_per_txq = true?

net/sched/sch_taprio.c:taprio_sched_to_offload() converts the mask to a
queue mask only when the driver asks for it:

		if (caps->gate_mask_per_txq)
			e->gate_mask = tc_map_to_queue_mask(dev,
							    entry->gate_mask);
		else
			e->gate_mask = entry->gate_mask;

and include/net/pkt_sched.h documents struct tc_taprio_sched_entry as:

	/* The gate_mask in the offloading side refers to traffic classes */
	u32 gate_mask;

macb_taprio_setup_replace() decodes that value as a hardware queue index:

		queue_id = order_base_2(entry->gate_mask);
		if (queue_id >= bp->num_queues) {
	...
		/* Enable ENST for all configured queues in one write */
		gem_writel(bp, ENST_CONTROL, configured_queues);

With a non-identity map, say "taprio num_tc 2 queues 2@0 2@2", a sched-entry
gate mask of 0x2 means TC1, i.e. queues 2 and 3, but queue_id becomes 1, so
the ENST window is programmed on hardware queue 1 while queues 2 and 3 stay
open, and configured_queues (a traffic class mask) is written into
GEM_ENST_CONTROL whose bits are per hardware queue.  The

		if (!is_power_of_2(entry->gate_mask))

check also rejects any entry that opens more than one traffic class.

Since this patch is what makes non-identity TC to queue maps a normal
configuration for macb, does the taprio gate mask handling need updating in
the same series?

>  static int macb_setup_tc(struct net_device *netdev, enum tc_setup_type type,
>  			 void *type_data)
>  {
> @@ -4500,6 +4554,9 @@ static int macb_setup_tc(struct net_device *netdev, enum tc_setup_type type,
>  	if (!netdev || !type_data)
>  		return -EINVAL;
>  
> +	if (type == TC_QUERY_CAPS)
> +		return macb_tc_query_caps(netdev, type_data);
> +
>  	bp = netdev_priv(netdev);
>  
>  	if (unlikely(!(netdev->hw_features & NETIF_F_HW_TC)))

[Severity: Low]
This isn't a bug, but the commit message doesn't mention that the new
offload is only reachable on parts where macb_init_dflt() sets
NETIF_F_HW_TC:

	/* Enable TC offload for TSN-capable hardware */
	if (bp->caps & MACB_CAPS_TC)
		netdev->hw_features |= NETIF_F_HW_TC;

macb_setup_mqprio() only manipulates net_device software state, so on
non-TSN macb variants "hw 1" returns -EOPNOTSUPP from macb_setup_tc().
Could the log state that mqprio hardware offload is limited to
MACB_CAPS_TC parts?

> @@ -4512,6 +4569,8 @@ static int macb_setup_tc(struct net_device *netdev, enum tc_setup_type type,
>  	}
>  
>  	switch (type) {
> +	case TC_SETUP_QDISC_MQPRIO:
> +		return macb_setup_mqprio(netdev, type_data);

[Severity: Medium]
Can the mqprio teardown be lost while the device is runtime suspended?

TC_QUERY_CAPS is dispatched before the runtime PM check, but
TC_SETUP_QDISC_MQPRIO is dispatched after it, so the num_tc == 0 path in
macb_setup_mqprio() that calls netdev_reset_tc() cannot run in that state:

	/* Check if Device is in runtime suspend */
	if (unlikely(pm_runtime_suspended(&bp->pdev->dev))) {
		netdev_err(netdev, "Device is in runtime suspend\n");
		return -EOPNOTSUPP;
	}

macb_close() drops the runtime PM reference and macb_probe() enables
autosuspend, so this sequence reaches it:

	ip link set eth0 up
	tc qdisc replace dev eth0 root mqprio ... hw 1
	ip link set eth0 down		/* autosuspend after MACB_PM_TIMEOUT */
	tc qdisc del dev eth0 root

net/sched/sch_mqprio.c:mqprio_destroy() skips its own fallback when the
qdisc was offloaded:

	if (priv->hw_offload && dev->netdev_ops->ndo_setup_tc)
		mqprio_disable_offload(sch);
	else
		netdev_set_num_tc(dev, 0);

and mqprio_disable_offload() discards the ndo_setup_tc() return value, so
the -EOPNOTSUPP is silent and dev->num_tc, dev->tc_to_txq[] and
dev->prio_tc_map[] stay configured after the owning qdisc is freed,
continuing to steer transmit queue selection.

Since this handler touches no registers, would it be better to handle the
num_tc == 0 reset before the runtime suspend check, the same way
TC_QUERY_CAPS is?

>  	case TC_SETUP_QDISC_TAPRIO:
>  		return macb_setup_taprio(netdev, type_data);
>  	default:

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909142056.1433875-1-vineeth.karumanchi%40amd.com

[PATCH net-next v2 4/4] net: macb: Add TSN CBS TC offload support

[Vineeth Karumanchi]

Add Credit-Based Shaper (CBS/IEEE 802.1Qav) TC offload support for
time-sensitive networking on GEM hardware. CBS is restricted to the
two highest-priority queues: Queue A (num_queues - 1) and Queue B
(num_queues - 2), matching hardware capability.

Validate that idleslope is positive and does not exceed the link
speed, preventing negative values from bypassing the bounds check
due to signed-to-unsigned promotion.

The idle slope register value is computed differently based on hardware
variant:

  High-speed GEM: scale idleslope linearly to the full 32-bit register
  range relative to link speed.

  Standard MACB: convert the kbps idleslope into the register's native
  unit, which depends on the interface width:
    - 1G (8-bit GMII):    bytes/sec,   scale kbps by 1000/8 (125)
    - 10/100M (4-bit MII): nibbles/sec, scale kbps by 1000/4 (250)

Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
Changes in v2:
- macb_cbs_get_queue_params() now returns the idleslope register offset
  (u32 *idleslope_reg) instead of a bool flag, and the idleslope is
  programmed via bp->macb_reg_writel(), dropping the per-queue if/else
  that open-coded gem_writel(CBS_IDLESLOPE_Q_A/Q_B).
- Expanded the idleslope kbps-to-hardware-unit conversion comment.
- Zero-initialize kset in macb_cbs_add() so an unpopulated link speed
  reads as 0 and is rejected; reorder locals to keep the declarations
  in reverse-christmas-tree order.
- Rebased on net-next, which renamed the struct net_device pointer to
  "netdev" (was "dev"/"ndev").

 drivers/net/ethernet/cadence/macb.h      |   9 ++
 drivers/net/ethernet/cadence/macb_main.c | 116 +++++++++++++++++++++++
 2 files changed, 125 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index f5359549f3d4..20faeba77d6d 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -184,6 +184,9 @@
 #define GEM_DCFG8		0x029C /* Design Config 8 */
 #define GEM_DCFG10		0x02A4 /* Design Config 10 */
 #define GEM_DCFG12		0x02AC /* Design Config 12 */
+#define GEM_CBS_CONTROL		0x04BC /* CBS Control Register */
+#define GEM_CBS_IDLESLOPE_Q_A	0x04C0 /* CBS IdleSlope Queue A */
+#define GEM_CBS_IDLESLOPE_Q_B	0x04C4 /* CBS IdleSlope Queue B */
 #define GEM_ENST_CONTROL	0x0880 /* ENST control register */
 #define GEM_USX_CONTROL		0x0A80 /* High speed PCS control register */
 #define GEM_USX_STATUS		0x0A88 /* High speed PCS status register */
@@ -224,6 +227,12 @@
 #define GEM_ENST_ON_TIME(hw_q)		(0x0820 + ((hw_q) << 2))
 #define GEM_ENST_OFF_TIME(hw_q)		(0x0840 + ((hw_q) << 2))
 
+/* Bitfields in CBS_CONTROL */
+#define GEM_CBS_ENABLE_QUEUE_A_OFFSET	0
+#define GEM_CBS_ENABLE_QUEUE_A_SIZE	1
+#define GEM_CBS_ENABLE_QUEUE_B_OFFSET	1
+#define GEM_CBS_ENABLE_QUEUE_B_SIZE	1
+
 /* Bitfields in ENST_CONTROL */
 #define GEM_ENST_DISABLE_QUEUE_OFFSET	16
 
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 67150ff03066..00c1c619dea8 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -99,6 +99,10 @@ struct sifive_fu540_macb_mgmt {
 
 #define MACB_MDIO_TIMEOUT	1000000 /* in usecs */
 
+/* CBS port transmit rate factors: 1000/interface_width */
+#define MACB_CBS_PORT_RATE_1G		125	/* 1000/8 for GMII (8-bit) */
+#define MACB_CBS_PORT_RATE_10_100M	250	/* 1000/4 for MII (4-bit) */
+
 /* DMA buffer descriptor might be different size
  * depends on hardware configuration:
  *
@@ -4492,6 +4496,116 @@ static int macb_setup_taprio(struct net_device *netdev,
 	return err;
 }
 
+static int macb_cbs_get_queue_params(struct macb *bp, u8 queue_num,
+				     u32 *enable_bit, u32 *idleslope_reg)
+{
+	/* Queue A is highest priority (num_queues - 1) */
+	if (queue_num == bp->num_queues - 1) {
+		*enable_bit = GEM_BIT(CBS_ENABLE_QUEUE_A);
+		*idleslope_reg = GEM_CBS_IDLESLOPE_Q_A;
+		return 0;
+	}
+
+	/* Queue B is second highest priority (num_queues - 2) */
+	if (queue_num == bp->num_queues - 2) {
+		*enable_bit = GEM_BIT(CBS_ENABLE_QUEUE_B);
+		*idleslope_reg = GEM_CBS_IDLESLOPE_Q_B;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int macb_cbs_add(struct net_device *netdev,
+			struct tc_cbs_qopt_offload *qopt)
+{
+	u32 enable_bit, idleslope, speed_kbps, ctrl, idleslope_reg;
+	struct ethtool_link_ksettings kset = {};
+	struct macb *bp = netdev_priv(netdev);
+	int err;
+
+	err = macb_cbs_get_queue_params(bp, qopt->queue, &enable_bit, &idleslope_reg);
+	if (err) {
+		netdev_err(netdev, "CBS: Queue %d not eligible (only top 2 queues support CBS)\n",
+			   qopt->queue);
+		return -EINVAL;
+	}
+
+	/* idleslope is calibrated for the current link speed; CBS is not
+	 * reprogrammed on link-speed changes, so it must be reconfigured
+	 * if the link speed changes.
+	 */
+	phylink_ethtool_ksettings_get(bp->phylink, &kset);
+
+	if (!kset.base.speed || kset.base.speed == SPEED_UNKNOWN) {
+		netdev_err(netdev, "CBS: Invalid link speed\n");
+		return -EINVAL;
+	}
+
+	speed_kbps = kset.base.speed * 1000;
+
+	if (qopt->idleslope <= 0 || (u32)qopt->idleslope > speed_kbps) {
+		netdev_err(netdev, "CBS: invalid idleslope %d (must be 1..%u kbps)\n",
+			   qopt->idleslope, speed_kbps);
+		return -EINVAL;
+	}
+
+	/* qopt->idleslope is in kbps; convert to the units the hardware
+	 * register expects:
+	 * - High-speed GEM: fraction of port bandwidth, scaled to the full
+	 *   32-bit register range
+	 * - Standard MACB: the register counts bytes/sec in 1G (8-bit GMII)
+	 *   mode and nibbles/sec in 10/100 (4-bit MII) mode, so scale kbps
+	 *   by 1000/8 (125) or 1000/4 (250) respectively
+	 */
+	if (bp->caps & MACB_CAPS_HIGH_SPEED)
+		idleslope = DIV_ROUND_UP_ULL((u64)qopt->idleslope * U32_MAX, speed_kbps);
+	else
+		idleslope = (u32)qopt->idleslope * (kset.base.speed >= 1000 ?
+					       MACB_CBS_PORT_RATE_1G : MACB_CBS_PORT_RATE_10_100M);
+
+	scoped_guard(spinlock_irqsave, &bp->lock) {
+		/* Disable CBS for the queue before updating idleslope */
+		ctrl = gem_readl(bp, CBS_CONTROL) & ~enable_bit;
+		gem_writel(bp, CBS_CONTROL, ctrl);
+		/* Update idleslope for the queue */
+		bp->macb_reg_writel(bp, idleslope_reg, idleslope);
+		/* Re-enable CBS for the queue with new idleslope */
+		gem_writel(bp, CBS_CONTROL, ctrl | enable_bit);
+	}
+
+	netdev_dbg(netdev, "CBS: Configured queue %d with idleslope 0x%x\n",
+		   qopt->queue, idleslope);
+
+	return 0;
+}
+
+static void macb_cbs_destroy(struct net_device *netdev, u8 queue_num)
+{
+	struct macb *bp = netdev_priv(netdev);
+	u32 enable_bit, idleslope_reg;
+
+	if (macb_cbs_get_queue_params(bp, queue_num, &enable_bit, &idleslope_reg))
+		return;
+
+	scoped_guard(spinlock_irqsave, &bp->lock) {
+		gem_writel(bp, CBS_CONTROL, gem_readl(bp, CBS_CONTROL) & ~enable_bit);
+		bp->macb_reg_writel(bp, idleslope_reg, 0);
+	}
+
+	netdev_dbg(netdev, "CBS: Disabled queue %d\n", queue_num);
+}
+
+static int macb_setup_cbs(struct net_device *netdev,
+			  struct tc_cbs_qopt_offload *qopt)
+{
+	if (qopt->enable)
+		return macb_cbs_add(netdev, qopt);
+
+	macb_cbs_destroy(netdev, qopt->queue);
+	return 0;
+}
+
 static int macb_setup_mqprio(struct net_device *netdev,
 			     struct tc_mqprio_qopt_offload *mqprio)
 {
@@ -4571,6 +4685,8 @@ static int macb_setup_tc(struct net_device *netdev, enum tc_setup_type type,
 	switch (type) {
 	case TC_SETUP_QDISC_MQPRIO:
 		return macb_setup_mqprio(netdev, type_data);
+	case TC_SETUP_QDISC_CBS:
+		return macb_setup_cbs(netdev, type_data);
 	case TC_SETUP_QDISC_TAPRIO:
 		return macb_setup_taprio(netdev, type_data);
 	default:
-- 
2.43.0

Re: [PATCH net-next v2 4/4] net: macb: Add TSN CBS TC offload support

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 6 · Low: 1

- [Medium] macb_cbs_add() (drivers/net/ethernet/cadence/macb_main.c:4519)
  bakes the instantaneous link speed into the value written to…
- [Medium] macb_cbs_add() consumes only qopt->queue, qopt->enable and
  qopt->idleslope from struct tc_cbs_qopt_offload and returns 0,…
- [Medium] The CBS disable request can be rejected before it reaches
  macb_cbs_destroy(), leaving the CBS enable bit and a stale idleslope…
- [Medium] The CBS enable bits and idleslope registers programmed by
  macb_cbs_add() are never cleared on interface down and never…
- [Medium] macb_cbs_add() and macb_cbs_destroy() perform MMIO
  (gem_readl/gem_writel of GEM_CBS_CONTROL at…
- [Medium] Incorrect queue mapping allows configuring CBS on the base
  best-effort queue on 1- or 2-queue devices.
- [Low] macb_cbs_add() samples the link speed with
  phylink_ethtool_ksettings_get() at macb_main.c:4538 (outside bp->lock)…

--

net: macb: Add TSN CBS TC offload support

This adds Credit-Based Shaper (IEEE 802.1Qav) TC offload for the two
highest-priority GEM queues, computing the idleslope register value from
the current link speed and arming the per-queue CBS enable bit. A few
questions about the link-speed dependency, the ignored qopt fields and
the interaction with runtime PM / suspend below.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 67150ff03066..00c1c619dea8 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -4492,6 +4496,116 @@ static int macb_setup_taprio(struct net_device *netdev,
>  	return err;
>  }
>  
> +static int macb_cbs_get_queue_params(struct macb *bp, u8 queue_num,
> +				     u32 *enable_bit, u32 *idleslope_reg)
> +{

[ ... ]

> +static int macb_cbs_add(struct net_device *netdev,
> +			struct tc_cbs_qopt_offload *qopt)
> +{
> +	u32 enable_bit, idleslope, speed_kbps, ctrl, idleslope_reg;
> +	struct ethtool_link_ksettings kset = {};
> +	struct macb *bp = netdev_priv(netdev);
> +	int err;
> +
> +	err = macb_cbs_get_queue_params(bp, qopt->queue, &enable_bit, &idleslope_reg);
> +	if (err) {
> +		netdev_err(netdev, "CBS: Queue %d not eligible (only top 2 queues support CBS)\n",
> +			   qopt->queue);
> +		return -EINVAL;
> +	}
> +
> +	/* idleslope is calibrated for the current link speed; CBS is not
> +	 * reprogrammed on link-speed changes, so it must be reconfigured
> +	 * if the link speed changes.
> +	 */
> +	phylink_ethtool_ksettings_get(bp->phylink, &kset);

[Severity: Medium]
Is there anything that makes the "must be reconfigured if the link speed
changes" requirement in this comment actually happen?

Grepping the tree, GEM_CBS_CONTROL and GEM_CBS_IDLESLOPE_Q_A/Q_B are
referenced only from macb_cbs_add() and macb_cbs_destroy().
macb_mac_link_up() reprograms NCFGR SPD/GBE for the new speed but leaves
the CBS registers alone, and on the core side cbs_dev_notifier() ->
cbs_set_port_rate() only refreshes the software port_rate on NETDEV_UP /
NETDEV_CHANGE, it never re-issues ndo_setup_tc(TC_SETUP_QDISC_CBS).

macb_cbs_add() also keeps no copy of qopt in struct macb, so is there any
data left to replay even if such a hook were added?

For standard MACB the *unit* of the register is chosen by the speed
(kset.base.speed >= 1000 selects bytes/sec vs nibbles/sec), so after a
1000 -> 100 Mbit renegotiation the register still holds a bytes/sec value
that hardware now consumes as nibbles/sec at one tenth of the port rate,
while the enable bit stays armed and userspace still sees the qdisc as
offloaded. Would deriving the port rate from the qopt parameters the way
stmmac's tc_setup_cbs() does avoid the dependency entirely?

  port_transmit_rate_kbps = qopt->idleslope - qopt->sendslope;

Related: rejecting speed 0 / SPEED_UNKNOWN means CBS cannot be installed
at all while the link is down. Is that intended?

> +
> +	if (!kset.base.speed || kset.base.speed == SPEED_UNKNOWN) {
> +		netdev_err(netdev, "CBS: Invalid link speed\n");
> +		return -EINVAL;
> +	}
> +
> +	speed_kbps = kset.base.speed * 1000;

[Severity: Low]
Can the speed sampled here change before the register write below?

The phylink_ethtool_ksettings_get() call is outside bp->lock, and the
idleslope is programmed later under scoped_guard(spinlock_irqsave,
&bp->lock). phylink_resolve() runs from the workqueue holding only
pl->phydev_mutex, phy->lock and pl->state_mutex - it does not take RTNL -
and calls phylink_link_up() -> macb_mac_link_up(), which changes the MAC
speed under bp->lock:

  macb_mac_link_up()
      spin_lock_irqsave(&bp->lock, flags);
      ctrl = macb_or_gem_readl(bp, NCFGR);
      ctrl &= ~(MACB_BIT(SPD) | MACB_BIT(FD));

So RTNL held by the qdisc path does not exclude a renegotiation landing
between the read of kset.base.speed and the write of idleslope_reg,
leaving a value scaled with the 1G factor on a 100 Mbit link. Should the
speed be sampled inside the same critical section as the programming, or
re-checked afterwards?

> +
> +	if (qopt->idleslope <= 0 || (u32)qopt->idleslope > speed_kbps) {
> +		netdev_err(netdev, "CBS: invalid idleslope %d (must be 1..%u kbps)\n",
> +			   qopt->idleslope, speed_kbps);
> +		return -EINVAL;
> +	}

[Severity: Medium]
Should qopt->hicredit, qopt->locredit and qopt->sendslope be rejected
rather than silently dropped?

Only qopt->queue, qopt->enable and qopt->idleslope are consumed here, and
0 is returned; those three field names do not appear anywhere in
macb_main.c. On the core side cbs_enable_offload() fills all four shaper
parameters:

  cbs.hicredit = opt->hicredit;
  cbs.locredit = opt->locredit;
  cbs.idleslope = opt->idleslope;
  cbs.sendslope = opt->sendslope;

and on a 0 return cbs_change() stores them and cbs_dump() echoes them
back, so "tc qdisc show" advertises credit limits and a sendslope that
hardware was never configured with.

sendslope in particular carries the port transmit rate in this ABI
(sendslope = idleslope - portTransmitRate). Would returning -EOPNOTSUPP
with an extack for the unsupported fields be preferable to reporting a
complete offload?

> +
> +	/* qopt->idleslope is in kbps; convert to the units the hardware
> +	 * register expects:
> +	 * - High-speed GEM: fraction of port bandwidth, scaled to the full
> +	 *   32-bit register range
> +	 * - Standard MACB: the register counts bytes/sec in 1G (8-bit GMII)
> +	 *   mode and nibbles/sec in 10/100 (4-bit MII) mode, so scale kbps
> +	 *   by 1000/8 (125) or 1000/4 (250) respectively
> +	 */
> +	if (bp->caps & MACB_CAPS_HIGH_SPEED)
> +		idleslope = DIV_ROUND_UP_ULL((u64)qopt->idleslope * U32_MAX, speed_kbps);
> +	else
> +		idleslope = (u32)qopt->idleslope * (kset.base.speed >= 1000 ?
> +					       MACB_CBS_PORT_RATE_1G : MACB_CBS_PORT_RATE_10_100M);
> +
> +	scoped_guard(spinlock_irqsave, &bp->lock) {
> +		/* Disable CBS for the queue before updating idleslope */
> +		ctrl = gem_readl(bp, CBS_CONTROL) & ~enable_bit;
> +		gem_writel(bp, CBS_CONTROL, ctrl);
> +		/* Update idleslope for the queue */
> +		bp->macb_reg_writel(bp, idleslope_reg, idleslope);
> +		/* Re-enable CBS for the queue with new idleslope */
> +		gem_writel(bp, CBS_CONTROL, ctrl | enable_bit);
> +	}

[Severity: Medium]
Can these register accesses run with the clocks already gated?

The only protection is the pm_runtime_suspended(&bp->pdev->dev) sample
taken earlier in macb_setup_tc(); no usage reference is taken here (no
pm_runtime_resume_and_get() / pm_runtime_put_autosuspend()). macb_close()
drops the driver's reference with pm_runtime_put() and the autosuspend
delay is MACB_PM_TIMEOUT (100 ms), and macb_runtime_suspend() gates the
clocks without taking bp->lock:

  macb_runtime_suspend()
      if (!(device_may_wakeup(dev)))
              macb_clks_disable(bp->pclk, bp->hclk, bp->tx_clk, bp->rx_clk, bp->tsu_clk);

so the scoped_guard(spinlock_irqsave, &bp->lock) here cannot delay the
transition. macb_mdio_read()/macb_mdio_write() and macb_open() take
pm_runtime_resume_and_get() before touching registers - should the CBS
paths do the same?

> +
> +	netdev_dbg(netdev, "CBS: Configured queue %d with idleslope 0x%x\n",
> +		   qopt->queue, idleslope);
> +
> +	return 0;
> +}
> +
> +static void macb_cbs_destroy(struct net_device *netdev, u8 queue_num)
> +{
> +	struct macb *bp = netdev_priv(netdev);
> +	u32 enable_bit, idleslope_reg;
> +
> +	if (macb_cbs_get_queue_params(bp, queue_num, &enable_bit, &idleslope_reg))
> +		return;
> +
> +	scoped_guard(spinlock_irqsave, &bp->lock) {
> +		gem_writel(bp, CBS_CONTROL, gem_readl(bp, CBS_CONTROL) & ~enable_bit);
> +		bp->macb_reg_writel(bp, idleslope_reg, 0);
> +	}

[Severity: Medium]
Does the CBS state survive an interface down/up or a suspend/resume in a
way that was intended?

macb_reset_hw(), used by macb_close() and by the non-WoL suspend path,
clears NCR/TSR/RSR/PBUFRXCUT and the per-queue interrupts but does not
touch GEM_CBS_CONTROL or GEM_CBS_IDLESLOPE_Q_A/Q_B, so after "ip link set
down" the shaper stays armed with the idleslope computed for the previous
link speed, and the next macb_open() transmits under it even with no CBS
qdisc attached.

In the other direction macb_resume() restores the state the driver tracks:

  if (netdev->hw_features & NETIF_F_NTUPLE)
          gem_writel_n(bp, ETHT, SCRT2_ETHT, bp->pm_data.scrt2);
  ...
  macb_init_hw(bp);

but there is no CBS restore, so if the controller loses register content
an installed, still-"offloaded" CBS qdisc silently stops shaping.

Since macb_cbs_add() stores nothing in struct macb, is there any way to
repair either direction at runtime?

> +
> +	netdev_dbg(netdev, "CBS: Disabled queue %d\n", queue_num);
> +}
> +
> +static int macb_setup_cbs(struct net_device *netdev,
> +			  struct tc_cbs_qopt_offload *qopt)
> +{
> +	if (qopt->enable)
> +		return macb_cbs_add(netdev, qopt);
> +
> +	macb_cbs_destroy(netdev, qopt->queue);
> +	return 0;
> +}
> +

[ ... ]

> @@ -4571,6 +4685,8 @@ static int macb_setup_tc(struct net_device *netdev, enum tc_setup_type type,
>  	switch (type) {
>  	case TC_SETUP_QDISC_MQPRIO:
>  		return macb_setup_mqprio(netdev, type_data);
> +	case TC_SETUP_QDISC_CBS:
> +		return macb_setup_cbs(netdev, type_data);

[Severity: Medium]
Can the disable request be dropped before it reaches this case?

macb_setup_tc() returns -EOPNOTSUPP for every TC_SETUP_QDISC_CBS call,
including enable == 0, when the device is runtime suspended:

  if (unlikely(pm_runtime_suspended(&bp->pdev->dev))) {
          netdev_err(netdev, "Device is in runtime suspend\n");
          return -EOPNOTSUPP;
  }

so this switch is never reached and macb_cbs_destroy() never runs. A
sequence like: offload CBS on the top queue, "ip link set ethX down"
(macb_close() -> pm_runtime_put(), then the 100 ms autosuspend), then "tc
qdisc del ..." ends in cbs_destroy() -> cbs_disable_offload(), which only
warns:

  err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
  if (err < 0)
          pr_warn("Couldn't disable CBS offload for queue %d\n", cbs.queue);

and frees the qdisc anyway. With macb_reset_hw()/macb_init_hw() not
touching GEM_CBS_CONTROL, does the queue stay credit-shaped with the old
idleslope after the next "ip link set up" even though no CBS qdisc
exists?

>  	case TC_SETUP_QDISC_TAPRIO:
>  		return macb_setup_taprio(netdev, type_data);
>  	default:

[ ... ]

> @@ -4492,6 +4496,116 @@ static int macb_setup_taprio(struct net_device *netdev,
>  	return err;
>  }
>  
> +static int macb_cbs_get_queue_params(struct macb *bp, u8 queue_num,
> +				     u32 *enable_bit, u32 *idleslope_reg)
> +{
> +	/* Queue A is highest priority (num_queues - 1) */
> +	if (queue_num == bp->num_queues - 1) {
> +		*enable_bit = GEM_BIT(CBS_ENABLE_QUEUE_A);
> +		*idleslope_reg = GEM_CBS_IDLESLOPE_Q_A;
> +		return 0;
> +	}
> +
> +	/* Queue B is second highest priority (num_queues - 2) */
> +	if (queue_num == bp->num_queues - 2) {
> +		*enable_bit = GEM_BIT(CBS_ENABLE_QUEUE_B);
> +		*idleslope_reg = GEM_CBS_IDLESLOPE_Q_B;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int macb_cbs_add(struct net_device *netdev,
> +			struct tc_cbs_qopt_offload *qopt)
> +{
> +	u32 enable_bit, idleslope, speed_kbps, ctrl, idleslope_reg;
> +	struct ethtool_link_ksettings kset = {};
> +	struct macb *bp = netdev_priv(netdev);
> +	int err;
> +
> +	err = macb_cbs_get_queue_params(bp, qopt->queue, &enable_bit, &idleslope_reg);
> +	if (err) {
> +		netdev_err(netdev, "CBS: Queue %d not eligible (only top 2 queues support CBS)\n",
> +			   qopt->queue);
> +		return -EINVAL;
> +	}
> +
> +	/* idleslope is calibrated for the current link speed; CBS is not
> +	 * reprogrammed on link-speed changes, so it must be reconfigured
> +	 * if the link speed changes.
> +	 */
> +	phylink_ethtool_ksettings_get(bp->phylink, &kset);
> +
> +	if (!kset.base.speed || kset.base.speed == SPEED_UNKNOWN) {
> +		netdev_err(netdev, "CBS: Invalid link speed\n");
> +		return -EINVAL;
> +	}
> +
> +	speed_kbps = kset.base.speed * 1000;
> +
> +	if (qopt->idleslope <= 0 || (u32)qopt->idleslope > speed_kbps) {
> +		netdev_err(netdev, "CBS: invalid idleslope %d (must be 1..%u kbps)\n",
> +			   qopt->idleslope, speed_kbps);
> +		return -EINVAL;
> +	}
> +
> +	/* qopt->idleslope is in kbps; convert to the units the hardware
> +	 * register expects:
> +	 * - High-speed GEM: fraction of port bandwidth, scaled to the full
> +	 *   32-bit register range
> +	 * - Standard MACB: the register counts bytes/sec in 1G (8-bit GMII)
> +	 *   mode and nibbles/sec in 10/100 (4-bit MII) mode, so scale kbps
> +	 *   by 1000/8 (125) or 1000/4 (250) respectively
> +	 */
> +	if (bp->caps & MACB_CAPS_HIGH_SPEED)
> +		idleslope = DIV_ROUND_UP_ULL((u64)qopt->idleslope * U32_MAX, speed_kbps);
> +	else
> +		idleslope = (u32)qopt->idleslope * (kset.base.speed >= 1000 ?
> +					       MACB_CBS_PORT_RATE_1G : MACB_CBS_PORT_RATE_10_100M);
> +
> +	scoped_guard(spinlock_irqsave, &bp->lock) {
> +		/* Disable CBS for the queue before updating idleslope */
> +		ctrl = gem_readl(bp, CBS_CONTROL) & ~enable_bit;
> +		gem_writel(bp, CBS_CONTROL, ctrl);
> +		/* Update idleslope for the queue */
> +		bp->macb_reg_writel(bp, idleslope_reg, idleslope);
> +		/* Re-enable CBS for the queue with new idleslope */
> +		gem_writel(bp, CBS_CONTROL, ctrl | enable_bit);
> +	}
> +
> +	netdev_dbg(netdev, "CBS: Configured queue %d with idleslope 0x%x\n",
> +		   qopt->queue, idleslope);
> +
> +	return 0;
> +}
> +
> +static void macb_cbs_destroy(struct net_device *netdev, u8 queue_num)
> +{
> +	struct macb *bp = netdev_priv(netdev);
> +	u32 enable_bit, idleslope_reg;
> +
> +	if (macb_cbs_get_queue_params(bp, queue_num, &enable_bit, &idleslope_reg))
> +		return;
> +
> +	scoped_guard(spinlock_irqsave, &bp->lock) {
> +		gem_writel(bp, CBS_CONTROL, gem_readl(bp, CBS_CONTROL) & ~enable_bit);
> +		bp->macb_reg_writel(bp, idleslope_reg, 0);
> +	}
> +
> +	netdev_dbg(netdev, "CBS: Disabled queue %d\n", queue_num);
> +}
> +
> +static int macb_setup_cbs(struct net_device *netdev,
> +			  struct tc_cbs_qopt_offload *qopt)
> +{
> +	if (qopt->enable)
> +		return macb_cbs_add(netdev, qopt);
> +
> +	macb_cbs_destroy(netdev, qopt->queue);
> +	return 0;
> +}
> +
>  static int macb_setup_mqprio(struct net_device *netdev,
>  			     struct tc_mqprio_qopt_offload *mqprio)
>  {

[Severity: Medium]
Does this mapping still hold on hardware with only one or two
queues?

bp->num_queues comes from the DCFG6 queue mask in macb_probe(), and
nothing here bounds it from below. On a 2-queue GEM, bp->num_queues -
2 == 0, so a CBS qdisc installed on queue 0 - the base best-effort
queue - matches the Queue B branch and gets GEM_BIT(CBS_ENABLE_QUEUE_B)
plus GEM_CBS_IDLESLOPE_Q_B programmed for it. On single-queue
hardware the same happens one branch earlier: bp->num_queues - 1 == 0,
so queue 0 is treated as Queue A. In both cases macb_cbs_add()
returns 0, the qdisc is reported as offloaded, and the CBS enable bit
is armed for a priority queue the design does not have, while the
traffic on queue 0 is not shaped at all.

The commit message says CBS is restricted to the two highest-priority
queues "matching hardware capability", but the base queue is exactly
the one that is not a priority queue, so the arithmetic silently
includes it once bp->num_queues drops to 2. Should
macb_cbs_get_queue_params() reject the request unless bp->num_queues
is at least 3, or is the intent to gate CBS on a capability flag (the
way MACB_CAPS_* is used elsewhere) and also refuse to advertise it in
macb_tc_query_caps() on such devices? Whichever way, an explicit
lower bound on bp->num_queues here would make the queue A/B
identification safe to read.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909142056.1433875-1-vineeth.karumanchi%40amd.com