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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250624230437.1ede2bcb@wsk>
Date: Tue, 24 Jun 2025 23:04:37 +0200
From: Lukasz Majewski <lukma@...x.de>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, davem@...emloft.net, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Rob Herring
 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>, Sascha Hauer
 <s.hauer@...gutronix.de>, Pengutronix Kernel Team <kernel@...gutronix.de>,
 Fabio Estevam <festevam@...il.com>, Richard Cochran
 <richardcochran@...il.com>, netdev@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org, Stefan Wahren
 <wahrenst@....net>, Simon Horman <horms@...nel.org>, Andrew Lunn
 <andrew@...n.ch>
Subject: Re: [net-next v13 04/11] net: mtip: The L2 switch driver for imx287

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@...x.de

Content of type "application/pgp-signature" skipped