Re: [PATCH net-next] net: ethernet: ti: cpsw: don't flush mcast entries while switch promisc mode

[Grygorii Strashko]

On 10/19/18 7:04 AM, Ivan Khoronzhuk wrote:
> On Thu, Oct 18, 2018 at 07:03:06PM -0500, Grygorii Strashko wrote:
> > On 10/18/18 1:00 PM, Ivan Khoronzhuk wrote:
> > > No need now to flush mcast entries in switch mode while toggling to
> > > promiscuous mode. It's not needed as vlan reg_mcast = ALL_PORTS
> > > and mcast/vlan ports = ALL_PORTS, the same happening for vlan
> > > unreg_mcast, it's set to ALL_PORT_MASK just after calling promisc
> > > mode routine by calling set allmulti. I suppose main reason to flush
> > > them is to use unreg_mcast to receive all to host port. Thus, now, all
> > > mcast packets are received anyway and no reason to flush mcast entries
> > > unsafely, as they were synced with __dev_mc_sync() previously and are
> > > not restored. Another way is to _dev_mc_unsync() them, but no need.
> >
> > User have possibility to add additional mcast entries or edit existing 
> > in switch-mode, which is now done using custom tool. So, Host in promisc
> > mode will not receive packets for mcast address X if port mask for this
> > addr set to (ALL_PORTS - HOST_PORT). Am I missing smth?
>
> I didn't take into account the custom tool changing entries directly,
> but even in this case there is at least a couple of interesting
> questions:
>
> 1) Before the patch applied only several days ago -
>  Â 5da1948969bc1991920987ce4361ea56046e5a98
>  Â "ti: cpsw: fix lost of mcast packets while rx_mode update"
>  Â It was impossible to do correctly anyway, as all mcast entries not
>  Â in the mc list were flushed (after rx_mode cb), by:
>  Â cpsw_ale_flush_multicast(cpsw->ale, ALE_ALL_PORTS, vid);
>  Â and those in mc, rewritten by adding them back in corrected form.
>  Â ... or this cb was not supposed to be called at all ...

It's not allowed to manipulate ALE table in dual_mac mode, so your
patches are safe in dual_mac mode. For switch-mode (unless we  move
forward with switch dev) standard linux interfaces allows create
default mcast entries which then (if required) corrected using custom
tool now.

> 2) What is the reason to add mcast switch entires
>  Â (ALL_PORTS - HOST_PORT) if its function is added anyway by
>  Â unreq_mcast & (ALL_PORTS - HOST_PORT) ?
>  Â So, doesn't matter it's added or not - it will work :-|.

because in switch mode not all traffic directed to the Host port -
only in promisc mode. Reason safety and performance - Host should not
receive traffic which is not designated for it.

promiscuous in switch mode:
- disables learning
- enables unicast flooding to Host port
- enables unregistered multi-cast flooding to the Host port
In other words, CPSW will continue forwarding packets between P1&P2, but 
also will "duplicate" packets to Host port. This will work only for
vlans which have host port as member.

> 3) Even so, toggling promisc mode will clear all these changes anyway,
>  Â even I will call _dev_mc_unsync() after flushing them.

there can be records which are not under control of Linux now.

> 4) If user can tune ALE table by hand, what stops him do it after moving
>  Â to promisc mode, seems he knows what he's doing?
>
> 5) It could be possible only for not default vlan entries, but mcast
>  Â vlan support is not supported yet. Who is gona restore those
>  Â entries after promisc off?
>
> This behaviour is arguable, and flushing mcast entries can bring more
> issues then leaving. For me it doesn't matter, I can archive the same
> by adding after flush one line, it's even shorter:
> __dev_mc_unsync(priv->ndev, NULL);

Again, unless we move forward with switch dev you can't assume that
Linux stack has full control over ALE table. Sry, hence this patch is
not a fix and can introduce changes in current behavior and cause
regression reports - NACK.

--
regards,
-grygorii