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

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

On 29.07.2025 11:59, Karumanchi, Vineeth wrote:
> 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.

I was asking as it looked to me that this validates the number of queues
the IP supports, which should have already been validated in probe.

> 
> 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.

I can't see how. That should translate into:

if (1 < 1 || 1 > 8)

which is in the end:

if (0)

Maybe it fails due to some other condition?

> 
> <...>
> 
>>> +    /* 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.

Please check the documentation pointed by Andrew. With that, my suggestion
here should be dropped.

Thank you,
Claudiu

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