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

[Lukasz Majewski <lukma@denx.de>]

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

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

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