linux-kernel - Re: [PATCH v3 4/4] 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] [day] [month] [year] [list]

Message-ID: <20250407114239.193cd217@wsk>
Date: Mon, 7 Apr 2025 11:42:39 +0200
From: Lukasz Majewski <lukma@...x.de>
To: Andrew Lunn <andrew@...n.ch>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, davem@...emloft.net, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
 <pabeni@...hat.com>, 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
Subject: Re: [PATCH v3 4/4] net: mtip: The L2 switch driver for imx287

Hi Andrew,

> > +struct switch_enet_private *mtip_netdev_get_priv(const struct
> > net_device *ndev) +{
> > +	if (ndev->netdev_ops == &mtip_netdev_ops)
> > +		return netdev_priv(ndev);
> > +
> > +	return NULL;
> > +}  
> 
> > +static bool mtip_port_dev_check(const struct net_device *ndev)
> > +{
> > +	if (!mtip_netdev_get_priv(ndev))
> > +		return false;
> > +
> > +	return true;
> > +}
> > +  
> 
> Rearranging the code a bit to make my point....
> 
> mtip_port_dev_check() tells us if this ndev is one of the ports of
> this switch.
> 
> > +/* netdev notifier */
> > +static int mtip_netdevice_event(struct notifier_block *unused,
> > +				unsigned long event, void *ptr)
> > +{
> > +	struct net_device *ndev = netdev_notifier_info_to_dev(ptr);
> > +	struct netdev_notifier_changeupper_info *info;
> > +	int ret = NOTIFY_DONE;
> > +
> > +	if (!mtip_port_dev_check(ndev))
> > +		return NOTIFY_DONE;  
> 
> We have received a notification about some interface. This filters out
> all but the switches interfaces.
> 
> > +
> > +	switch (event) {
> > +	case NETDEV_CHANGEUPPER:
> > +		info = ptr;  
> 
> CHANGERUPPER is that a netdev has been added or removed from a bridge,
> or some other sort of master device, e.g. a bond.
> 
> > +
> > +		if (netif_is_bridge_master(info->upper_dev)) {
> > +			if (info->linking)
> > +				ret = mtip_ndev_port_link(ndev,
> > +
> > info->upper_dev);  
> 
> Call mtip_ndev_port_link() has been added to some bridge.
> 
> > +static int mtip_ndev_port_link(struct net_device *ndev,
> > +			       struct net_device *br_ndev)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(ndev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +
> > +	dev_dbg(&ndev->dev, "%s: ndev: %s br: %s fep: 0x%x\n",
> > +		__func__, ndev->name,  br_ndev->name, (unsigned
> > int)fep); +
> > +	/* Check if MTIP switch is already enabled */
> > +	if (!fep->br_offload) {
> > +		if (!priv->master_dev)
> > +			priv->master_dev = br_ndev;
> > +
> > +		fep->br_offload = 1;
> > +		mtip_switch_dis_port_separation(fep);
> > +		mtip_clear_atable(fep);
> > +	}  
> 
> So lets consider
> 
> ip link add br0 type bridge
> ip link add br1 type bridge
> ip link set dev lan1 master br0
> 
> We create two bridges, and add the first port to one of the bridges.
> 
> fep->br_offload should be False
> priv->master_dev should be NULL.
> 
> So fep->br_offload is set to 1, priv->master_dev is set to br0 and the
> separation between the ports is removed.
> 
> It seems like the hardware will now be bridging packets between the
> two interfaces, despite lan2 not being a member of any bridge....
> 
> Now
> 
> ip link set dev lan2 master br1
> 
> I make the second port a member of some other bridge. fep->br_offload
> is True, so nothing happens.
> 
> This is why i said this code needs expanding.
> 
> If you look at other switch drivers, you will see each port keeps
> track of what bridge it has been joined to. There is then logic which
> iterates over the ports, finds which ports are members of the same
> bridge, and enables packets to flow between those ports.
> 
> With only two ports, you can make some simplifications, but you should
> only disable the separation once both ports are the member of the same
> bridge.
> 

I think that I do have your point. Thanks for the info.

> 	Andrew




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