[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <61d545dd-c32c-4cc3-94fb-53954eee365b@tuxon.dev>
Date: Thu, 31 Jul 2025 12:07:15 +0300
From: Claudiu Beznea <claudiu.beznea@...on.dev>
To: "Karumanchi, Vineeth" <vineeth@....com>, vineeth.karumanchi@....com,
nicolas.ferre@...rochip.com, andrew+netdev@...n.ch, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com
Cc: git@....com, netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 3/6] net: macb: Add IEEE 802.1Qbv TAPRIO REPLACE
command offload support
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
Powered by blists - more mailing lists