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

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 <[email protected]>
> ---
> Changes in v2:
> - Split the patches for net and net-next.
> - Updated commit message
> - Link to v1 : https://lore.kernel.org/netdev/[email protected]/
> ---
>
> [...]
>
> @@ -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 <[email protected]>

Thanks,

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