linux-kernel - Re: [net-next v14 04/12] 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: <20250710230413.3b6798f7@wsk>
Date: Thu, 10 Jul 2025 23:04:13 +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 v14 04/12] net: mtip: The L2 switch driver for imx287

Hi Paolo,

> Hi Paolo,
> 
> > On 7/1/25 1:49 PM, Lukasz Majewski wrote:  
> > > Changes for v14:
> > > - Increase the maximal received frame size to 1536 (for VLAN)
> > > - Use spin_{un}lock_irq{save|restore} when altering dynamic table
> > > of the switch and mtip_adjust_link() as both cannot be done when
> > > switch IRQ is potentially enabled    
> > 
> > Why?
> > 
> >  (the previous one alters entries in switching table  
> > >   the latter one may reset the whole IP block)    
> > 
> > What really matters is the scope (process/atomic, bh, hardirq) of
> > the relevant callers (the functions that do acquire the given
> > locks). 
> 
> Maybe I will explain the problem here case (function) by case:
> - mtip_adjust_link()
>   This function is called when link change is detected (speed, duplex,
>   up/down link).
> 
>   The problem here is that:
> 	1. It is called for both MTIP ports (as both are managed by
> 	this driver)
> 
> 	2. NXP's "legacy" driver advises reset of the whole IP block
> 	when such change is detected. 
> 
> 	Considering the above - interrupts shall be disabled as we may
> 	end up in undefined state of the IP block - especially that
> 	re-configuration of switch requires interrupts initialization.
> 
> 
> - mtip_atable_dynamicms_learn_migration() - update of the switching
>   table
> 
> 	Can be called from:
> 	1. function triggered when timer fires (once per 100ms)
> 
> 	2. mtip_switch_rx() which is called from mtip_rx_napi()
> callback (which is protected by net core).
> 
> 	It looks like the _irqsave/_irqrestore is an overkill here.
> 	Both above contexts seems to not require IRQs disabled. I can
> 	confirm that use of plain spin_{un}lock() functions works.
> 
> >   
> > > +/* dynamicms MAC address table learn and migration */
> > > +static void
> > > +mtip_atable_dynamicms_learn_migration(struct switch_enet_private
> > > *fep,
> > > +				      int curr_time, unsigned
> > > char *mac,
> > > +				      u8 *rx_port)
> > > +{
> > > +	u8 port = MTIP_PORT_FORWARDING_INIT;
> > > +	struct mtip_port_info *port_info;
> > > +	u32 rx_mac_lo = 0, rx_mac_hi = 0;
> > > +	unsigned long flags;
> > > +	int index;
> > > +
> > > +	spin_lock_irqsave(&fep->learn_lock, flags);    
> > 
> > If the _irqsave() part is needed (and I don't see why??!) than all
> > the other `learn_lock` users should also use such variant, unless
> > already in hardirq scope.
> > 
> > [...]  
> > > +static void mtip_adjust_link(struct net_device *dev)
> > > +{
> > > +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> > > +	struct switch_enet_private *fep = priv->fep;
> > > +	struct phy_device *phy_dev;
> > > +	int status_change = 0, idx;
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&fep->hw_lock, flags);    
> > 
> > Same here.  
> 
> Please see the explanation above.
> 

Is the explanation enough (and from other e-mails), so I can proceed
with next version of this patch set?

> > 
> > /P
> >   
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH, Managing Director: Johanna Denk,
> Tabea Lutz HRB 165235 Munich, Office: Kirchenstr.5, D-82194
> Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> lukma@...x.de




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH, Managing Director: Johanna Denk,
Tabea Lutz 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