| Message ID | 20230904120209.741207-5-lukma@denx.de (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: hsr: Enable HSR HW offloading for KSZ9477 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Mon, Sep 04, 2023 at 02:02:09PM +0200, Lukasz Majewski wrote: > This patch provides the common KSZ (i.e. Microchip) DSA code with support > for HSR aware devices. > > To be more specific - generic ksz_hsr_{join|leave} functions are provided, > now only supporting KSZ9477 IC. > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > --- > Changes for v2: > - None > > Changes for v3: > - Do not return -EOPNOTSUPP for only PRP_V1 (as v2 will not be caught) > --- > drivers/net/dsa/microchip/ksz_common.c | 69 ++++++++++++++++++++++++++ > 1 file changed, 69 insertions(+) > > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c > index 579fde54d1e1..91d1acaf4494 100644 > --- a/drivers/net/dsa/microchip/ksz_common.c > +++ b/drivers/net/dsa/microchip/ksz_common.c > @@ -16,6 +16,7 @@ > #include <linux/etherdevice.h> > #include <linux/if_bridge.h> > #include <linux/if_vlan.h> > +#include <linux/if_hsr.h> > #include <linux/irq.h> > #include <linux/irqdomain.h> > #include <linux/of_mdio.h> This conflicts with commit f44a90104ee5 ("net: dsa: Explicitly include correct DT includes") from July, merged through net-next. "New features" material for networking goes through this tree, please submit patches that were formatted (and tested) on top of the most recent version of the "main" branch, and use git-send-email --subject-prefix "[RFC PATCH vN net-next]" to denote that. https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/log/ If patches fail to apply to the target kernel, you lose the benefit of automatic build testing (which would have highlighted a problem that exists since v3). With RFC patches, the kbuild test robot sends build breakage reports only to you - with normal patches it sends them to everybody. Please wait for more feedback before posting RFC v5. I will review in more detail, but it will take some time. Thanks.
Hi Vladimir, > On Mon, Sep 04, 2023 at 02:02:09PM +0200, Lukasz Majewski wrote: > > This patch provides the common KSZ (i.e. Microchip) DSA code with > > support for HSR aware devices. > > > > To be more specific - generic ksz_hsr_{join|leave} functions are > > provided, now only supporting KSZ9477 IC. > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > --- > > Changes for v2: > > - None > > > > Changes for v3: > > - Do not return -EOPNOTSUPP for only PRP_V1 (as v2 will not be > > caught) --- > > drivers/net/dsa/microchip/ksz_common.c | 69 > > ++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) > > > > diff --git a/drivers/net/dsa/microchip/ksz_common.c > > b/drivers/net/dsa/microchip/ksz_common.c index > > 579fde54d1e1..91d1acaf4494 100644 --- > > a/drivers/net/dsa/microchip/ksz_common.c +++ > > b/drivers/net/dsa/microchip/ksz_common.c @@ -16,6 +16,7 @@ > > #include <linux/etherdevice.h> > > #include <linux/if_bridge.h> > > #include <linux/if_vlan.h> > > +#include <linux/if_hsr.h> > > #include <linux/irq.h> > > #include <linux/irqdomain.h> > > #include <linux/of_mdio.h> > > This conflicts with commit f44a90104ee5 ("net: dsa: Explicitly include > correct DT includes") from July, merged through net-next. > > "New features" material for networking goes through this tree, please > submit patches that were formatted (and tested) on top of the most > recent version of the "main" branch, and use git-send-email > --subject-prefix "[RFC PATCH vN net-next]" to denote that. > > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/log/ > > If patches fail to apply to the target kernel, you lose the benefit of > automatic build testing (which would have highlighted a problem that > exists since v3). With RFC patches, the kbuild test robot sends build > breakage reports only to you - with normal patches it sends them to > everybody. Thanks for the info. > > Please wait for more feedback before posting RFC v5. I will review in > more detail, but it will take some time. Ok. I will wait for your feedback and then sent RFC v4. > > Thanks. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Mon, Sep 04, 2023 at 02:02:09PM +0200, Lukasz Majewski wrote: > This patch provides the common KSZ (i.e. Microchip) DSA code with support > for HSR aware devices. > > To be more specific - generic ksz_hsr_{join|leave} functions are provided, > now only supporting KSZ9477 IC. > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > --- > Changes for v2: > - None > > Changes for v3: > - Do not return -EOPNOTSUPP for only PRP_V1 (as v2 will not be caught) Should be squashed into patch 3/4. The split does not make the code easier to review for me. > --- > drivers/net/dsa/microchip/ksz_common.c | 69 ++++++++++++++++++++++++++ > 1 file changed, 69 insertions(+) > > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c > index 579fde54d1e1..91d1acaf4494 100644 > --- a/drivers/net/dsa/microchip/ksz_common.c > +++ b/drivers/net/dsa/microchip/ksz_common.c > @@ -16,6 +16,7 @@ > #include <linux/etherdevice.h> > #include <linux/if_bridge.h> > #include <linux/if_vlan.h> > +#include <linux/if_hsr.h> > #include <linux/irq.h> > #include <linux/irqdomain.h> > #include <linux/of_mdio.h> > @@ -3433,6 +3434,72 @@ u16 ksz_hsr_get_ports(struct dsa_switch *ds) > return 0; > } > > +static int ksz_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr) > +{ > + struct dsa_port *partner = NULL, *dp; > + struct ksz_device *dev = ds->priv; > + enum hsr_version ver; > + int ret; > + > + ret = hsr_get_version(hsr, &ver); > + if (ret) > + return ret; > + > + switch (dev->chip_id) { > + case KSZ9477_CHIP_ID: > + if (!(ver == HSR_V0 || ver == HSR_V1)) > + return -EOPNOTSUPP; move the "default: return -EOPNOTSUPP" statement from below here. > + } I don't see any restriction to allow offloading a single HSR device. Looking at patch 3/4, that will obviously not work due to some hardware registers which are global and would be overwritten by the second HSR device. For example, a5psw_port_bridge_join() has a similar restriction to offload a single bridge device. If you return -EOPNOTSUPP, then DSA should fall back to an unoffloaded, 100% software-based HSR device, and that should work too. It would be good if you could verify that the unoffloaded HSR works well after the changes too. > + > + /* We can't enable redundancy on the switch until both > + * redundant ports have signed up. > + */ > + dsa_hsr_foreach_port(dp, ds, hsr) { > + if (dp->index != port) { > + partner = dp; > + break; > + } > + } > + > + if (!partner) > + return 0; > + > + switch (dev->chip_id) { > + case KSZ9477_CHIP_ID: > + return ksz9477_hsr_join(ds, port, hsr, partner); > + default: > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + > +static int ksz_hsr_leave(struct dsa_switch *ds, int port, > + struct net_device *hsr) > +{ > + struct dsa_port *partner = NULL, *dp; > + struct ksz_device *dev = ds->priv; > + > + dsa_hsr_foreach_port(dp, ds, hsr) { > + if (dp->index != port) { > + partner = dp; > + break; > + } > + } > + > + if (!partner) > + return 0; > + > + switch (dev->chip_id) { > + case KSZ9477_CHIP_ID: > + return ksz9477_hsr_leave(ds, port, hsr, partner); > + default: > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + > static const struct dsa_switch_ops ksz_switch_ops = { > .get_tag_protocol = ksz_get_tag_protocol, > .connect_tag_protocol = ksz_connect_tag_protocol, > @@ -3452,6 +3519,8 @@ static const struct dsa_switch_ops ksz_switch_ops = { > .get_sset_count = ksz_sset_count, > .port_bridge_join = ksz_port_bridge_join, > .port_bridge_leave = ksz_port_bridge_leave, > + .port_hsr_join = ksz_hsr_join, > + .port_hsr_leave = ksz_hsr_leave, > .port_stp_state_set = ksz_port_stp_state_set, > .port_pre_bridge_flags = ksz_port_pre_bridge_flags, > .port_bridge_flags = ksz_port_bridge_flags, > -- > 2.20.1 >
Hi Vladimir, > On Mon, Sep 04, 2023 at 02:02:09PM +0200, Lukasz Majewski wrote: > > This patch provides the common KSZ (i.e. Microchip) DSA code with > > support for HSR aware devices. > > > > To be more specific - generic ksz_hsr_{join|leave} functions are > > provided, now only supporting KSZ9477 IC. > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > --- > > Changes for v2: > > - None > > > > Changes for v3: > > - Do not return -EOPNOTSUPP for only PRP_V1 (as v2 will not be > > caught) > > Should be squashed into patch 3/4. The split does not make the code > easier to review for me. So you recommend to have only one patch in which the hsr_join{leave} function from ksz_common.c and ksz9477_hsr_join{leave} from ksz9477.c are added? > > > --- > > drivers/net/dsa/microchip/ksz_common.c | 69 > > ++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) > > > > diff --git a/drivers/net/dsa/microchip/ksz_common.c > > b/drivers/net/dsa/microchip/ksz_common.c index > > 579fde54d1e1..91d1acaf4494 100644 --- > > a/drivers/net/dsa/microchip/ksz_common.c +++ > > b/drivers/net/dsa/microchip/ksz_common.c @@ -16,6 +16,7 @@ > > #include <linux/etherdevice.h> > > #include <linux/if_bridge.h> > > #include <linux/if_vlan.h> > > +#include <linux/if_hsr.h> > > #include <linux/irq.h> > > #include <linux/irqdomain.h> > > #include <linux/of_mdio.h> > > @@ -3433,6 +3434,72 @@ u16 ksz_hsr_get_ports(struct dsa_switch *ds) > > return 0; > > } > > > > +static int ksz_hsr_join(struct dsa_switch *ds, int port, struct > > net_device *hsr) +{ > > + struct dsa_port *partner = NULL, *dp; > > + struct ksz_device *dev = ds->priv; > > + enum hsr_version ver; > > + int ret; > > + > > + ret = hsr_get_version(hsr, &ver); > > + if (ret) > > + return ret; > > + > > + switch (dev->chip_id) { > > + case KSZ9477_CHIP_ID: > > + if (!(ver == HSR_V0 || ver == HSR_V1)) > > + return -EOPNOTSUPP; > > move the "default: return -EOPNOTSUPP" statement from below here. > Ok, I will add default statement with -EOPNOTSUPP. > > + } > > I don't see any restriction to allow offloading a single HSR device. As I've written in the other response - I've followed the xrs700x.c convention. Moreover, for me it seems more natural, that we only allow full HSR support for 2 ports or none. Please be aware, that HSR supposed to support only 2 ports, and having only one working is not recommended by vendor. > Looking at patch 3/4, that will obviously not work due to some > hardware registers which are global and would be overwritten by the > second HSR device. I cannot guarantee that there will not be any "side effects" with this approach. And to be honest - I would prefer to spent time on testing recommended setups. > > For example, a5psw_port_bridge_join() has a similar restriction to > offload a single bridge device. HSR is IMHO a bit different than plain "bridge" offloading. > > If you return -EOPNOTSUPP, then DSA should fall back to an > unoffloaded, 100% software-based HSR device, and that should work > too. And then we would have one port with SW HSR and another one with HW HSR? >It would be good if you could verify that the unoffloaded HSR > works well after the changes too. I've tested on KSZ9477-EVB the SW HSR operation with two ports (and two or three boards) and HW HSR offloading. Results are presented in the cover-letter. > > > + > > + /* We can't enable redundancy on the switch until both > > + * redundant ports have signed up. > > + */ > > + dsa_hsr_foreach_port(dp, ds, hsr) { > > + if (dp->index != port) { > > + partner = dp; > > + break; > > + } > > + } > > + > > + if (!partner) > > + return 0; > > + > > + switch (dev->chip_id) { > > + case KSZ9477_CHIP_ID: > > + return ksz9477_hsr_join(ds, port, hsr, partner); > > + default: > > + return -EOPNOTSUPP; > > + } > > + > > + return 0; > > +} > > + > > +static int ksz_hsr_leave(struct dsa_switch *ds, int port, > > + struct net_device *hsr) > > +{ > > + struct dsa_port *partner = NULL, *dp; > > + struct ksz_device *dev = ds->priv; > > + > > + dsa_hsr_foreach_port(dp, ds, hsr) { > > + if (dp->index != port) { > > + partner = dp; > > + break; > > + } > > + } > > + > > + if (!partner) > > + return 0; > > + > > + switch (dev->chip_id) { > > + case KSZ9477_CHIP_ID: > > + return ksz9477_hsr_leave(ds, port, hsr, partner); > > + default: > > + return -EOPNOTSUPP; > > + } > > + > > + return 0; > > +} > > + > > static const struct dsa_switch_ops ksz_switch_ops = { > > .get_tag_protocol = ksz_get_tag_protocol, > > .connect_tag_protocol = ksz_connect_tag_protocol, > > @@ -3452,6 +3519,8 @@ static const struct dsa_switch_ops > > ksz_switch_ops = { .get_sset_count = ksz_sset_count, > > .port_bridge_join = ksz_port_bridge_join, > > .port_bridge_leave = ksz_port_bridge_leave, > > + .port_hsr_join = ksz_hsr_join, > > + .port_hsr_leave = ksz_hsr_leave, > > .port_stp_state_set = ksz_port_stp_state_set, > > .port_pre_bridge_flags = ksz_port_pre_bridge_flags, > > .port_bridge_flags = ksz_port_bridge_flags, > > -- > > 2.20.1 > > > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Tue, Sep 05, 2023 at 01:23:51PM +0200, Lukasz Majewski wrote: > > Should be squashed into patch 3/4. The split does not make the code > > easier to review for me. > > So you recommend to have only one patch in which the hsr_join{leave} > function from ksz_common.c and ksz9477_hsr_join{leave} from ksz9477.c > are added? Correct. In addition, patch 1/4 will be dropped. So there will be 2 patches, one on net/dsa/tag_ksz.c and the other on drivers/net/dsa/microchip/. > > I don't see any restriction to allow offloading a single HSR device. > > As I've written in the other response - I've followed the xrs700x.c > convention. "the xrs700x.c convention" xrs700x_hsr_join(): /* Only ports 1 and 2 can be HSR/PRP redundant ports. */ if (port != 1 && port != 2) return -EOPNOTSUPP; So, checking for ports 1 and 2 is a stronger condition than the one I'm asking you to enforce. The KSZ9477 is more flexible. It can enable HSR offload on any 2 ports, not just on ports 1 and 2. But it can still be 2 ports max, no more. You haven't copied the xrs700x convention, but you haven't adapted it, either. > Moreover, for me it seems more natural, that we only allow full HSR > support for 2 ports or none. Please be aware, that HSR supposed to > support only 2 ports, and having only one working is not recommended by > vendor. And where is it enforced that full HSR offload is only applied to 2 ports or none? > > Looking at patch 3/4, that will obviously not work due to some > > hardware registers which are global and would be overwritten by the > > second HSR device. > > I cannot guarantee that there will not be any "side effects" with this > approach. And to be honest - I would prefer to spent time on testing > recommended setups. Please read my reply again, keeping in mind that by "HSR device" I mean "hsr0" in the commands below, and not the member ports as you've assumed when responding. > > > > For example, a5psw_port_bridge_join() has a similar restriction to > > offload a single bridge device. > > HSR is IMHO a bit different than plain "bridge" offloading. Maybe this was not clear, but by "similar" I mean: if you replace the "bridge" word with "hsr", and you copy that code snippet from a5psw, you get the desired outcome. static int ksz_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr, /* optionally pass the extack argument from the caller */) { struct ksz_device *dev = ds->priv; /* We only support 1 HSR device */ if (dev->hsr_dev && hsr != dev->hsr_dev) { NL_SET_ERR_MSG_MOD(extack, "Offload supported for a single HSR"); return -EOPNOTSUPP; } dev->hsr_dev = hsr; ... return 0; } I did not imply that HSR is not different than bridge offloading. I don't see how that is even related to the discussion. > > If you return -EOPNOTSUPP, then DSA should fall back to an > > unoffloaded, 100% software-based HSR device, and that should work > > too. > > And then we would have one port with SW HSR and another one with HW > HSR? No. One HSR device (hsr0, with 2 member ports) with offload and one HSR device (hsr1, with 2 member ports) without offload (see (b) below). > >It would be good if you could verify that the unoffloaded HSR > > works well after the changes too. > > I've tested on KSZ9477-EVB the SW HSR operation with two ports (and two > or three boards) and HW HSR offloading. Results are presented in the > cover-letter. "results in the cover letter" Results: With KSZ9477 offloading support added: RX: 100 Mbps TX: 98 Mbps With no offloading RX: 63 Mbps TX: 63 Mbps What was the setup for the "no offloading" case? (a) kernel did not contain the offloading patch set (b) the testing was on hsr1, in the situation below: ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision 45 version 1 # offloaded ip link add name hsr1 type hsr slave1 lan3 slave2 lan4 supervision 45 version 1 # unoffloaded (d) in some other way (please detail) I was specifically asking about situation (b).
> > And then we would have one port with SW HSR and another one with HW > > HSR? > > No. One HSR device (hsr0, with 2 member ports) with offload and one > HSR device (hsr1, with 2 member ports) without offload (see (b) below). I just wanted to comment that offloading is about taking what Linux can do in software and getting the hardware to do it. Linux should happily allow two HSR devices, working in software. If you can only offload one of them, Linux should continue to do the other one in software. So please do follow what Vladimir is suggesting. Andrew
Hi Vladimir, > On Tue, Sep 05, 2023 at 01:23:51PM +0200, Lukasz Majewski wrote: > > > Should be squashed into patch 3/4. The split does not make the > > > code easier to review for me. > > > > So you recommend to have only one patch in which the hsr_join{leave} > > function from ksz_common.c and ksz9477_hsr_join{leave} from > > ksz9477.c are added? > > Correct. In addition, patch 1/4 will be dropped. So there will be 2 > patches, one on net/dsa/tag_ksz.c and the other on > drivers/net/dsa/microchip/. Ok. > > > > I don't see any restriction to allow offloading a single HSR > > > device. > > > > As I've written in the other response - I've followed the xrs700x.c > > convention. > > "the xrs700x.c convention" > > xrs700x_hsr_join(): > > /* Only ports 1 and 2 can be HSR/PRP redundant ports. */ > if (port != 1 && port != 2) > return -EOPNOTSUPP; > > So, checking for ports 1 and 2 is a stronger condition than the one > I'm asking you to enforce. > > The KSZ9477 is more flexible. It can enable HSR offload on any 2 > ports, not just on ports 1 and 2. But it can still be 2 ports max, no > more. You haven't copied the xrs700x convention, but you haven't > adapted it, either. Ok. I've misuderstood your suggestion. You were asking about having one hsr offloaded setup (hsr0 => lan1 + lan2) and in the same time non-offloaded setup (hsr1 => lan3 + lan4). > > > Moreover, for me it seems more natural, that we only allow full HSR > > support for 2 ports or none. Please be aware, that HSR supposed to > > support only 2 ports, and having only one working is not > > recommended by vendor. > > And where is it enforced that full HSR offload is only applied to 2 > ports or none? In the ksz_jsr_join() at ksz_common.c -> we exit from this function when !partner. > > > > Looking at patch 3/4, that will obviously not work due to some > > > hardware registers which are global and would be overwritten by > > > the second HSR device. > > > > I cannot guarantee that there will not be any "side effects" with > > this approach. And to be honest - I would prefer to spent time on > > testing recommended setups. > > Please read my reply again, keeping in mind that by "HSR device" I > mean "hsr0" in the commands below, and not the member ports as you've > assumed when responding. Yes. I do get your point. > > > > > > > For example, a5psw_port_bridge_join() has a similar restriction to > > > offload a single bridge device. > > > > HSR is IMHO a bit different than plain "bridge" offloading. > > Maybe this was not clear, but by "similar" I mean: if you replace the > "bridge" word with "hsr", and you copy that code snippet from a5psw, > you get the desired outcome. > > static int ksz_hsr_join(struct dsa_switch *ds, int port, struct > net_device *hsr, /* optionally pass the extack argument from the > caller */) { > struct ksz_device *dev = ds->priv; > > /* We only support 1 HSR device */ > if (dev->hsr_dev && hsr != dev->hsr_dev) { > NL_SET_ERR_MSG_MOD(extack, > "Offload supported for a single > HSR"); return -EOPNOTSUPP; > } > Ok. > dev->hsr_dev = hsr; > > ... > > return 0; > } > > I did not imply that HSR is not different than bridge offloading. > I don't see how that is even related to the discussion. > > > > If you return -EOPNOTSUPP, then DSA should fall back to an > > > unoffloaded, 100% software-based HSR device, and that should work > > > too. > > > > And then we would have one port with SW HSR and another one with HW > > HSR? > > No. One HSR device (hsr0, with 2 member ports) with offload and one > HSR device (hsr1, with 2 member ports) without offload (see (b) > below). > > > >It would be good if you could verify that the unoffloaded HSR > > > works well after the changes too. > > > > I've tested on KSZ9477-EVB the SW HSR operation with two ports (and > > two or three boards) and HW HSR offloading. Results are presented > > in the cover-letter. > > "results in the cover letter" > > Results: > With KSZ9477 offloading support added: RX: 100 Mbps TX: 98 Mbps > With no offloading RX: 63 Mbps TX: 63 Mbps > > What was the setup for the "no offloading" case? > I used two boards. I've used the same kernel with and without HSR offloading patches. Cables were connected to port1 and port2 on each board. Non HSR network was connected to port3. > (a) kernel did not contain the offloading patch set > Yes. > (b) the testing was on hsr1, in the situation below: > > ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision 45 > version 1 # offloaded ip link add name hsr1 type hsr slave1 lan3 > slave2 lan4 supervision 45 version 1 # unoffloaded > I did not setup two hsr devices on the same board. I've used two boards with hsr0 setup on each. > (d) in some other way (please detail) > > I was specifically asking about situation (b). I did not tested the hsr0, hsr1 as my (real) use case is with KSZ9477 having only 3 ports available for connection (port 1,2,3). Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Tue, Sep 05, 2023 at 03:47:44PM +0200, Lukasz Majewski wrote: > > > Moreover, for me it seems more natural, that we only allow full HSR > > > support for 2 ports or none. Please be aware, that HSR supposed to > > > support only 2 ports, and having only one working is not > > > recommended by vendor. > > > > And where is it enforced that full HSR offload is only applied to 2 > > ports or none? > > In the ksz_jsr_join() at ksz_common.c -> we exit from this function > when !partner. And what about 4 or 6 ports being placed as members of 2 or 3 HSR devices? How does the !partner condition help there? > > Results: > > With KSZ9477 offloading support added: RX: 100 Mbps TX: 98 Mbps > > With no offloading RX: 63 Mbps TX: 63 Mbps > > > > What was the setup for the "no offloading" case? > > > > I used two boards. I've used the same kernel with and without HSR > offloading patches. > > Cables were connected to port1 and port2 on each board. Non HSR network > was connected to port3. > > > (a) kernel did not contain the offloading patch set > > > > Yes. Ok, then it would be good to check that your patch set does not break something that used to work (software HSR - easiest to check with a second pair of ports, but if not possible, it can also be emulated by returning -EOPNOTSUPP in .port_hsr_join on an artificial other condition). > > (b) the testing was on hsr1, in the situation below: > > > > ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision 45 > > version 1 # offloaded ip link add name hsr1 type hsr slave1 lan3 > > slave2 lan4 supervision 45 version 1 # unoffloaded > > I did not setup two hsr devices on the same board. I've used two boards > with hsr0 setup on each. Ok, but can you? > > (d) in some other way (please detail) > > > > I was specifically asking about situation (b). > > I did not tested the hsr0, hsr1 as my (real) use case is with KSZ9477 > having only 3 ports available for connection (port 1,2,3). ksz_chip_data :: port_cnt is set to 7 for KSZ9477. I guess that the limitation of having only 3 user ports for testing is specific to your board, and not to the entire switch as may be seen on other boards, correct? It doesn't mean that the "single HSR device" restriction shouldn't be enforced.
On Mon, Sep 04, 2023 at 02:02:09PM +0200, Lukasz Majewski wrote: > +static int ksz_hsr_leave(struct dsa_switch *ds, int port, > + struct net_device *hsr) > +{ > + struct dsa_port *partner = NULL, *dp; > + struct ksz_device *dev = ds->priv; > + > + dsa_hsr_foreach_port(dp, ds, hsr) { > + if (dp->index != port) { > + partner = dp; > + break; > + } > + } > + > + if (!partner) > + return 0; > + > + switch (dev->chip_id) { > + case KSZ9477_CHIP_ID: > + return ksz9477_hsr_leave(ds, port, hsr, partner); > + default: > + return -EOPNOTSUPP; > + } > + > + return 0; > +} It's hard for me to put this in the proper perspective in this email, since ksz9477_hsr_leave() is implemented in a different patch, so I'm just going to reproduce it here: int ksz9477_hsr_leave(struct dsa_switch *ds, int port, struct net_device *hsr, struct dsa_port *partner) { struct ksz_device *dev = ds->priv; /* Clear ports HSR support */ ksz_write32(dev, REG_HSR_PORT_MAP__4, 0); /* Disable forwarding frames between HSR ports */ ksz_prmw32(dev, port, REG_PORT_VLAN_MEMBERSHIP__4, dev->hsr_ports, 0); ksz_prmw32(dev, partner->index, REG_PORT_VLAN_MEMBERSHIP__4, dev->hsr_ports, 0); /* Disable per port self-address filtering */ ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL, PORT_SRC_ADDR_FILTER, false); ksz_port_cfg(dev, partner->index, REG_PORT_LUE_CTRL, PORT_SRC_ADDR_FILTER, false); return 0; } The code pattern from ksz_hsr_leave() is to disable HSR offload in both member ports, after *both* member ports have left the HSR device, correct? So it means that after this set of commands: ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision 45 version 1 ip link set dev lan1 up ip link set dev lan2 up ip link set lan1 nomaster lan1 will still have HSR offload enabled, and forwarding enabled towards lan2, correct? That's not good, because lan1 is now a standalone port and should operate as such.
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index 579fde54d1e1..91d1acaf4494 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -16,6 +16,7 @@ #include <linux/etherdevice.h> #include <linux/if_bridge.h> #include <linux/if_vlan.h> +#include <linux/if_hsr.h> #include <linux/irq.h> #include <linux/irqdomain.h> #include <linux/of_mdio.h> @@ -3433,6 +3434,72 @@ u16 ksz_hsr_get_ports(struct dsa_switch *ds) return 0; } +static int ksz_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr) +{ + struct dsa_port *partner = NULL, *dp; + struct ksz_device *dev = ds->priv; + enum hsr_version ver; + int ret; + + ret = hsr_get_version(hsr, &ver); + if (ret) + return ret; + + switch (dev->chip_id) { + case KSZ9477_CHIP_ID: + if (!(ver == HSR_V0 || ver == HSR_V1)) + return -EOPNOTSUPP; + } + + /* We can't enable redundancy on the switch until both + * redundant ports have signed up. + */ + dsa_hsr_foreach_port(dp, ds, hsr) { + if (dp->index != port) { + partner = dp; + break; + } + } + + if (!partner) + return 0; + + switch (dev->chip_id) { + case KSZ9477_CHIP_ID: + return ksz9477_hsr_join(ds, port, hsr, partner); + default: + return -EOPNOTSUPP; + } + + return 0; +} + +static int ksz_hsr_leave(struct dsa_switch *ds, int port, + struct net_device *hsr) +{ + struct dsa_port *partner = NULL, *dp; + struct ksz_device *dev = ds->priv; + + dsa_hsr_foreach_port(dp, ds, hsr) { + if (dp->index != port) { + partner = dp; + break; + } + } + + if (!partner) + return 0; + + switch (dev->chip_id) { + case KSZ9477_CHIP_ID: + return ksz9477_hsr_leave(ds, port, hsr, partner); + default: + return -EOPNOTSUPP; + } + + return 0; +} + static const struct dsa_switch_ops ksz_switch_ops = { .get_tag_protocol = ksz_get_tag_protocol, .connect_tag_protocol = ksz_connect_tag_protocol, @@ -3452,6 +3519,8 @@ static const struct dsa_switch_ops ksz_switch_ops = { .get_sset_count = ksz_sset_count, .port_bridge_join = ksz_port_bridge_join, .port_bridge_leave = ksz_port_bridge_leave, + .port_hsr_join = ksz_hsr_join, + .port_hsr_leave = ksz_hsr_leave, .port_stp_state_set = ksz_port_stp_state_set, .port_pre_bridge_flags = ksz_port_pre_bridge_flags, .port_bridge_flags = ksz_port_bridge_flags,
This patch provides the common KSZ (i.e. Microchip) DSA code with support for HSR aware devices. To be more specific - generic ksz_hsr_{join|leave} functions are provided, now only supporting KSZ9477 IC. Signed-off-by: Lukasz Majewski <lukma@denx.de> --- Changes for v2: - None Changes for v3: - Do not return -EOPNOTSUPP for only PRP_V1 (as v2 will not be caught) --- drivers/net/dsa/microchip/ksz_common.c | 69 ++++++++++++++++++++++++++ 1 file changed, 69 insertions(+)