Re: [PATCH net-next 3/3] net: lan966x: Extend switchdev with mdb support

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Mon, Jan 03, 2022 at 05:22:44PM +0100, Horatiu Vultur wrote:
> > > +static int lan966x_mdb_ip_del(struct lan966x_port *port,
> > > +                           const struct switchdev_obj_port_mdb *mdb,
> > > +                           enum macaccess_entry_type type)
> > > +{
> > > +     bool cpu_port = netif_is_bridge_master(mdb->obj.orig_dev);
> > > +     struct lan966x *lan966x = port->lan966x;
> > > +     struct lan966x_mdb_entry *mdb_entry;
> > > +     unsigned char mac[ETH_ALEN];
> > > +
> > > +     mdb_entry = lan966x_mdb_entry_get(lan966x, mdb->addr, mdb->vid);
> > > +     if (!mdb_entry) {
> > > +             /* If the CPU originted this and the entry was not found, it is
> >
> > s/originted/originated/
> >
> > > +              * because already another port has removed the entry
> > > +              */
> > > +             if (cpu_port)
> > > +                     return 0;
> >
> > Could you explain again why this is normal? If another port has removed
> > the entry, how is the address copied to the CPU any longer?
>
> The cpu_port is set only when the entry is deleted by the bridge. So then
> all the ports under the bridge will be notified about this. So when the first
> port is notified it would delete the entry and then when the second port is
> notified, it would try to delete the entry which is already deleted by
> the first port. That is the reason why it returns 0 and not an error.


> > The HOST_MDB switchdev events are replicated per the number of ports in
> > the bridge.
>
> Correct.
>
> > So I would expect that you keep refcounts on them, otherwise
> > the first deletion of such an element would trigger the removal of the
> > entry from hardware even though it's still in use.
>
> Sorry, I am not sure I am following, by whom is it still in use?
>
> I was trying the following commands:
>
> ip link set dev eth0 master br0
> ip link set dev eth1 master br0
> bridge mdb add dev br0 port br0 grp 225.1.2.3
>
> Then both ports eth0 and eth1 will get a notification about this. And
> they will add an entry in the MAC table for this.
> Then when the following command is run:
>
> bridge mdb del dev br0 port br0 grp 225.1.2.3
>
> Then again both ports will get a notification about this and eth0 will
> delete the entry from the MAC table. So why is this not correct? Then
> will be eth1 who will get the notification and try to delete the entry
> which is already deleted.

root@debian:~# ip link set swp0 up
[   42.938394] fsl_enetc 0000:00:00.2 eno2: configuring for fixed/internal link mode
[   42.949675] fsl_enetc 0000:00:00.2 eno2: Link is Up - 2.5Gbps/Full - flow control rx/tx
[   42.959044] mscc_felix 0000:00:00.5 swp0: configuring for inband/qsgmii link mode
root@debian:~# ip link set swp1 up
[   44.440675] mscc_felix 0000:00:00.5 swp1: configuring for inband/qsgmii link mode
root@debian:~# ip link set swp2 up
[   45.788811] mscc_felix 0000:00:00.5 swp2: configuring for inband/qsgmii link mode
root@debian:~# ip link set swp3 up
[   47.784439] mscc_felix 0000:00:00.5 swp3: configuring for inband/qsgmii link mode
[   51.887374] mscc_felix 0000:00:00.5 swp3: Link is Up - 1Gbps/Full - flow control rx/tx
[   51.895491] IPv6: ADDRCONF(NETDEV_CHANGE): swp3: link becomes ready
[  100.303070] mscc_felix 0000:00:00.5 swp0: Link is Up - 1Gbps/Full - flow control off
[  100.311016] IPv6: ADDRCONF(NETDEV_CHANGE): swp0: link becomes ready
root@debian:~# ip link add br0 type bridge
root@debian:~# ip link set swp0 master br0
[  115.436652] br0: port 1(swp0) entered blocking state
[  115.441736] br0: port 1(swp0) entered disabled state
[  115.447618] device swp0 entered promiscuous mode
root@debian:~# ip link set swp1 master br0
[  117.760700] br0: port 2(swp1) entered blocking state
[  117.765797] br0: port 2(swp1) entered disabled state
[  117.771335] device swp1 entered promiscuous mode
root@debian:~# ip link set swp2 master br0
[  119.612665] br0: port 3(swp2) entered blocking state
[  119.617750] br0: port 3(swp2) entered disabled state
[  119.623304] device swp2 entered promiscuous mode
root@debian:~# ip link set swp3 master br0
[  121.388388] br0: port 4(swp3) entered blocking state
[  121.393416] br0: port 4(swp3) entered disabled state
[  121.399292] device swp3 entered promiscuous mode
root@debian:~# [  148.905819] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 0 addr 33:33:ff:9e:9e:96 vid 0
[  148.914819] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 1 addr 33:33:ff:9e:9e:96 vid 0
[  148.923723] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 2 addr 33:33:ff:9e:9e:96 vid 0
[  148.932606] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 3 addr 33:33:ff:9e:9e:96 vid 0
[  148.941426] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 0 addr 33:33:00:00:00:6a vid 0
[  148.950351] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 1 addr 33:33:00:00:00:6a vid 0
[  148.959164] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 2 addr 33:33:00:00:00:6a vid 0
[  148.967967] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 3 addr 33:33:00:00:00:6a vid 0
[  150.649744] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 0 addr 33:33:00:00:00:fb vid 0
[  150.658674] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 1 addr 33:33:00:00:00:fb vid 0
[  150.667538] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 2 addr 33:33:00:00:00:fb vid 0
[  150.676389] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 3 addr 33:33:00:00:00:fb vid 0
root@debian:~# ip link set br0 up
[  148.868268] br0: port 4(swp3) entered blocking state
[  148.873283] br0: port 4(swp3) entered forwarding state
[  148.878530] br0: port 1(swp0) entered blocking state
[  148.883546] br0: port 1(swp0) entered forwarding state
[  148.889443] IPv6: ADDRCONF(NETDEV_CHANGE): br0: link becomes ready
[  148.905819] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 0 addr 33:33:ff:9e:9e:96 vid 0
[  148.914819] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 1 addr 33:33:ff:9e:9e:96 vid 0
[  148.923723] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 2 addr 33:33:ff:9e:9e:96 vid 0
[  148.932606] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 3 addr 33:33:ff:9e:9e:96 vid 0
[  148.941426] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 0 addr 33:33:00:00:00:6a vid 0
[  148.950351] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 1 addr 33:33:00:00:00:6a vid 0
[  148.959164] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 2 addr 33:33:00:00:00:6a vid 0
[  148.967967] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 3 addr 33:33:00:00:00:6a vid 0
[  150.649744] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 0 addr 33:33:00:00:00:fb vid 0
[  150.658674] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 1 addr 33:33:00:00:00:fb vid 0
[  150.667538] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 2 addr 33:33:00:00:00:fb vid 0
[  150.676389] mscc_felix 0000:00:00.5: dsa_port_host_mdb_add: port 3 addr 33:33:00:00:00:fb vid 0
root@debian:~# ip link set swp3 nomaster
[  160.416054] device swp3 left promiscuous mode
[  160.420745] br0: port 4(swp3) entered disabled state
[  160.445427] mscc_felix 0000:00:00.5: dsa_port_host_mdb_del: port 3 addr 33:33:00:00:00:fb vid 0
[  160.454201] mscc_felix 0000:00:00.5: dsa_port_host_mdb_del: port 3 addr 33:33:00:00:00:6a vid 0
[  160.464843] mscc_felix 0000:00:00.5: dsa_port_host_mdb_del: port 3 addr 33:33:ff:9e:9e:96 vid 0

Hope it's a bit clearer now. swp0, swp1 and swp2 are still under br0.