| Message ID | 20240619134248.1228443-1-lukma@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net-next] net: dsa: Allow only up to two HSR HW offloaded ports for KSZ9477 | expand |
You went above and beyond. Thanks so much! :)
Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
regards,
dan carpenter
> + /* KSZ9477 can only perform HSR offloading for up to two ports */ > + if (hweight8(dev->hsr_ports) >= 2) { > + NL_SET_ERR_MSG_MOD(extack, > + "Cannot offload more than two ports - use software HSR"); Bit of a nit pick. 'use' suggests it is a directive, you need to changing the configuration to make it work. 'using' would indicate nothing needs changing, it has decided to use software HSR for you. Other than that: Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Wed, Jun 19, 2024 at 03:42:48PM +0200, Lukasz Majewski wrote: > The KSZ9477 allows HSR in-HW offloading for any of two selected ports. > This patch adds check if one tries to use more than two ports with > HSR offloading enabled. > > The problem is with RedBox configuration (HSR-SAN) - when configuring: > ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 interlink lan3 \ > supervision 45 version 1 > > The lan1 (port0) and lan2 (port1) are correctly configured as ports, which > can use HSR offloading on ksz9477. > > However, when we do already have two bits set in hsr_ports, we need to > return (-ENOTSUPP), so the interlink port (lan3) would be used with EOPNOTSUPP > SW based HSR RedBox support. > > Otherwise, I do see some strange network behavior, as some HSR frames are > visible on non-HSR network and vice versa. > > This causes the switch connected to interlink port (lan3) to drop frames > and no communication is possible. > > Moreover, conceptually - the interlink (i.e. HSR-SAN port - lan3/port2) > shall be only supported in software as it is also possible to use ksz9477 > with only SW based HSR (i.e. port0/1 -> hsr0 with offloading, port2 -> > HSR-SAN/interlink, port4/5 -> hsr1 with SW based HSR). > > Fixes: 5055cccfc2d1 ("net: hsr: Provide RedBox support (HSR-SAN)") > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > --- > Changes for v2: > - Add more verbose description with Fixes: tag > - Check the condition earlier and remove extra check if SoC is ksz9477 > - Add comment in the source code file > --- > drivers/net/dsa/microchip/ksz_common.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c > index 2818e24e2a51..72bb419e34b0 100644 > --- a/drivers/net/dsa/microchip/ksz_common.c > +++ b/drivers/net/dsa/microchip/ksz_common.c > @@ -3906,6 +3906,13 @@ static int ksz_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr, > return -EOPNOTSUPP; > } > > + /* KSZ9477 can only perform HSR offloading for up to two ports */ > + if (hweight8(dev->hsr_ports) >= 2) { > + NL_SET_ERR_MSG_MOD(extack, > + "Cannot offload more than two ports - use software HSR"); > + return -EOPNOTSUPP; > + } > + > /* Self MAC address filtering, to avoid frames traversing > * the HSR ring more than once. > */ > -- > 2.20.1 > How do you know you are rejecting the offloading of the interlink port, and not of one of the ring ports? Basing this off of calling order is fragile, and does not actually reflect the hardware limitation.
Hi Vladimir, > On Wed, Jun 19, 2024 at 03:42:48PM +0200, Lukasz Majewski wrote: > > The KSZ9477 allows HSR in-HW offloading for any of two selected > > ports. This patch adds check if one tries to use more than two > > ports with HSR offloading enabled. > > > > The problem is with RedBox configuration (HSR-SAN) - when > > configuring: ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 > > interlink lan3 \ supervision 45 version 1 > > > > The lan1 (port0) and lan2 (port1) are correctly configured as > > ports, which can use HSR offloading on ksz9477. > > > > However, when we do already have two bits set in hsr_ports, we need > > to return (-ENOTSUPP), so the interlink port (lan3) would be used > > with > > EOPNOTSUPP > > > SW based HSR RedBox support. > > > > Otherwise, I do see some strange network behavior, as some HSR > > frames are visible on non-HSR network and vice versa. > > > > This causes the switch connected to interlink port (lan3) to drop > > frames and no communication is possible. > > > > Moreover, conceptually - the interlink (i.e. HSR-SAN port - > > lan3/port2) shall be only supported in software as it is also > > possible to use ksz9477 with only SW based HSR (i.e. port0/1 -> > > hsr0 with offloading, port2 -> HSR-SAN/interlink, port4/5 -> hsr1 > > with SW based HSR). > > > > Fixes: 5055cccfc2d1 ("net: hsr: Provide RedBox support (HSR-SAN)") > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > > > --- > > Changes for v2: > > - Add more verbose description with Fixes: tag > > - Check the condition earlier and remove extra check if SoC is > > ksz9477 > > - Add comment in the source code file > > --- > > drivers/net/dsa/microchip/ksz_common.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/net/dsa/microchip/ksz_common.c > > b/drivers/net/dsa/microchip/ksz_common.c index > > 2818e24e2a51..72bb419e34b0 100644 --- > > a/drivers/net/dsa/microchip/ksz_common.c +++ > > b/drivers/net/dsa/microchip/ksz_common.c @@ -3906,6 +3906,13 @@ > > static int ksz_hsr_join(struct dsa_switch *ds, int port, struct > > net_device *hsr, return -EOPNOTSUPP; } > > > > + /* KSZ9477 can only perform HSR offloading for up to two > > ports */ > > + if (hweight8(dev->hsr_ports) >= 2) { > > + NL_SET_ERR_MSG_MOD(extack, > > + "Cannot offload more than two > > ports - use software HSR"); > > + return -EOPNOTSUPP; > > + } > > + > > /* Self MAC address filtering, to avoid frames traversing > > * the HSR ring more than once. > > */ > > -- > > 2.20.1 > > > > How do you know you are rejecting the offloading of the interlink > port, and not of one of the ring ports? It seems like iproute2 is providing the correct ordering (and assures that lan3/port2 is called as a third one - please see below). > Basing this off of calling > order is fragile, In the accepted form - yes - the interlink port is called as a third one, so it is refused to configure it in the same way as ports, which are supporting HSR in HW (i.e. offloading). > and does not actually reflect the hardware > limitation. root@sama5d3-xplained-sd:~# ip link add name hsr0 type hsr slave1 lan1 interlink lan3 slave2 lan2 supervision 45 version 1 ksz-switch spi1.0 lan1: entered promiscuous mode ksz-switch spi1.0 lan2: entered promiscuous mode ksz-switch spi1.0 lan3: entered promiscuous mode port: 2 Warning: ksz_switch: Cannot offload more than two ports - using software HSR. Gives the same output as: root@sama5d3-xplained-sd:~# ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 interlink lan3 supervision 45 version 1 ksz-switch spi1.0 lan1: entered promiscuous mode ksz-switch spi1.0 lan2: entered promiscuous mode ksz-switch spi1.0 lan3: entered promiscuous mode port: 2 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 Wed, Jun 19, 2024 at 05:10:57PM +0200, Lukasz Majewski wrote: > > How do you know you are rejecting the offloading of the interlink > > port, and not of one of the ring ports? > > It seems like iproute2 is providing the correct ordering (and assures > that lan3/port2 is called as a third one - please see below). This is not iproute2 providing the correct ordering, but rather hsr_dev_finalize() in the kernel calling hsr_add_port() in a certain order that matches what is expected in ksz9477. Granted, this isn't an actual functional problem, but given that you are fixing a newly developed feature for net-next, and that this is API that gets progressively harder to change as more devices implement offloads, I would expect a more obvious signaling mechanism to exist for this, and now seems a good time to do it, rather than opting for the most minimal fix. One way to make the restriction more elegantly obvious (both to the user and to the kernel developer) that it is about interlink ports rather than the number of ports in general is to carry the port type in a structure similar to struct netdev_lag_upper_info. You would have to make hsr_portdev_setup() call something else rather than netdev_upper_dev_link(), because that eats the "void *upper_info" argument when calling __netdev_upper_dev_link(). Possibly create a new netdev_upper_dev_link_info() ("link upper dev with extra info") function, which only HSR calls with a new info structure. Then, the DSA core has access to that and implicitly to the port type, and from there on, you can apply the proper restriction.
On Wed, Jun 19, 2024 at 06:48:14PM +0300, Vladimir Oltean wrote: > Granted, this isn't an actual functional problem, but given that you > are fixing a newly developed feature for net-next, and that this is API > that gets progressively harder to change as more devices implement > offloads, I would expect a more obvious signaling mechanism to exist > for this, and now seems a good time to do it, rather than opting for the > most minimal fix. Actually I'm not even so sure about this basic fact, that it isn't a functional problem already. xrs700x_hsr_join() has explicit checks for port 1 and 2. Obviously it expects those ports to be ring ports. But if you configure from user space ports 0 and 1 to be ring ports, and port 2 to be an interlink port, the kernel will accept that configuration. It will return -EOPNOTSUPP for port 0, falling back to software mode for the first ring port, then accept offload for ring ports 1 and 2. But it doesn't match what user space requested, because port 2 should be interlink... I think you really should pass the port type down to drivers and reject offloading interlink ports...
On Wed, Jun 19, 2024 at 06:48:14PM +0300, Vladimir Oltean wrote: > You would have to make hsr_portdev_setup() call something else rather > than netdev_upper_dev_link(), because that eats the "void *upper_info" > argument when calling __netdev_upper_dev_link(). Possibly create a new > netdev_upper_dev_link_info() ("link upper dev with extra info") > function, which only HSR calls with a new info structure. Then, the DSA > core has access to that and implicitly to the port type, and from there > on, you can apply the proper restriction. Yet another comment. TL;DR: I think we should also make HSR use netdev_master_upper_dev_link() anyway (as a separate change), and thus, no new API is needed, since that function is able to pass a void *upper_info already. Explanation: "master" uppers are called like that because any lower interface can have only at most one. Bridge, team, bond, etc are all "master" uppers. So an interface cannot have 2 bridge uppers, or a team and a bond upper at the same time, etc. Compare this to regular upper interfaces like VLANs. You can have as many VLAN upper interfaces as you want (including if you already have a master upper interface). The point is that, AFAIU, the "master" upper restriction comes from the use of an rx_handler. You can't chain RX handlers, and a lower netdev can have a single one, so if the upper netdev uses an RX handler, it'd better make sure that no one else does, or it does but in a coordinated manner. Upper drivers like macvlan do use RX handlers, and are not "master" uppers nonetheless. This is not a contradiction in terms, because they take care to set up the RX handler of the lower interface only once. HSR is not as careful, and uses an rx_handler very plainly, but also does not mark itself as a "master" upper. An attempt to put a physical interface under a HSR upper, and then under a second one (without removing it from the first one), would fail the second time around due to hsr_portdev_setup() -> netdev_rx_handler_register() -> netdev_is_rx_handler_busy() -> -EBUSY. Nor does that configuration appear to make too much sense to me, so you could mark HSR as master, in order for that second attempt to fail one step earlier: hsr_portdev_setup() -> netdev_master_upper_dev_link() -> __netdev_master_upper_dev_get() -> -EBUSY. With that in place, you also have free access now to the "upper_info" parameter, for what was discussed earlier to be propagated down.
Hi Vladimir, > On Wed, Jun 19, 2024 at 06:48:14PM +0300, Vladimir Oltean wrote: > > Granted, this isn't an actual functional problem, but given that you > > are fixing a newly developed feature for net-next, and that this is > > API that gets progressively harder to change as more devices > > implement offloads, I would expect a more obvious signaling > > mechanism to exist for this, and now seems a good time to do it, > > rather than opting for the most minimal fix. > > Actually I'm not even so sure about this basic fact, that it isn't a > functional problem already. > > xrs700x_hsr_join() has explicit checks for port 1 and 2. Obviously it > expects those ports to be ring ports. Yes. > > But if you configure from user space ports 0 and 1 to be ring ports, > and port 2 to be an interlink port, the kernel will accept that > configuration. Yes. > It will return -EOPNOTSUPP for port 0, This comment is for xrs700x_hsr_join()? For the ksz_hsr_join() we do explicitly check for the KSZ9477_CHIP_ID. I do regard this fix as a ksz9477 specific one, as there are some issues (IMHO - this is the "unexpected behaviour" case for this IC) when we add interlink to SoC VLAN. I don't understand why you bring up xrs700x case here? Is it to get a "broader context"? > falling back to > software mode for the first ring port, then accept offload for ring > ports 1 and 2. But it doesn't match what user space requested, because > port 2 should be interlink... Please correct me if I'm wrong, but this seems to not be the case for ksz9477 - as I stated in the other mail - the ordering is correct (I've checked it). > > I think you really should pass the port type down to drivers and > reject offloading interlink ports... As stated above - IMHO I do provide a fix for this particular IC (KSZ9477). With xrs700x we do have fixed ports supporting HSR (port 1,2), so there is no other choice. As a result the HSR Interlink would be supporting only SW emulation. 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 Thu, Jun 20, 2024 at 09:59:20AM +0200, Lukasz Majewski wrote: > > It will return -EOPNOTSUPP for port 0, > > This comment is for xrs700x_hsr_join()? Yes. > For the ksz_hsr_join() we do explicitly check for the KSZ9477_CHIP_ID. > > I do regard this fix as a ksz9477 specific one, as there are some > issues (IMHO - this is the "unexpected behaviour" case for this IC) when > we add interlink to SoC VLAN. > > I don't understand why you bring up xrs700x case here? Is it to get a > "broader context"? You have the Fixes: tag set to a HSR driver change, the fix to which you provide in an offloading device driver. What I'm trying to tell you is to look around and see that KSZ9477 is not the only one which is confused by the addition of an interlink port. So is XRS700X, yet for another reason. > > falling back to > > software mode for the first ring port, then accept offload for ring > > ports 1 and 2. But it doesn't match what user space requested, because > > port 2 should be interlink... > > Please correct me if I'm wrong, but this seems to not be the case for > ksz9477 - as I stated in the other mail - the ordering is correct (I've > checked it). I was never claiming it to be about KSZ9477. > > I think you really should pass the port type down to drivers and > > reject offloading interlink ports... > > As stated above - IMHO I do provide a fix for this particular IC > (KSZ9477). With xrs700x we do have fixed ports supporting HSR (port > 1,2), so there is no other choice. As a result the HSR Interlink would > be supporting only SW emulation. But there is another choice, and I think I've already explained it. HSR_PT_SLAVE_A HSR_PT_SLAVE_B HSR_PT_INTERLINK ---------------------------------------------------------------- user space 0 1 2 requests ---------------------------------------------------------------- XRS700X driver 1 2 - understands I am bringing this as an argument for the fact that you should pass the port type explicitly from HSR to the offload, and use it throughout the offloading drivers. The hweight(ports) >= 2 happens to work for KSZ9477, but IMO misidentifies the problem as having to do with the number of ports rather than the port type. Because of this, a largely similar issue introduced by the same blamed commit but in XRS700X is left unaddressed and unidentified (the fixed ports check which is already present masks the fact that it's not really about the ports, but their type, which must still be checked, otherwise the driver has no idea what HSR wants from it).
Hi Vladimir, > On Thu, Jun 20, 2024 at 09:59:20AM +0200, Lukasz Majewski wrote: > > > It will return -EOPNOTSUPP for port 0, > > > > This comment is for xrs700x_hsr_join()? > > Yes. > > > For the ksz_hsr_join() we do explicitly check for the > > KSZ9477_CHIP_ID. > > > > I do regard this fix as a ksz9477 specific one, as there are some > > issues (IMHO - this is the "unexpected behaviour" case for this IC) > > when we add interlink to SoC VLAN. > > > > I don't understand why you bring up xrs700x case here? Is it to get > > a "broader context"? > > You have the Fixes: tag set to a HSR driver change, the fix to which > you provide in an offloading device driver. What I'm trying to tell > you is to look around and see that KSZ9477 is not the only one which > is confused by the addition of an interlink port. As of now - the HSR interlink was tested with hsr_redbox.sh script with QEMU setup and with KSZ9477 IC with and without offloading enabled. > So is XRS700X, yet > for another reason. > > > > falling back to > > > software mode for the first ring port, then accept offload for > > > ring ports 1 and 2. But it doesn't match what user space > > > requested, because port 2 should be interlink... > > > > Please correct me if I'm wrong, but this seems to not be the case > > for ksz9477 - as I stated in the other mail - the ordering is > > correct (I've checked it). > > I was never claiming it to be about KSZ9477. Ok. > > > > I think you really should pass the port type down to drivers and > > > reject offloading interlink ports... > > > > As stated above - IMHO I do provide a fix for this particular IC > > (KSZ9477). With xrs700x we do have fixed ports supporting HSR (port > > 1,2), so there is no other choice. As a result the HSR Interlink > > would be supporting only SW emulation. > > But there is another choice, and I think I've already explained it. > > HSR_PT_SLAVE_A HSR_PT_SLAVE_B HSR_PT_INTERLINK > ---------------------------------------------------------------- > user > space 0 1 2 > requests > ---------------------------------------------------------------- > XRS700X > driver 1 2 - > understands > > I am bringing this as an argument for the fact that you should pass > the port type explicitly from HSR to the offload, and use it > throughout the offloading drivers. The hweight(ports) >= 2 happens to > work for KSZ9477, And hence it is added to ksz_hsr_join() function, which for now only checks if we use this particular IC. > but IMO misidentifies the problem as having to do > with the number of ports rather than the port type. In general I do understand your concerns - however, as I've stated this patch fixes oddity of the KSZ9477. I can test it with it. > Because of this, > a largely similar issue introduced by the same blamed commit but in > XRS700X is left unaddressed and unidentified (the fixed ports check > which is already present masks the fact that it's not really about > the ports, but their type, which must still be checked, otherwise the > driver has no idea what HSR wants from it). To keep it short: I do see your point, but I believe that it is out of the scope for this particular patch. 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 Thu, Jun 20, 2024 at 02:00:44PM +0200, Lukasz Majewski wrote: > In general I do understand your concerns - however, as I've stated this > patch fixes oddity of the KSZ9477. I can test it with it. > To keep it short: I do see your point, but I believe that it is out of > the scope for this particular patch. So that's it? Can't test with anything other than KSZ9477 => don't care about anything else, and will ignore review feedback, even if the static analysis of the code plausibly points to a more widespread issue? As the author of commit 5055cccfc2d1 ("net: hsr: Provide RedBox support (HSR-SAN)"), who do you think should be responsible of taking care that it plays well with existing offloading drivers?
Hi Vladimir, > On Thu, Jun 20, 2024 at 02:00:44PM +0200, Lukasz Majewski wrote: > > In general I do understand your concerns - however, as I've stated > > this patch fixes oddity of the KSZ9477. I can test it with it. > > > To keep it short: I do see your point, but I believe that it is out > > of the scope for this particular patch. > > So that's it? Can't test with anything other than KSZ9477 => don't > care about anything else, For this particular code the QEMU tests were specially developed (hsr_redbox.sh). Moreover, I've tested it on a real HW (the SW emulation of HSR) - EVB-KSZ9477. Additionally, the code was tested with offloaded case. For all this stuff reproduction steps were provided with commit messages. I would not call this "don't care about anything else" case... Going further - the response for those patches - in terms of comments was not as big as expected. > and will ignore review feedback, As I stated before - I do understand your concerns. However, I do believe that with this patch I do address issue for KSZ9477. > even if > the static analysis of the code plausibly points to a more widespread > issue? I don't have xrs700x to test. Shall I spend time on fixing some perceived issue for IC which I don't have? Maybe somebody (like manufacturer or _real_ user) with xrc700x shall test the code and provide feedback? I've used get_maintainer script to add all people involved. > As the author of commit 5055cccfc2d1 ("net: hsr: Provide RedBox > support (HSR-SAN)"), who do you think should be responsible of taking > care that it plays well with existing offloading drivers? As I've written above - code which I've provided (including tests) is IMHO enough. However, if the community has other impression, then please provide feedback. I will try to take proper actions. 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 Thu, Jun 20, 2024 at 03:28:19PM +0200, Lukasz Majewski wrote: > I don't have xrs700x to test. Shall I spend time on fixing some > perceived issue for IC which I don't have? > > Maybe somebody (like manufacturer or _real_ user) with xrc700x shall > test the code and provide feedback? One of the basic premises when you introduce a new core feature with offload potential is that you consider how the existing drivers will handle it. Either they do something reasonable already (great but rarely happens), or they refuse offloading the new feature until, as you say, the developer or real user has a look at what would be needed. Once you get things to that stage, that would be, in my mind, the cutoff point between the responsibility of who's adding the core feature and who's interested in it on random other hardware. Sometimes, the burden of checking/modifying all existing offloading drivers before adding a new feature is so high, that some offloading API is developed with an opt-in rather than opt-out model. AKA, rather than the configuration being directly given to you and you rejecting what you don't support, the core first assumed you can't offload anything, and you have to set a bit from the driver to announce the core that you can. qdisc_offload_query_caps() is an implementation of this model, though I'm pretty sure the NETDEV_CHANGEUPPER notifier doesn't have anything similar currently. That being said, I think the responsibility falls on your side here, given that you introduced a new HSR port type and offload drivers still implicitly think it's a ring port, because there's no API to tell them otherwise. This is not to take away from the good things you _have_ done already.
Hi Vladimir, > On Thu, Jun 20, 2024 at 03:28:19PM +0200, Lukasz Majewski wrote: > > I don't have xrs700x to test. Shall I spend time on fixing some > > perceived issue for IC which I don't have? > > > > Maybe somebody (like manufacturer or _real_ user) with xrc700x shall > > test the code and provide feedback? > > One of the basic premises when you introduce a new core feature with > offload potential is that you consider how the existing drivers will > handle it. Either they do something reasonable already (great but > rarely happens), or they refuse offloading the new feature until, as > you say, the developer or real user has a look at what would be > needed. Once you get things to that stage, that would be, in my mind, > the cutoff point between the responsibility of who's adding the core > feature and who's interested in it on random other hardware. > > Sometimes, the burden of checking/modifying all existing offloading > drivers before adding a new feature is so high, that some offloading > API is developed with an opt-in rather than opt-out model. AKA, > rather than the configuration being directly given to you and you > rejecting what you don't support, the core first assumed you can't > offload anything, and you have to set a bit from the driver to > announce the core that you can. qdisc_offload_query_caps() is an > implementation of this model, though I'm pretty sure the > NETDEV_CHANGEUPPER notifier doesn't have anything similar currently. > Thanks for the explanation. > That being said, I think the responsibility falls on your side here, > given that you introduced a new HSR port type and offload drivers > still implicitly think it's a ring port, because there's no API to > tell them otherwise. IMHO, the above problem is not related to the patch send here. It shall be addressed with new patch series. > > This is not to take away from the good things you _have_ done already. 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 Fri, Jun 21, 2024 at 10:31:44AM +0200, Lukasz Majewski wrote: > > That being said, I think the responsibility falls on your side here, > > given that you introduced a new HSR port type and offload drivers > > still implicitly think it's a ring port, because there's no API to > > tell them otherwise. > > IMHO, the above problem is not related to the patch send here. It shall > be addressed with new patch series. Why is it not related? Testing for HSR port type is the explicit and more predictable variant of your current patch that waits for the 3rd NETDEV_CHANGEUPPER notifier and implicitly associates it with an interlink port. If you're going to add the ability to test for HSR port type in offloading drivers anyway, I don't understand why you wouldn't want to opt for a uniform approach in ksz9477.
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index 2818e24e2a51..72bb419e34b0 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -3906,6 +3906,13 @@ static int ksz_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr, return -EOPNOTSUPP; } + /* KSZ9477 can only perform HSR offloading for up to two ports */ + if (hweight8(dev->hsr_ports) >= 2) { + NL_SET_ERR_MSG_MOD(extack, + "Cannot offload more than two ports - use software HSR"); + return -EOPNOTSUPP; + } + /* Self MAC address filtering, to avoid frames traversing * the HSR ring more than once. */
The KSZ9477 allows HSR in-HW offloading for any of two selected ports. This patch adds check if one tries to use more than two ports with HSR offloading enabled. The problem is with RedBox configuration (HSR-SAN) - when configuring: ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 interlink lan3 \ supervision 45 version 1 The lan1 (port0) and lan2 (port1) are correctly configured as ports, which can use HSR offloading on ksz9477. However, when we do already have two bits set in hsr_ports, we need to return (-ENOTSUPP), so the interlink port (lan3) would be used with SW based HSR RedBox support. Otherwise, I do see some strange network behavior, as some HSR frames are visible on non-HSR network and vice versa. This causes the switch connected to interlink port (lan3) to drop frames and no communication is possible. Moreover, conceptually - the interlink (i.e. HSR-SAN port - lan3/port2) shall be only supported in software as it is also possible to use ksz9477 with only SW based HSR (i.e. port0/1 -> hsr0 with offloading, port2 -> HSR-SAN/interlink, port4/5 -> hsr1 with SW based HSR). Fixes: 5055cccfc2d1 ("net: hsr: Provide RedBox support (HSR-SAN)") Signed-off-by: Lukasz Majewski <lukma@denx.de> --- Changes for v2: - Add more verbose description with Fixes: tag - Check the condition earlier and remove extra check if SoC is ksz9477 - Add comment in the source code file --- drivers/net/dsa/microchip/ksz_common.c | 7 +++++++ 1 file changed, 7 insertions(+)