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

[Karumanchi, Vineeth]

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