diff mbox series

[net-next] net: ethernet: ti: cpsw: drop vid0 configuration in dual_mac mode

Message ID 20181125234626.28474-1-grygorii.strashko@ti.com
State New
Headers show
Series [net-next] net: ethernet: ti: cpsw: drop vid0 configuration in dual_mac mode | expand

Commit Message

Grygorii Strashko Nov. 25, 2018, 11:46 p.m. UTC 
In dual_mac mode CPSW driver uses vid1 and vid2 by default to implement
dual mac mode wich are used to configure pvids for each external ports.
But, historicaly, it also adds vid0 to ALE table and sets "untag" bits for both
ext. ports. As result, it's imposible to use priority tagged packets in
dual mac mode.

Hence, drop vid0 configuration in dual mac mode as it's not required for dual
mac mode functionality and, this way, make it possible to use priority
tagged packet in dual mac mode.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

---
 drivers/net/ethernet/ti/cpsw.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

-- 
2.10.5

Comments

Ivan Khoronzhuk Nov. 26, 2018, 4:26 p.m. UTC | #1 
On Sun, Nov 25, 2018 at 05:46:26PM -0600, Grygorii Strashko wrote:
>In dual_mac mode CPSW driver uses vid1 and vid2 by default to implement

>dual mac mode wich are used to configure pvids for each external ports.

>But, historicaly, it also adds vid0 to ALE table and sets "untag" bits for both

>ext. ports. As result, it's imposible to use priority tagged packets in

>dual mac mode.

>

>Hence, drop vid0 configuration in dual mac mode as it's not required for dual

>mac mode functionality and, this way, make it possible to use priority

>tagged packet in dual mac mode.

So, now it's enabled to be added via regular ndo.
I have similar change in mind, but was going to send it after
mcast/ucast, and - enabling same vlans patch...

2 things stopped me to add this:

1) Moving it to be enabled via regular call is Ok, but in dual mac mode
it causes overlaps, at least while vid deletion. So decided to wait till
same vlans series is applied.

2) Wanted implement somehow similar handling for single port boards
in one patch, not only for dual mac mode. This part was not clear and
not verified completely...

So, if it's needed now, maybe better at this moment only remove
untag field? and remove vlan0 later, once other vlan changes applied.

Say:

cpsw_ale_add_vlan(cpsw->ale, cpsw->data.default_vlan,
		  ALE_ALL_PORTS, 0, ALE_ALL_PORTS, 0);

instead of:
cpsw_ale_add_vlan(cpsw->ale, cpsw->data.default_vlan,
		  ALE_ALL_PORTS, ALE_ALL_PORTS, 0, 0);

>

>Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

>---

> drivers/net/ethernet/ti/cpsw.c | 7 ++-----

> 1 file changed, 2 insertions(+), 5 deletions(-)

>

>diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c

>index 15d563c..4f3a159 100644

>--- a/drivers/net/ethernet/ti/cpsw.c

>+++ b/drivers/net/ethernet/ti/cpsw.c

>@@ -2036,9 +2036,6 @@ static int cpsw_ndo_open(struct net_device *ndev)

> 	/* Add default VLAN */

> 	if (!cpsw->data.dual_emac)

> 		cpsw_add_default_vlan(priv);

>-	else

>-		cpsw_ale_add_vlan(cpsw->ale, cpsw->data.default_vlan,

>-				  ALE_ALL_PORTS, ALE_ALL_PORTS, 0, 0);

>

> 	/* initialize shared resources for every ndev */

