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: 4986 bytes --]

Hi Paolo,

> On 6/24/25 11:04 PM, Lukasz Majewski wrote:
> >> 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.  
> 
> I guess you can get the same effect storing computing the difference
> from an initial jiffies value and using jiffies_to_msecs(<delta>)/10.

With some coding assuring only 10 bit width of the resulting clock
(based on jiffies) I can have a monotonic clock which will not start
from 0.

> 
> >> [...]  
> >>> +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 a periodic timer can't call such function?

Yes, the kthread can be replaced with timer with 100ms period.

Just to explain - the mtip_atable_dynamicms_learn_migration(), which
requires monotonic value incremented once per 10ms, is called at two
places:

1. mtip_switch_rx() -> the dynamic table is examined if required (i.e.
new frame arrives). In this place the counter requires 10ms resolution
(can be extracted from jiffies).

2. The mtip_sw_learning() - which now is run from kthread, but it can
be replaced with timer (100ms resolution).

> 
> >   
> >> 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.  
> 
> FTR, my suggestion is to increase the
> mtip_atable_dynamicms_learn_migration's call period to 100ms

As mentioned above - it is called in two places. One is in kthread
started at 100ms period, another one is asynchronous when frame arrives.

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

I get your point.

> 
> /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 --]