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

[Andrew Lunn <andrew@lunn.ch>]

> +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.

	Andrew