| Message ID | 20181018180006.7065-1-ivan.khoronzhuk@linaro.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: ethernet: ti: cpsw: don't flush mcast entries while switch promisc mode | expand |
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? > > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> > --- > > Based on net-next/master > Tasted on am572x EVM and BBB > > drivers/net/ethernet/ti/cpsw.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c > index 226be2a56c1f..0e475020a674 100644 > --- a/drivers/net/ethernet/ti/cpsw.c > +++ b/drivers/net/ethernet/ti/cpsw.c > @@ -638,9 +638,6 @@ static void cpsw_set_promiscious(struct net_device *ndev, bool enable) > } while (time_after(timeout, jiffies)); > cpsw_ale_control_set(ale, 0, ALE_AGEOUT, 1); > > - /* Clear all mcast from ALE */ > - cpsw_ale_flush_multicast(ale, ALE_ALL_PORTS, -1); > - > /* Flood All Unicast Packets to Host port */ > cpsw_ale_control_set(ale, 0, ALE_P0_UNI_FLOOD, 1); > dev_dbg(&ndev->dev, "promiscuity enabled\n"); >
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 ... 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 :-|. 3) Even so, toggling promisc mode will clear all these changes anyway, even I will call _dev_mc_unsync() after flushing them. 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); > >> >>Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >>--- >> >>Based on net-next/master >>Tasted on am572x EVM and BBB >> >> drivers/net/ethernet/ti/cpsw.c | 3 --- >> 1 file changed, 3 deletions(-) >> >>diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c >>index 226be2a56c1f..0e475020a674 100644 >>--- a/drivers/net/ethernet/ti/cpsw.c >>+++ b/drivers/net/ethernet/ti/cpsw.c >>@@ -638,9 +638,6 @@ static void cpsw_set_promiscious(struct net_device *ndev, bool enable) >> } while (time_after(timeout, jiffies)); >> cpsw_ale_control_set(ale, 0, ALE_AGEOUT, 1); >>- /* Clear all mcast from ALE */ >>- cpsw_ale_flush_multicast(ale, ALE_ALL_PORTS, -1); >>- >> /* Flood All Unicast Packets to Host port */ >> cpsw_ale_control_set(ale, 0, ALE_P0_UNI_FLOOD, 1); >> dev_dbg(&ndev->dev, "promiscuity enabled\n"); >> > >-- >regards, >-grygorii
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.
On Fri, Oct 19, 2018 at 12:23:28PM -0500, Grygorii Strashko wrote: > > >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. It's not related to my patches at all, this as it was from very beginning. My proposition with __dev_mc_unsync(priv->ndev, NULL) is safe as for dual_mac mode as for switch mode. > >> >>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 - No objections, that's exactly I'm talking about here, only p1&p2. And looks like you forgot about single port board reusing switch implementation, which by fact is not a switch but reuse this code. Proposition to flush all its mcast table w/o restore - softly speaking isn't best choice. >only in promisc mode. Reason safety and performance - Host should not >receive traffic which is not designated for it. Any objections. > >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. Sorry, but what part was not clearly described in the cover letter? If you mean vlan entries added not by linux (linux vlans have host port), and having no host port as a memeber - flush() hasn't been helping in this case also. > >> >>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. yes, and they are flushed w/o restore (along with linux ones). that's why fix is needed. > >> >>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. and that's why Linux stack is flushing all not created by it mcast entries... but this what it was doing w/o my changes ). after my patch it won't, Look: - it adds/removes entries under control of linux - it doesn't touch entries added whoever but not a linux - it restores only entries added linux (it was before also, but with flushing all not related entries.. ..deleting what you don't want to delete) >Sry, hence this patch is >not a fix and can introduce changes in current behavior and cause >regression reports - NACK. Sorry but what regression you are talking about, what the problem with __dev_mc_unsync(priv->ndev, NULL)? after flush? This is more than fix. Restoring entries that were under control of linux is not a fix? It doesn't touch anything added by custom tool and restores entries in mc list like it was before. According to your comments, it seems absolutely one fix you need, ...and don't forget about BBB with one port. > >-- >regards, >-grygorii
Grygorii, On Fri, Oct 19, 2018 at 10:24:55PM +0300, Ivan Khoronzhuk wrote: >On Fri, Oct 19, 2018 at 12:23:28PM -0500, Grygorii Strashko wrote: >> >> >>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. I've sent new patch achiving the same but with second viariant described in this cover letter. Please, look at it. No rush.
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c index 226be2a56c1f..0e475020a674 100644 --- a/drivers/net/ethernet/ti/cpsw.c +++ b/drivers/net/ethernet/ti/cpsw.c @@ -638,9 +638,6 @@ static void cpsw_set_promiscious(struct net_device *ndev, bool enable) } while (time_after(timeout, jiffies)); cpsw_ale_control_set(ale, 0, ALE_AGEOUT, 1); - /* Clear all mcast from ALE */ - cpsw_ale_flush_multicast(ale, ALE_ALL_PORTS, -1); - /* Flood All Unicast Packets to Host port */ cpsw_ale_control_set(ale, 0, ALE_P0_UNI_FLOOD, 1); dev_dbg(&ndev->dev, "promiscuity enabled\n");
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. Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> --- Based on net-next/master Tasted on am572x EVM and BBB drivers/net/ethernet/ti/cpsw.c | 3 --- 1 file changed, 3 deletions(-)