Re: [PATCH net-next 3/6] net: macb: Add IEEE 802.1Qbv TAPRIO REPLACE command offload support

["Karumanchi, Vineeth" <vineeth@amd.com>]

Hi Claudiu,

Thanks for the review.

On 7/26/2025 5:55 PM, claudiu beznea (tuxon) wrote:
> 
> 
> On 7/22/25 18:41, Vineeth Karumanchi wrote:
>> Implement Time-Aware Traffic Scheduling (TAPRIO) hardware offload for

<..>

> 
> as it is used along with conf->num_entries which has size_t type.
> 
>> +
>> +    /* Validate queue configuration */
>> +    if (bp->num_queues < 1 || bp->num_queues > MACB_MAX_QUEUES) {
> 
> Can this happen?

Yes, GEM in Zynq devices has single queue.

Currently, TAPRIO offload validation depends solely on the presence of 
.ndo_setup_tc. On Zynq-based devices, if the user configures the 
scheduler using tc replace, the operation fails at this point.

<...>

>> +    /* All validations passed - proceed with hardware configuration */
>> +    spin_lock_irqsave(&bp->lock, flags);
> 
> You can use guard(spinlock_irqsave)(&bp->lock) or 
> scoped_guard(spinlock_irqsave, &bp->lock)
>

ok, will leverage scoped_guard(spinlock_irqsave, &bp->lock).

>> +
>> +    /* Disable ENST queues if running before configuring */
>> +    if (gem_readl(bp, ENST_CONTROL))
> 
> Is this read necessary?
> 

Not necessary, I thought of disabling only if enabled.
But, will disable directly.

>> +        gem_writel(bp, ENST_CONTROL,
>> +               GENMASK(bp->num_queues - 1, 0) << 
>> GEM_ENST_DISABLE_QUEUE_OFFSET);
> 
> This could be replaced by GEM_BF(GENMASK(...), ENST_DISABLE_QUEUE) if 
> you define GEM_ENST_DISABLE_QUEUE_SIZE along with 
> GEM_ENST_DISABLE_QUEUE_OFFSET.
> 

I can leverage bp->queue_mask << GEM_ENST_DISABLE_QUEUE_OFFSET.
And remove GEM_ENST_ENABLE_QUEUE(hw_q) and GEM_ENST_DISABLE_QUEUE(hw_q) 
implementations.

>> +
>> +    for (i = 0; i < conf->num_entries; i++) {
>> +        queue = &bp->queues[enst_queue[i].queue_id];
>> +        /* Configure queue timing registers */
>> +        queue_writel(queue, ENST_START_TIME, 
>> enst_queue[i].start_time_mask);
>> +        queue_writel(queue, ENST_ON_TIME, enst_queue[i].on_time_bytes);
>> +        queue_writel(queue, ENST_OFF_TIME, 
>> enst_queue[i].off_time_bytes);
>> +    }
>> +
>> +    /* Enable ENST for all configured queues in one write */
>> +    gem_writel(bp, ENST_CONTROL, configured_queues);
> 
> Can this function be executed while other queues are configured? If so, 
> would the configured_queues contains it (as well as conf)?
> 

No, the tc add/replace command re-configures all queues, replacing any 
previous setup. Parameters such as START_TIME, ON_TIME, and CYCLE_TIME 
are recalculated based on the new configuration.

>> +    spin_unlock_irqrestore(&bp->lock, flags);
>> +
>> +    netdev_info(ndev, "TAPRIO configuration completed successfully: 
>> %lu entries, %d queues configured\n",
>> +            conf->num_entries, hweight32(configured_queues));
>> +
>> +cleanup:
>> +    kfree(enst_queue);
> 
> With the suggestions above, this could be dropped.
> 

ok.


> Thank you,
> Claudiu
> 
>> +    return err;
>> +}
>> +
>>   static const struct net_device_ops macb_netdev_ops = {
>>       .ndo_open        = macb_open,
>>       .ndo_stop        = macb_close,
> 

Thanks
-- 
🙏 vineeth