netdev - 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20159d14-7d6b-4c16-9f00-ae993cc16f90@redhat.com>
Date: Wed, 25 Jun 2025 09:13:02 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Lukasz Majewski <lukma@...x.de>
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

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.

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

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

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