[PATCH net v2] net: macb: configure ENST registers for all queues

[PATCH net v2] net: macb: configure ENST registers for all queues

[Vineeth Karumanchi]

When a taprio config only covered a subset of queues, the driver
programmed the ENST registers only for the queues named in the config
and left the remaining queues holding stale register values. This
produced an inconsistent hardware setup that affected the scheduling
of the configured queues.

This was observed on a GEM instance with four hardware queues, all
enabled:

Initial configuration:
- All four queues are enabled.
- enst_on_time_qX registers are left at their reset value (0x0001FFFF).
- Only q0 and q1 are configured with valid, non-overlapping ENST
  schedules (T0 and T1 respectively).
- Traffic streams p0 and p1 are bound to q0 and q1.
- ENST is enabled only on q0 and q1.

Observed behavior:
- During T0 on-time, both p0 and p1 packets are transmitted.
- During T1 on-time, both p0 and p1 packets are transmitted.

With the unused queues (q2 and q3) explicitly programmed with
enst_on_time = 0x0:
- During T0 on-time, only p0 packets are transmitted.
- During T1 on-time, only p1 packets are transmitted.

Leaving the ENST on-time registers of unused queues at their reset
value (0x0001FFFF) disrupts the scheduling of the configured queues,
whereas programming them with 0x0 yields the expected ENST operation.

Program the ENST registers for every queue unconditionally. The
per-queue configuration array is now allocated for bp->num_queues and
indexed directly by queue_id; unconfigured queues are left
zero-initialized by kcalloc(), so their registers are cleared.
Indexing the array by queue_id also makes the queue_id field in
struct macb_queue_enst_config redundant, so drop it.

