Re: [PATCH v3 4/4] net: mtip: The L2 switch driver for imx287

[Lukasz Majewski]

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

Attachment: pgpx3MFhZZOWI.pgp
Description: OpenPGP digital signature