> 	if (!cpsw->usage_count) {

>@@ -2490,7 +2487,7 @@ static int cpsw_ndo_vlan_rx_add_vid(struct net_device *ndev,

> 	struct cpsw_common *cpsw = priv->cpsw;

> 	int ret;

>

>-	if (vid == cpsw->data.default_vlan)

>+	if (!cpsw->data.dual_emac && vid == cpsw->data.default_vlan)

> 		return 0;

>

> 	ret = pm_runtime_get_sync(cpsw->dev);

>@@ -2528,7 +2525,7 @@ static int cpsw_ndo_vlan_rx_kill_vid(struct net_device *ndev,

> 	struct cpsw_common *cpsw = priv->cpsw;

> 	int ret;

>

>-	if (vid == cpsw->data.default_vlan)

>+	if (!cpsw->data.dual_emac && vid == cpsw->data.default_vlan)

> 		return 0;

>

> 	ret = pm_runtime_get_sync(cpsw->dev);

>-- 

>2.10.5

>


-- 
Regards,
Ivan Khoronzhuk
Ivan Khoronzhuk Nov. 30, 2018, 1:42 p.m. UTC | #2 
On Thu, Nov 29, 2018 at 05:23:09PM -0600, Grygorii Strashko wrote:
>

>

>On 11/29/18 9:26 AM, Ivan Khoronzhuk wrote:

>> On Wed, Nov 28, 2018 at 03:15:46PM -0600, Grygorii Strashko wrote:

>>>

>>>

>>> On 11/26/18 2:07 PM, Ivan Khoronzhuk wrote:

>>>> On Mon, Nov 26, 2018 at 12:57:20PM -0600, Grygorii Strashko wrote:

>>>>>

>>>>>

>>>>> On 11/26/18 10:26 AM, Ivan Khoronzhuk wrote:

>>>>>> On Sun, Nov 25, 2018 at 05:46:26PM -0600, Grygorii Strashko wrote:

>>>>>>> In dual_mac mode CPSW driver uses vid1 and vid2 by default to implement

>>>>>>> dual mac mode wich are used to configure pvids for each external ports.

>>>>>>> But, historicaly, it also adds vid0 to ALE table and sets "untag" bits for both

>>>>>>> ext. ports. As result, it's imposible to use priority tagged packets in

>>>>>>> dual mac mode.

>>>>>>>

>>>>>>> Hence, drop vid0 configuration in dual mac mode as it's not required for dual

>>>>>>> mac mode functionality and, this way, make it possible to use priority

>>>>>>> tagged packet in dual mac mode.

>>>>>> So, now it's enabled to be added via regular ndo.

>>>>>> I have similar change in mind, but was going to send it after

>>>>>> mcast/ucast, and - enabling same vlans patch...

>>>>>>

>>>>>> 2 things stopped me to add this:

>>>>>>

>>>>>> 1) Moving it to be enabled via regular call is Ok, but in dual mac mode

>>>>>> it causes overlaps, at least while vid deletion. So decided to wait till

>>>>>> same vlans series is applied.

>>>>>

>>>>> TI driver documentation mentions this restriction

>>>>> "While adding VLAN id to the eth interfaces,

>>>>> same VLAN id should not be added in both interfaces which will lead to VLAN

>>>>> forwarding and act as switch"

>>>> It's not accurate now.

>>>> This sw bug "acting like a switch" was fixed indirectly in LKML ).

>>>> And at least for upstream version, not TISDK, desc should be updated,

>>>> but better do this when it fixed completely and merged with TISDK.

>>>>

>>>> I know about this "written" restriction

>>>> (for tiSDK, and it's not TRM after all ...),

>>>> it can be avoided and it's partly avoided now ...

>>>

>>> I'd like to clarify point about supporting same VLANs in dual_mac mode,

>>> to avoid future misunderstanding, overall: it's *not* supported as

>>> adding same VLAN to both netdevices will cause unknown unicast packets

>>> leaking between interfaces and it can't be avoided - hw limitation.

>>

>> Simple test shows no issues with ucast leaking.

>> But for current buggy ucast vlan implementation it's not possible to verify,

>> not sure but probably leaking in your case cuased by hidden toggling of

>> interface to promisc while added ucast to vlans or other reason or so.

>> Anyway I just decided to check specifically ucasts

>> (macst as you know are not normal now).

>>

>> For verification you need to apply ucast fix (including vlans) first:

>> https://git.linaro.org/people/ivan.khoronzhuk/tsn_kernel.git/log/?h=vlan_addr_flt_fix

>>

>> This is generic fix (not sure it will be approved, need try RFC) but implement

>> the same as local fix for vlan ucasts:

>> https://git.linaro.org/people/ivan.khoronzhuk/tsn_kernel.git/log/?h=ucast_vlan_fix

>>

>> Any of those are correct. I've used generic one.

>> Applied the following scheme:

>>

>>                      +--------------------------+

>>                      | host 74:DA:EA:47:7D:9C   |

>>                      +--------------------------+

>>

>>                         +---------------------+

>>              |       am572 evm     |

>>                         |    eth0      eth1   |

>>                         +----------+----------+

>>                         | eth0.400 | eth1.400 |

>>                         +----------+----------+

>>                             ^          |

>>                             |          |  +-----------+

>> +-----------------+        |          |  |     PC    |

>> | BBB eth0.400    |--------+          +->| Wireshark |

>> +-----------------+                      +-----------+

>>

>>

>> 1) Configure vlans on am572x evm

>> ip link add link eth0 name eth0.400 type vlan id 400

>> ip link add link eth1 name eth1.400 type vlan id 400

>>

>> 2) On BBB side:

