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

[Jakub Kicinski <kuba@kernel.org>]

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.