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

[Lukasz Majewski <lukma@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 3287 bytes --]

Hi Paolo,

> On 6/22/25 11:37 AM, Lukasz Majewski wrote:
> > +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.

However, I've rewritten relevant portions where GENMASK() could be used
to simplify and make the code more readable.

> 
> [...]
> > +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).

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

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

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

I've already excluded bridge and mgnt patches. Also moved out some code
from this one.

> 
> /P
> 


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]