>> # ip link add link eth0 name eth0.400 type vlan id 400

>> Send ucast vlan traffic to the am572 evm, vlan ucast address is unreq on am572.

>> # ./plget -i eth0.400 -t ptpl2 -m tx-lat -n 160 -s 10 -a 74:DA:EA:47:7D:66

>> # ./plget -i eth0.400 -t ptpl2 -m tx-lat -n 160 -s 10 -a 18:03:73:66:87:42

>>

>> 3) Observe silence on PC wireshark.

>>

>> Thus, no see issues with this.

>>

>> PS: I'm sure in plget tool, you can use your own.

>

>I'm using packeth to generate udp packets (vlan) src=PC dst=unknown

>if there is record in ALE table which looks like:

>type: vlan , vid = 100, untag_force = 0x0, reg_mcast = 0x7, unreg_mcast = 0x0, member_list = 0x7

>then above udp packet will be forwarded to BBB.

Agree, seems no normal way to avoid ucast leak.

>

>

>

>-- 

>regards,

>-grygorii


-- 
Regards,
Ivan Khoronzhuk
Ivan Khoronzhuk Nov. 30, 2018, 1:55 p.m. UTC | #3 
On Fri, Nov 30, 2018 at 03:42:29PM +0200, Ivan Khoronzhuk wrote:
>On Thu, Nov 29, 2018 at 05:23:09PM -0600, Grygorii Strashko wrote:

>>

>>

>>On 11/29/18 9:26 AM, Ivan Khoronzhuk wrote:

>>>On Wed, Nov 28, 2018 at 03:15:46PM -0600, Grygorii Strashko wrote:

>>>>

>>>>

>>>>On 11/26/18 2:07 PM, Ivan Khoronzhuk wrote:

>>>>>On Mon, Nov 26, 2018 at 12:57:20PM -0600, Grygorii Strashko wrote:

>>>>>>

>>>>>>

>>>>>>On 11/26/18 10:26 AM, Ivan Khoronzhuk wrote:

>>>>>>>On Sun, Nov 25, 2018 at 05:46:26PM -0600, Grygorii Strashko wrote:

>>>>>>>>In dual_mac mode CPSW driver uses vid1 and vid2 by default to implement

>>>>>>>>dual mac mode wich are used to configure pvids for each external ports.

>>>>>>>>But, historicaly, it also adds vid0 to ALE table and sets "untag" bits for both

>>>>>>>>ext. ports. As result, it's imposible to use priority tagged packets in

>>>>>>>>dual mac mode.

>>>>>>>>

>>>>>>>>Hence, drop vid0 configuration in dual mac mode as it's not required for dual

>>>>>>>>mac mode functionality and, this way, make it possible to use priority

>>>>>>>>tagged packet in dual mac mode.

>>>>>>>So, now it's enabled to be added via regular ndo.

>>>>>>>I have similar change in mind, but was going to send it after

>>>>>>>mcast/ucast, and - enabling same vlans patch...

>>>>>>>

>>>>>>>2 things stopped me to add this:

>>>>>>>

>>>>>>>1) Moving it to be enabled via regular call is Ok, but in dual mac mode

>>>>>>>it causes overlaps, at least while vid deletion. So decided to wait till

>>>>>>>same vlans series is applied.

>>>>>>

>>>>>>TI driver documentation mentions this restriction

>>>>>>"While adding VLAN id to the eth interfaces,

>>>>>>same VLAN id should not be added in both interfaces which will lead to VLAN

>>>>>>forwarding and act as switch"

>>>>>It's not accurate now.

>>>>>This sw bug "acting like a switch" was fixed indirectly in LKML ).

>>>>>And at least for upstream version, not TISDK, desc should be updated,

>>>>>but better do this when it fixed completely and merged with TISDK.

>>>>>

>>>>>I know about this "written" restriction

