[PATCH] net: dsa: ksz: Fix port membership

[PATCH] net: dsa: ksz: Fix port membership

[Marek Vasut]

If two ports are in the same bridge and in forwarding state, the packets
must be able to pass between them in both directions. The current code
only configures this bridge membership for a port newly added to the
bridge, but does not update all the other ports. Thus, ingress packets
on the new port will be forwarded, but ingress packets on other ports
destined for the new port (eg. a reply) will not be forwarded back to
the new port, because they are not configured to do so. This patch fixes
that by updating the membership registers of all ports.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Cc: Woojung Huh <woojung.huh@microchip.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Tristram Ha <Tristram.Ha@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 0684657fbf9a9..e24dd14ccde77 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -396,7 +396,7 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port,
 	struct ksz_device *dev = ds->priv;
 	struct ksz_port *p = &dev->ports[port];
 	u8 data;
-	int member = -1;
+	int i, member = -1;
 
 	ksz_pread8(dev, port, P_STP_CTRL, &data);
 	data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE | PORT_LEARN_DISABLE);
@@ -454,8 +454,8 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port,
 		dev->tx_ports &= ~(1 << port);
 
 	/* Port membership may share register with STP state. */
-	if (member >= 0 && member != p->member)
-		ksz9477_cfg_port_member(dev, port, (u8)member);
+	for (i = 0; i < SWITCH_PORT_NUM; i++)
+		ksz9477_cfg_port_member(dev, i, (u8)member);
 
 	/* Check if forwarding needs to be updated. */
 	if (state != BR_STATE_FORWARDING) {
-- 
2.18.0

RE: [PATCH] net: dsa: ksz: Fix port membership

[Tristram.Ha]

> If two ports are in the same bridge and in forwarding state, the packets
> must be able to pass between them in both directions. The current code
> only configures this bridge membership for a port newly added to the
> bridge, but does not update all the other ports. Thus, ingress packets
> on the new port will be forwarded, but ingress packets on other ports
> destined for the new port (eg. a reply) will not be forwarded back to
> the new port, because they are not configured to do so. This patch fixes
> that by updating the membership registers of all ports.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Cc: Woojung Huh <woojung.huh@microchip.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Tristram Ha <Tristram.Ha@microchip.com>
> ---
>  drivers/net/dsa/microchip/ksz9477.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c
> b/drivers/net/dsa/microchip/ksz9477.c
> index 0684657fbf9a9..e24dd14ccde77 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -396,7 +396,7 @@ static void ksz9477_port_stp_state_set(struct
> dsa_switch *ds, int port,
>  	struct ksz_device *dev = ds->priv;
>  	struct ksz_port *p = &dev->ports[port];
>  	u8 data;
> -	int member = -1;
> +	int i, member = -1;
> 
>  	ksz_pread8(dev, port, P_STP_CTRL, &data);
>  	data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE |
> PORT_LEARN_DISABLE);
> @@ -454,8 +454,8 @@ static void ksz9477_port_stp_state_set(struct
> dsa_switch *ds, int port,
>  		dev->tx_ports &= ~(1 << port);
> 
>  	/* Port membership may share register with STP state. */
> -	if (member >= 0 && member != p->member)
> -		ksz9477_cfg_port_member(dev, port, (u8)member);
> +	for (i = 0; i < SWITCH_PORT_NUM; i++)
> +		ksz9477_cfg_port_member(dev, i, (u8)member);
> 
>  	/* Check if forwarding needs to be updated. */
>  	if (state != BR_STATE_FORWARDING) {

The original DSA model did not have a way to tell the bridge device not to
forward the frame, so the switch driver always setup the membership to
disable forwarding between ports.

When lan devices are setup they act like individual devices.  A bridge device
adding them under it will forward the frames.

The new switchdev model adds the offload_fwd_mark bit to tell the bridge not to
forward frame.

The ksz_update_port_member function in ksz_common.c is doing this membership
setup for all forwarding ports.  It was finally enabled in one of the RFC patches I
submitted recently (Add switch forward offloading support).

I think if you do this without setting offload_fwd_mark you will receive duplicate
frame.

Re: [PATCH] net: dsa: ksz: Fix port membership

[Florian Fainelli]

On 12/7/18 11:37 AM, Tristram.Ha@microchip.com wrote:
>> If two ports are in the same bridge and in forwarding state, the packets
>> must be able to pass between them in both directions. The current code
>> only configures this bridge membership for a port newly added to the
>> bridge, but does not update all the other ports. Thus, ingress packets
>> on the new port will be forwarded, but ingress packets on other ports
>> destined for the new port (eg. a reply) will not be forwarded back to
>> the new port, because they are not configured to do so. This patch fixes
>> that by updating the membership registers of all ports.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> Cc: Woojung Huh <woojung.huh@microchip.com>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Tristram Ha <Tristram.Ha@microchip.com>
>> ---
>>  drivers/net/dsa/microchip/ksz9477.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/dsa/microchip/ksz9477.c
>> b/drivers/net/dsa/microchip/ksz9477.c
>> index 0684657fbf9a9..e24dd14ccde77 100644
>> --- a/drivers/net/dsa/microchip/ksz9477.c
>> +++ b/drivers/net/dsa/microchip/ksz9477.c
>> @@ -396,7 +396,7 @@ static void ksz9477_port_stp_state_set(struct
>> dsa_switch *ds, int port,
>>  	struct ksz_device *dev = ds->priv;
>>  	struct ksz_port *p = &dev->ports[port];
>>  	u8 data;
>> -	int member = -1;
>> +	int i, member = -1;
>>
>>  	ksz_pread8(dev, port, P_STP_CTRL, &data);
>>  	data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE |
>> PORT_LEARN_DISABLE);
>> @@ -454,8 +454,8 @@ static void ksz9477_port_stp_state_set(struct
>> dsa_switch *ds, int port,
>>  		dev->tx_ports &= ~(1 << port);
>>
>>  	/* Port membership may share register with STP state. */
>> -	if (member >= 0 && member != p->member)
>> -		ksz9477_cfg_port_member(dev, port, (u8)member);
>> +	for (i = 0; i < SWITCH_PORT_NUM; i++)
>> +		ksz9477_cfg_port_member(dev, i, (u8)member);
>>
>>  	/* Check if forwarding needs to be updated. */
>>  	if (state != BR_STATE_FORWARDING) {
> 
> The original DSA model did not have a way to tell the bridge device not to
> forward the frame, so the switch driver always setup the membership to
> disable forwarding between ports.
> 
> When lan devices are setup they act like individual devices.  A bridge device
> adding them under it will forward the frames.
> 
> The new switchdev model adds the offload_fwd_mark bit to tell the bridge not to
> forward frame.
> 
> The ksz_update_port_member function in ksz_common.c is doing this membership
> setup for all forwarding ports.  It was finally enabled in one of the RFC patches I
> submitted recently (Add switch forward offloading support).
> 
> I think if you do this without setting offload_fwd_mark you will receive duplicate
> frame.
> 

Either I am misreading Marek's patch, or I don't quite understand your
response, but what is happening when you enslave a switch port into a
bridge is that you need to make sure that:

- the switch port being enslaved will be part of the same forwarding
group as any other switch port already in the bridge
- any existing switch port already enslaved in the bridge must now also
be allowed to forward to the port that is being enslaved

That is to me, exactly what Marek's patch is fixing, your response is
about something slightly orthogonal here.
-- 
Florian

RE: [PATCH] net: dsa: ksz: Fix port membership

[Tristram.Ha]

> >> If two ports are in the same bridge and in forwarding state, the packets
> >> must be able to pass between them in both directions. The current code
> >> only configures this bridge membership for a port newly added to the
> >> bridge, but does not update all the other ports. Thus, ingress packets
> >> on the new port will be forwarded, but ingress packets on other ports
> >> destined for the new port (eg. a reply) will not be forwarded back to
> >> the new port, because they are not configured to do so. This patch fixes
> >> that by updating the membership registers of all ports.
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> Cc: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> >> Cc: Woojung Huh <woojung.huh@microchip.com>
> >> Cc: David S. Miller <davem@davemloft.net>
> >> Cc: Tristram Ha <Tristram.Ha@microchip.com>
> >> ---
> >>  drivers/net/dsa/microchip/ksz9477.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/net/dsa/microchip/ksz9477.c
> >> b/drivers/net/dsa/microchip/ksz9477.c
> >> index 0684657fbf9a9..e24dd14ccde77 100644
> >> --- a/drivers/net/dsa/microchip/ksz9477.c
> >> +++ b/drivers/net/dsa/microchip/ksz9477.c
> >> @@ -396,7 +396,7 @@ static void ksz9477_port_stp_state_set(struct
> >> dsa_switch *ds, int port,
> >>  	struct ksz_device *dev = ds->priv;
> >>  	struct ksz_port *p = &dev->ports[port];
> >>  	u8 data;
> >> -	int member = -1;
> >> +	int i, member = -1;
> >>
> >>  	ksz_pread8(dev, port, P_STP_CTRL, &data);
> >>  	data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE |
> >> PORT_LEARN_DISABLE);
> >> @@ -454,8 +454,8 @@ static void ksz9477_port_stp_state_set(struct
> >> dsa_switch *ds, int port,
> >>  		dev->tx_ports &= ~(1 << port);
> >>
> >>  	/* Port membership may share register with STP state. */
> >> -	if (member >= 0 && member != p->member)
> >> -		ksz9477_cfg_port_member(dev, port, (u8)member);
> >> +	for (i = 0; i < SWITCH_PORT_NUM; i++)
> >> +		ksz9477_cfg_port_member(dev, i, (u8)member);
> >>
> >>  	/* Check if forwarding needs to be updated. */
> >>  	if (state != BR_STATE_FORWARDING) {
> >
> > The original DSA model did not have a way to tell the bridge device not to
> > forward the frame, so the switch driver always setup the membership to
> > disable forwarding between ports.
> >
> > When lan devices are setup they act like individual devices.  A bridge device
> > adding them under it will forward the frames.
> >
> > The new switchdev model adds the offload_fwd_mark bit to tell the bridge
> not to
> > forward frame.
> >
> > The ksz_update_port_member function in ksz_common.c is doing this
> membership
> > setup for all forwarding ports.  It was finally enabled in one of the RFC
> patches I
> > submitted recently (Add switch forward offloading support).
> >
> > I think if you do this without setting offload_fwd_mark you will receive
> duplicate
> > frame.
> >
> 
> Either I am misreading Marek's patch, or I don't quite understand your
> response, but what is happening when you enslave a switch port into a
> bridge is that you need to make sure that:
> 
> - the switch port being enslaved will be part of the same forwarding
> group as any other switch port already in the bridge
> - any existing switch port already enslaved in the bridge must now also
> be allowed to forward to the port that is being enslaved
> 
> That is to me, exactly what Marek's patch is fixing, your response is
> about something slightly orthogonal here.

I got confused here as the code is obviously wrong and should not work,
so I found out why it works in the bridge device situation.  There is actually
a bug in the driver that enables this behavior.  The port_vlan_filtering function
turns off the port membership enforcement.  Fixing this problem should be
easy, but this port_vlan_filtering function is also not implemented right.  It treats the
operation as a simple VLAN on/off, but it is more complex than that.

Anyway it seems to work in the bridge device situation, but it does not work
in the default situation:

Assume there are two port devices lan1 and lan2.  The device lan1 is assigned
an IP address and can talk to outside.  Enabling and disabling lan2 by doing
"ifconfig lan2 up" and "ifconfig lan2 down."  The device lan1 is no longer working.

Create a bridge device and add a child device by doing "brctl addbr br0" and
"brctl addif br0 lan1."  This will call port_vlan_filtering and then the feature
UNICAST_VLAN_BOUNDARY is disabled.  This causes port membership to have no
effect on unicast packets and so it does not matter what member value is used.
The device lan1 can start working again.

The fix is to avoid disabling UNICAST_VLAN_BOUNDARY and it should be set all
the time.  In this switch the default is on.

Re: [PATCH] net: dsa: ksz: Fix port membership

[Marek Vasut]

On 12/07/2018 08:37 PM, Tristram.Ha@microchip.com wrote:
>> If two ports are in the same bridge and in forwarding state, the packets
>> must be able to pass between them in both directions. The current code
>> only configures this bridge membership for a port newly added to the
>> bridge, but does not update all the other ports. Thus, ingress packets
>> on the new port will be forwarded, but ingress packets on other ports
>> destined for the new port (eg. a reply) will not be forwarded back to
>> the new port, because they are not configured to do so. This patch fixes
>> that by updating the membership registers of all ports.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> Cc: Woojung Huh <woojung.huh@microchip.com>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Tristram Ha <Tristram.Ha@microchip.com>
>> ---
>>  drivers/net/dsa/microchip/ksz9477.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/dsa/microchip/ksz9477.c
>> b/drivers/net/dsa/microchip/ksz9477.c
>> index 0684657fbf9a9..e24dd14ccde77 100644
>> --- a/drivers/net/dsa/microchip/ksz9477.c
>> +++ b/drivers/net/dsa/microchip/ksz9477.c
>> @@ -396,7 +396,7 @@ static void ksz9477_port_stp_state_set(struct
>> dsa_switch *ds, int port,
>>  	struct ksz_device *dev = ds->priv;
>>  	struct ksz_port *p = &dev->ports[port];
>>  	u8 data;
>> -	int member = -1;
>> +	int i, member = -1;
>>
>>  	ksz_pread8(dev, port, P_STP_CTRL, &data);
>>  	data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE |
>> PORT_LEARN_DISABLE);
>> @@ -454,8 +454,8 @@ static void ksz9477_port_stp_state_set(struct
>> dsa_switch *ds, int port,
>>  		dev->tx_ports &= ~(1 << port);
>>
>>  	/* Port membership may share register with STP state. */
>> -	if (member >= 0 && member != p->member)
>> -		ksz9477_cfg_port_member(dev, port, (u8)member);
>> +	for (i = 0; i < SWITCH_PORT_NUM; i++)
>> +		ksz9477_cfg_port_member(dev, i, (u8)member);
>>
>>  	/* Check if forwarding needs to be updated. */
>>  	if (state != BR_STATE_FORWARDING) {
> 
> The original DSA model did not have a way to tell the bridge device not to
> forward the frame, so the switch driver always setup the membership to
> disable forwarding between ports.
> 
> When lan devices are setup they act like individual devices.  A bridge device
> adding them under it will forward the frames.
> 
> The new switchdev model adds the offload_fwd_mark bit to tell the bridge not to
> forward frame.
> 
> The ksz_update_port_member function in ksz_common.c is doing this membership
> setup for all forwarding ports.  It was finally enabled in one of the RFC patches I
> submitted recently (Add switch forward offloading support).
> 
> I think if you do this without setting offload_fwd_mark you will receive duplicate
> frame.

Do you have a git tree with all the KSZ patches based on -next
somewhere, so I don't have to look for them in random MLs ?

-- 
Best regards,
Marek Vasut

RE: [PATCH] net: dsa: ksz: Fix port membership

[Tristram.Ha]

> Do you have a git tree with all the KSZ patches based on -next
> somewhere, so I don't have to look for them in random MLs ?

I just sent it this Monday and the subject for that patch is
"[PATCH RFC 6/6] net: dsa: microchip: Add switch offload forwarding support."

Re: [PATCH] net: dsa: ksz: Fix port membership

[Marek Vasut]

On 12/08/2018 01:13 AM, Tristram.Ha@microchip.com wrote:
>> Do you have a git tree with all the KSZ patches based on -next
>> somewhere, so I don't have to look for them in random MLs ?
> 
> I just sent it this Monday and the subject for that patch is
> "[PATCH RFC 6/6] net: dsa: microchip: Add switch offload forwarding support."

Is all that collected in some git tree somewhere, so I don't have to
look for various patches in varying states of decay throughout the ML?

-- 
Best regards,
Marek Vasut

Re: [PATCH] net: dsa: ksz: Fix port membership

[Andrew Lunn]

On Sat, Dec 08, 2018 at 05:23:25AM +0100, Marek Vasut wrote:
> On 12/08/2018 01:13 AM, Tristram.Ha@microchip.com wrote:
> >> Do you have a git tree with all the KSZ patches based on -next
> >> somewhere, so I don't have to look for them in random MLs ?
> > 
> > I just sent it this Monday and the subject for that patch is
> > "[PATCH RFC 6/6] net: dsa: microchip: Add switch offload forwarding support."
> 
> Is all that collected in some git tree somewhere, so I don't have to
> look for various patches in varying states of decay throughout the ML?

Hi Marek, Tristram

It is unfortunate that we have two patchsets being submitted at the
same time, with overlapping functionality. Some degree of cooperation
is needed to handle this.

Could i ask you both to publish a git tree of your patches. And then
figure out how to submit them. Maybe as smaller sets, with the easy,
non-overlapping bits first?

	Andrew

Re: [PATCH] net: dsa: ksz: Fix port membership

[Marek Vasut]

On 12/08/2018 12:00 PM, Andrew Lunn wrote:
> On Sat, Dec 08, 2018 at 05:23:25AM +0100, Marek Vasut wrote:
>> On 12/08/2018 01:13 AM, Tristram.Ha@microchip.com wrote:
>>>> Do you have a git tree with all the KSZ patches based on -next
>>>> somewhere, so I don't have to look for them in random MLs ?
>>>
>>> I just sent it this Monday and the subject for that patch is
>>> "[PATCH RFC 6/6] net: dsa: microchip: Add switch offload forwarding support."
>>
>> Is all that collected in some git tree somewhere, so I don't have to
>> look for various patches in varying states of decay throughout the ML?
> 
> Hi Marek, Tristram
> 
> It is unfortunate that we have two patchsets being submitted at the
> same time, with overlapping functionality. Some degree of cooperation
> is needed to handle this.
> 
> Could i ask you both to publish a git tree of your patches. And then
> figure out how to submit them. Maybe as smaller sets, with the easy,
> non-overlapping bits first?

See the first three patches I sent separately, ignore the 5 patch series
for now.

-- 
Best regards,
Marek Vasut

Re: [PATCH] net: dsa: ksz: Fix port membership

[Marek Vasut]

On 12/08/2018 12:00 PM, Andrew Lunn wrote:
> On Sat, Dec 08, 2018 at 05:23:25AM +0100, Marek Vasut wrote:
>> On 12/08/2018 01:13 AM, Tristram.Ha@microchip.com wrote:
>>>> Do you have a git tree with all the KSZ patches based on -next
>>>> somewhere, so I don't have to look for them in random MLs ?
>>>
>>> I just sent it this Monday and the subject for that patch is
>>> "[PATCH RFC 6/6] net: dsa: microchip: Add switch offload forwarding support."
>>
>> Is all that collected in some git tree somewhere, so I don't have to
>> look for various patches in varying states of decay throughout the ML?
> 
> Hi Marek, Tristram
> 
> It is unfortunate that we have two patchsets being submitted at the
> same time, with overlapping functionality. Some degree of cooperation
> is needed to handle this.
> 
> Could i ask you both to publish a git tree of your patches. And then
> figure out how to submit them. Maybe as smaller sets, with the easy,
> non-overlapping bits first?

https://git.kernel.org/pub/scm/linux/kernel/git/marex/linux-2.6.git/log/?h=ksz8795-v1

-- 
Best regards,
Marek Vasut

Re: [PATCH] net: dsa: ksz: Fix port membership

[Andrew Lunn]

> I think if you do this without setting offload_fwd_mark you will
> receive duplicate frame.

I don't think it will, at least not in the normal case. The hardware
should know the egress port, so there is no need to forward a copy to
the CPU. The only time it should forward to the CPU is when the egress
port is not known, so it floods. Without offload_fwd_mark set, the SW
bridge will flood it back out the ports causing duplication. But that
is not too bad. The Marvell driver did this for a while and nothing
bad was reported.

    Andrew

RE: [PATCH] net: dsa: ksz: Fix port membership

[Tristram.Ha]

> > I think if you do this without setting offload_fwd_mark you will
> > receive duplicate frame.
> 
> I don't think it will, at least not in the normal case. The hardware
> should know the egress port, so there is no need to forward a copy to
> the CPU. The only time it should forward to the CPU is when the egress
> port is not known, so it floods. Without offload_fwd_mark set, the SW
> bridge will flood it back out the ports causing duplication. But that
> is not too bad. The Marvell driver did this for a while and nothing
> bad was reported.

For unicast frames it is okay as the CPU port does not see it after the first
one.  For multicast frames there will be duplicates, and it is tolerated?