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

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.

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

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