| Message ID | 20181207174424.10989-1-marex@denx.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: ksz: Fix port membership | expand |
> 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.
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.
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 ?
> 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
> >> 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.
> > 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?
> 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."
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?
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
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.
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
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) {
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(-)