Thread (23 messages) 23 messages, 2 authors, 2025-06-30

Re: [net-next v13 04/11] net: mtip: The L2 switch driver for imx287

From: Paolo Abeni <pabeni@redhat.com>
Date: 2025-06-25 07:13:16
Also in: imx, linux-arm-kernel, linux-devicetree, lkml

On 6/24/25 11:04 PM, Lukasz Majewski wrote:
quoted
On 6/22/25 11:37 AM, Lukasz Majewski wrote:
quoted
+static void mtip_aging_timer(struct timer_list *t)
+{
+	struct switch_enet_private *fep = timer_container_of(fep,
t,
+
timer_aging); +
+	fep->curr_time = mtip_timeincrement(fep->curr_time);
+
+	mod_timer(&fep->timer_aging,
+		  jiffies +
msecs_to_jiffies(LEARNING_AGING_INTERVAL)); +}  
It's unclear to me why you decided to maintain this function and timer
while you could/should have used a macro around jiffies instead.
This is a bit more tricky than just getting value from jiffies.

The current code provides a monotonic, starting from 0 time "base" for
learning and managing entries in internal routing tables for MTIP.

To be more specific - the fep->curr_time is a value incremented after
each ~10ms.

Simple masking of jiffies would not provide such features.
I guess you can get the same effect storing computing the difference
from an initial jiffies value and using jiffies_to_msecs(<delta>)/10.
quoted
[...]
quoted
+static int mtip_sw_learning(void *arg)
+{
+	struct switch_enet_private *fep = arg;
+
+	while (!kthread_should_stop()) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		/* check learning record valid */
+		mtip_atable_dynamicms_learn_migration(fep,
fep->curr_time,
+						      NULL, NULL);
+		schedule_timeout(HZ / 100);
+	}
+
+	return 0;
+}  
Why are you using a full blown kernel thread here? 
The MTIP IP block requires the thread for learning. It is a HW based
switching accelerator, but the learning feature must be performed by
SW (by writing values to its registers).
quoted
Here a timer could
possibly make more sense.
Unfortunately, not - the code (in
mtip_atable_dynamicms_learn_migration() must be called). This function
has another role - it updates internal routing table with timestamps
(provided by timer mentioned above).
Why a periodic timer can't call such function?
quoted
Why are checking the table every 10ms, while
the learning intervall is 100ms? 
Yes, this is correct. In 10ms interval the internal routing table is
updated. 100 ms is for learning.
quoted
I guess you could/should align the
frequency here with such interval.
IMHO learning with 10ms interval would bring a lot of overhead.

Just to mention - the MTIP IP block can generate interrupt for
learning event. However, it has been advised (bu NXP support), that a
thread with 100ms interval shall be used to avoid too many interrupts.
FTR, my suggestion is to increase the
mtip_atable_dynamicms_learn_migration's call period to 100ms
quoted
Side note: I think you should move the buffer management to a later
patch: this one is still IMHO too big.
And this is problematic - the most time I've spent for v13 to separate
the code - i.e. I exclude one function, then there are warnings that
other function is unused (and of course WARNINGS in a separate patches
are a legitimate reason to call for another patch set revision).
A trick to break that kind of dependencies chain is to leave a function
implementation empty.

On the same topic, you could have left mtip_rx_napi() implementation
empty up to patch 6 or you could have introduced napi initialization and
cleanup only after such patch.

In a similar way, you could introduce buffer managements in a later
patch and add the relevant calls afterwards.

/P
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help