Fixes: 89934dbf169e ("net: macb: Add TAPRIO traffic scheduling support")
Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
Changes in v2:
- Split the patches for net and net-next.
- Updated commit message
- Link to v1 : https://lore.kernel.org/netdev/20260724043257.2221030-1-vineeth.karumanchi@amd.com/
---
 drivers/net/ethernet/cadence/macb.h      |  2 --
 drivers/net/ethernet/cadence/macb_main.c | 20 +++++++++-----------
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 2de56017ee0d..0ee857c985fa 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -1496,7 +1496,6 @@ struct macb_platform_data {
  * @start_time_mask:  Bitmask representing the start time for the queue
  * @on_time_bytes:    "on" time nsec expressed in bytes
  * @off_time_bytes:   "off" time nsec expressed in bytes
- * @queue_id:         Identifier for the queue
  *
  * This structure holds the configuration parameters for an ENST queue,
  * used to control time-based transmission scheduling in the MACB driver.
@@ -1505,7 +1504,6 @@ struct macb_queue_enst_config {
 	u32 start_time_mask;
 	u32 on_time_bytes;
 	u32 off_time_bytes;
-	u8 queue_id;
 };
 
 #endif /* _MACB_H */
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index d394f1f43b68..d58430fe9c41 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4329,7 +4329,7 @@ static int macb_taprio_setup_replace(struct net_device *ndev,
 	struct macb_queue *queue;
 	u32 queue_mask;
 	u8 queue_id;
-	size_t i;
+	size_t i, q;
 	int err;
 
 	if (conf->num_entries > bp->num_queues) {
@@ -4357,7 +4357,7 @@ static int macb_taprio_setup_replace(struct net_device *ndev,
 		return -EINVAL;
 	}
 
-	enst_queue = kcalloc(conf->num_entries, sizeof(*enst_queue), GFP_KERNEL);
+	enst_queue = kcalloc(bp->num_queues, sizeof(*enst_queue), GFP_KERNEL);
 	if (unlikely(!enst_queue))
 		return -ENOMEM;
 
@@ -4416,13 +4416,12 @@ static int macb_taprio_setup_replace(struct net_device *ndev,
 			goto cleanup;
 		}
 
-		enst_queue[i].queue_id = queue_id;
-		enst_queue[i].start_time_mask =
+		enst_queue[queue_id].start_time_mask =
 			(start_time_sec << GEM_START_TIME_SEC_OFFSET) |
 			start_time_nsec;
-		enst_queue[i].on_time_bytes =
+		enst_queue[queue_id].on_time_bytes =
 			enst_ns_to_hw_units(entry->interval, speed);
-		enst_queue[i].off_time_bytes =
+		enst_queue[queue_id].off_time_bytes =
 			enst_ns_to_hw_units(conf->cycle_time - entry->interval, speed);
 
 		configured_queues |= entry->gate_mask;
@@ -4448,15 +4447,14 @@ static int macb_taprio_setup_replace(struct net_device *ndev,
 		gem_writel(bp, ENST_CONTROL,
 			   queue_mask << GEM_ENST_DISABLE_QUEUE_OFFSET);
 
-		for (i = 0; i < conf->num_entries; i++) {
-			queue = &bp->queues[enst_queue[i].queue_id];
+		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
 			/* Configure queue timing registers */
 			queue_writel(queue, ENST_START_TIME,
-				     enst_queue[i].start_time_mask);
+				     enst_queue[q].start_time_mask);
 			queue_writel(queue, ENST_ON_TIME,
-				     enst_queue[i].on_time_bytes);
+				     enst_queue[q].on_time_bytes);
 			queue_writel(queue, ENST_OFF_TIME,
-				     enst_queue[i].off_time_bytes);
+				     enst_queue[q].off_time_bytes);
 		}
 
 		/* Enable ENST for all configured queues in one write */
-- 
2.44.4

Re: [PATCH net v2] net: macb: configure ENST registers for all queues

[Théo Lebrun]

Hello Vineeth,

On Wed Aug 5, 2026 at 6:42 AM CEST, Vineeth Karumanchi wrote:
> When a taprio config only covered a subset of queues, the driver
> programmed the ENST registers only for the queues named in the config
> and left the remaining queues holding stale register values. This
> produced an inconsistent hardware setup that affected the scheduling
> of the configured queues.
>
> This was observed on a GEM instance with four hardware queues, all
> enabled:
>
> Initial configuration:
> - All four queues are enabled.
> - enst_on_time_qX registers are left at their reset value (0x0001FFFF).
> - Only q0 and q1 are configured with valid, non-overlapping ENST
>   schedules (T0 and T1 respectively).
> - Traffic streams p0 and p1 are bound to q0 and q1.
> - ENST is enabled only on q0 and q1.
>
> Observed behavior:
> - During T0 on-time, both p0 and p1 packets are transmitted.
> - During T1 on-time, both p0 and p1 packets are transmitted.
>
> With the unused queues (q2 and q3) explicitly programmed with
> enst_on_time = 0x0:
> - During T0 on-time, only p0 packets are transmitted.
> - During T1 on-time, only p1 packets are transmitted.

Thanks for the expanded commit message.

> Leaving the ENST on-time registers of unused queues at their reset
> value (0x0001FFFF) disrupts the scheduling of the configured queues,
> whereas programming them with 0x0 yields the expected ENST operation.
>
> Program the ENST registers for every queue unconditionally. The
> per-queue configuration array is now allocated for bp->num_queues and
> indexed directly by queue_id; unconfigured queues are left
> zero-initialized by kcalloc(), so their registers are cleared.
> Indexing the array by queue_id also makes the queue_id field in
> struct macb_queue_enst_config redundant, so drop it.
>
> Fixes: 89934dbf169e ("net: macb: Add TAPRIO traffic scheduling support")
> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
> ---
> Changes in v2:
> - Split the patches for net and net-next.
> - Updated commit message
> - Link to v1 : https://lore.kernel.org/netdev/20260724043257.2221030-1-vineeth.karumanchi@amd.com/
> ---
>
> [...]
>
> @@ -4357,7 +4357,7 @@ static int macb_taprio_setup_replace(struct net_device *ndev,
>  		return -EINVAL;
>  	}
>  
> -	enst_queue = kcalloc(conf->num_entries, sizeof(*enst_queue), GFP_KERNEL);
> +	enst_queue = kcalloc(bp->num_queues, sizeof(*enst_queue), GFP_KERNEL);
>  	if (unlikely(!enst_queue))
>  		return -ENOMEM;

My first reaction to this was that we should be using the new
kzalloc_objs() API. But actually those 96 bytes are not worth the
trouble of a kmalloc, it could be stack allocated.

   struct macb_queue_enst_config enst_config[MACB_MAX_QUEUES] = {};

Anyway this is a bit orthogonal to your change.
Whether you change it or not:

Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net v2] net: macb: configure ENST registers for all queues

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email says "What's the expectation for the other queues?"
---
net: macb: configure ENST registers for all queues