>>>>>(for tiSDK, and it's not TRM after all ...),

>>>>>it can be avoided and it's partly avoided now ...

>>>>

>>>>I'd like to clarify point about supporting same VLANs in dual_mac mode,

>>>>to avoid future misunderstanding, overall: it's *not* supported as

>>>>adding same VLAN to both netdevices will cause unknown unicast packets

>>>>leaking between interfaces and it can't be avoided - hw limitation.

>>>

>>>Simple test shows no issues with ucast leaking.

>>>But for current buggy ucast vlan implementation it's not possible to verify,

>>>not sure but probably leaking in your case cuased by hidden toggling of

>>>interface to promisc while added ucast to vlans or other reason or so.

>>>Anyway I just decided to check specifically ucasts

>>>(macst as you know are not normal now).

>>>

>>>For verification you need to apply ucast fix (including vlans) first:

>>>https://git.linaro.org/people/ivan.khoronzhuk/tsn_kernel.git/log/?h=vlan_addr_flt_fix

>>>

>>>This is generic fix (not sure it will be approved, need try RFC) but implement

>>>the same as local fix for vlan ucasts:

>>>https://git.linaro.org/people/ivan.khoronzhuk/tsn_kernel.git/log/?h=ucast_vlan_fix

>>>

>>>Any of those are correct. I've used generic one.

>>>Applied the following scheme:

>>>

>>>                     +--------------------------+

>>>                     | host 74:DA:EA:47:7D:9C   |

>>>                     +--------------------------+

>>>

>>>                        +---------------------+

>>>             |       am572 evm     |

>>>                        |    eth0      eth1   |

>>>                        +----------+----------+

>>>                        | eth0.400 | eth1.400 |

>>>                        +----------+----------+

>>>                            ^          |

>>>                            |          |  +-----------+

>>>+-----------------+        |          |  |     PC    |

>>>| BBB eth0.400    |--------+          +->| Wireshark |

>>>+-----------------+                      +-----------+

>>>

>>>

>>>1) Configure vlans on am572x evm

>>>ip link add link eth0 name eth0.400 type vlan id 400

>>>ip link add link eth1 name eth1.400 type vlan id 400

>>>

>>>2) On BBB side:

>>># ip link add link eth0 name eth0.400 type vlan id 400

>>>Send ucast vlan traffic to the am572 evm, vlan ucast address is unreq on am572.

>>># ./plget -i eth0.400 -t ptpl2 -m tx-lat -n 160 -s 10 -a 74:DA:EA:47:7D:66

>>># ./plget -i eth0.400 -t ptpl2 -m tx-lat -n 160 -s 10 -a 18:03:73:66:87:42

>>>

>>>3) Observe silence on PC wireshark.

>>>

>>>Thus, no see issues with this.

>>>

>>>PS: I'm sure in plget tool, you can use your own.

>>

>>I'm using packeth to generate udp packets (vlan) src=PC dst=unknown

>>if there is record in ALE table which looks like:

>>type: vlan , vid = 100, untag_force = 0x0, reg_mcast = 0x7, unreg_mcast = 0x0, member_list = 0x7

>>then above udp packet will be forwarded to BBB.

>Agree, seems no normal way to avoid ucast leak.


One of the ways could be removing end ports as memembers, leaving only port0:

type: vlan , vid = 100, untag_force = 0x0, reg_mcast = 0x7, unreg_mcast = 0x0, member_list = 0x1

and allow tagged packets to be received by ports beeing non memebers of a vlan:

       cpsw_ale_control_set(cpsw->ale, slave_port,
			    ALE_PORT_DROP_UNKNOWN_VLAN, 0);

So that only unknown vlans are dropped...

>

>>

>>

>>

>>-- 

>>regards,

>>-grygorii

>

>-- 

>Regards,

>Ivan Khoronzhuk


-- 
Regards,
Ivan Khoronzhuk
diff mbox series

Patch

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 15d563c..4f3a159 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2036,9 +2036,6 @@  static int cpsw_ndo_open(struct net_device *ndev)
 	/* Add default VLAN */
 	if (!cpsw->data.dual_emac)
 		cpsw_add_default_vlan(priv);
-	else
-		cpsw_ale_add_vlan(cpsw->ale, cpsw->data.default_vlan,
-				  ALE_ALL_PORTS, ALE_ALL_PORTS, 0, 0);
 
 	/* initialize shared resources for every ndev */
 	if (!cpsw->usage_count) {
@@ -2490,7 +2487,7 @@  static int cpsw_ndo_vlan_rx_add_vid(struct net_device *ndev,
 	struct cpsw_common *cpsw = priv->cpsw;
 	int ret;
 
-	if (vid == cpsw->data.default_vlan)
+	if (!cpsw->data.dual_emac && vid == cpsw->data.default_vlan)
 		return 0;
 
 	ret = pm_runtime_get_sync(cpsw->dev);
@@ -2528,7 +2525,7 @@  static int cpsw_ndo_vlan_rx_kill_vid(struct net_device *ndev,
 	struct cpsw_common *cpsw = priv->cpsw;
 	int ret;
 
-	if (vid == cpsw->data.default_vlan)
+	if (!cpsw->data.dual_emac && vid == cpsw->data.default_vlan)
 		return 0;
 
 	ret = pm_runtime_get_sync(cpsw->dev);