The per-queue ENST config array is now sized to bp->num_queues and indexed
by queue_id, and the hardware programming loop writes ENST_START_TIME,
ENST_ON_TIME and ENST_OFF_TIME for every queue instead of only the queues
named in the taprio config. Queues absent from the config get the
kcalloc()-zeroed slot, so their timing registers are written as 0.

Link: https://lore.kernel.org/netdev/20260724043257.2221030-1-vineeth.karumanchi@amd.com/

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index d394f1f43b685..d58430fe9c41e 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -4357,7 +4357,7 @@ static int macb_taprio_setup_replace(struct net_device *ndev,
>  		return -EINVAL;
>  	}
>  
> -	enst_queue = kcalloc(conf->num_entries, sizeof(*enst_queue), GFP_KERNEL);
> +	enst_queue = kcalloc(bp->num_queues, sizeof(*enst_queue), GFP_KERNEL);
>  	if (unlikely(!enst_queue))
>  		return -ENOMEM;
>  

[ ... ]

> @@ -4448,15 +4447,14 @@ static int macb_taprio_setup_replace(struct net_device *ndev,
>  		gem_writel(bp, ENST_CONTROL,
>  			   queue_mask << GEM_ENST_DISABLE_QUEUE_OFFSET);
>  
> -		for (i = 0; i < conf->num_entries; i++) {
> -			queue = &bp->queues[enst_queue[i].queue_id];
> +		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>  			/* Configure queue timing registers */
>  			queue_writel(queue, ENST_START_TIME,
> -				     enst_queue[i].start_time_mask);
> +				     enst_queue[q].start_time_mask);
>  			queue_writel(queue, ENST_ON_TIME,
> -				     enst_queue[i].on_time_bytes);
> +				     enst_queue[q].on_time_bytes);
>  			queue_writel(queue, ENST_OFF_TIME,
> -				     enst_queue[i].off_time_bytes);
> +				     enst_queue[q].off_time_bytes);
>  		}
>  
>  		/* Enable ENST for all configured queues in one write */

[Severity: Medium]
Can transmission on the queues left out of the taprio config stall after
this change?

For a queue that does not appear in conf->entries, enst_queue[q] is the
kcalloc()-zeroed slot, so macb_taprio_setup_replace() now writes
ENST_ON_TIME = 0 for it, and the following write only arms the configured
ones:

	gem_writel(bp, ENST_CONTROL, configured_queues);

The commit message argues the change is needed because the stale on-time of
the unconfigured queues altered the gating of q0/q1:

    Leaving the ENST on-time registers of unused queues at their reset
    value (0x0001FFFF) disrupts the scheduling of the configured queues,
    whereas programming them with 0x0 yields the expected ENST operation.

If the hardware consults ENST_ON_TIME of a queue whose ENST enable bit in
ENST_CONTROL is clear, then on_time = 0 in the 17-bit GEM_ON_TIME field is
not obviously a neutral "cleared" value; it could read as an on-window that
never opens.

The reported measurements only cover p0 and p1 on the two configured queues:

    With the unused queues (q2 and q3) explicitly programmed with
    enst_on_time = 0x0:
    - During T0 on-time, only p0 packets are transmitted.
    - During T1 on-time, only p1 packets are transmitted.

Was traffic on q2/q3 checked in that same experiment? All bp->num_queues
queues are real netdev TX queues:

drivers/net/ethernet/cadence/macb_main.c:macb_probe() {
	...
	dev = alloc_etherdev_mq(sizeof(*bp), num_queues);
	...
}

so ordinary traffic is hashed onto q2/q3 while the taprio qdisc is
installed.

macb_taprio_destroy() writes the same zeros, but there ENST_CONTROL holds
the disable mask for every queue and nothing is re-enabled afterwards:

drivers/net/ethernet/cadence/macb_main.c:macb_taprio_destroy() {
	...
		gem_writel(bp, ENST_CONTROL,
			   queue_mask << GEM_ENST_DISABLE_QUEUE_OFFSET);

		/* Clear all queue ENST registers in batch */
		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
			queue_writel(queue, ENST_START_TIME, 0);
	...
}

That path leaves the engine globally quiescent, so it does not show that
all-zero timing registers are inert for a non-enabled queue while ENST is
running for other queues.

Since this carries a Fixes: tag and is stable-bound, could the commit
message state whether the non-scheduled queues still transmit normally, or
cite the hardware rule that makes on_time = 0 inert for a queue whose ENST
enable bit is clear? If the timing registers of disabled queues are in fact
not honoured, the stated mechanism behind the fix would need reworking too.