netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] net: dsa: microchip: Prevent overriding of HSR port forwarding
@ 2025-08-13 15:26 Frieder Schrempf
  2025-08-13 15:45 ` Łukasz Majewski
  2025-08-14 22:59 ` Andrew Lunn
  0 siblings, 2 replies; 8+ messages in thread
From: Frieder Schrempf @ 2025-08-13 15:26 UTC (permalink / raw)
  To: netdev, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, linux-kernel, Lukasz Majewski, Paolo Abeni,
	UNGLinuxDriver, Vladimir Oltean, Woojung Huh
  Cc: Frieder Schrempf, Florian Fainelli, Jesse Van Gavere,
	Oleksij Rempel, Pieter Van Trappen, Russell King (Oracle),
	Simon Horman, Tristram Ha, Vadim Fedorenko

From: Frieder Schrempf <frieder.schrempf@kontron.de>

The KSZ9477 supports NETIF_F_HW_HSR_FWD to forward packets between
HSR ports. This is set up when creating the HSR interface via
ksz9477_hsr_join() and ksz9477_cfg_port_member().

At the same time ksz_update_port_member() is called on every
state change of a port and reconfiguring the forwarding to the
default state which means packets get only forwarded to the CPU
port.

If the ports are brought up before setting up the HSR interface
and then the port state is not changed afterwards, everything works
as intended:

  ip link set lan1 up
  ip link set lan2 up
  ip link add name hsr type hsr slave1 lan1 slave2 lan2 supervision 45 version 1
  ip addr add dev hsr 10.0.0.10/24
  ip link set hsr up

If the port state is changed after creating the HSR interface, this results
in a non-working HSR setup:

  ip link add name hsr type hsr slave1 lan1 slave2 lan2 supervision 45 version 1
  ip addr add dev hsr 10.0.0.10/24
  ip link set lan1 up
  ip link set lan2 up
  ip link set hsr up

In this state, packets will not get forwarded between the HSR ports and
communication between HSR nodes that are not direct neighbours in the
topology fails.

To avoid this, we prevent all forwarding reconfiguration requests for ports
that are part of a HSR setup with NETIF_F_HW_HSR_FWD enabled.

Fixes: 2d61298fdd7b ("net: dsa: microchip: Enable HSR offloading for KSZ9477")
Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
I'm posting this as RFC as my knowledge of the driver and the stack in
general is very limited. Please review thoroughly and provide feedback.
Thanks!
---
---
 drivers/net/dsa/microchip/ksz_common.c | 11 +++++++++++
 include/net/dsa.h                      | 12 ++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 7c142c17b3f69..56370ecdfe4ee 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2286,6 +2286,17 @@ static void ksz_update_port_member(struct ksz_device *dev, int port)
 		return;
 
 	dp = dsa_to_port(ds, port);
+
+	/*
+	 * HSR ports might use forwarding configured during setup. Prevent any
+	 * modifications as long as the port is part of a HSR setup with
+	 * NETIF_F_HW_HSR_FWD enabled.
+	 */
+	if (dev->hsr_dev && dp->user &&
+	    (dp->user->features & NETIF_F_HW_HSR_FWD) &&
+	    dsa_is_hsr_port(ds, dev->hsr_dev, port))
+		return;
+
 	cpu_port = BIT(dsa_upstream_port(ds, port));
 
 	for (i = 0; i < ds->num_ports; i++) {
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 55e2d97f247eb..846a2cc2f2fc3 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -565,6 +565,18 @@ static inline bool dsa_is_user_port(struct dsa_switch *ds, int p)
 	return dsa_to_port(ds, p)->type == DSA_PORT_TYPE_USER;
 }
 
+static inline bool dsa_is_hsr_port(struct dsa_switch *ds, struct net_device *hsr, int p)
+{
+	struct dsa_port *hsr_dp;
+
+	dsa_hsr_foreach_port(hsr_dp, ds, hsr) {
+		if (hsr_dp->index == p)
+			return true;
+	}
+
+	return false;
+}
+
 #define dsa_tree_for_each_user_port(_dp, _dst) \
 	list_for_each_entry((_dp), &(_dst)->ports, list) \
 		if (dsa_port_is_user((_dp)))
-- 
2.50.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] net: dsa: microchip: Prevent overriding of HSR port forwarding
  2025-08-13 15:26 [RFC PATCH] net: dsa: microchip: Prevent overriding of HSR port forwarding Frieder Schrempf
@ 2025-08-13 15:45 ` Łukasz Majewski
  2025-08-13 15:57   ` Frieder Schrempf
  2025-08-14 22:59 ` Andrew Lunn
  1 sibling, 1 reply; 8+ messages in thread
From: Łukasz Majewski @ 2025-08-13 15:45 UTC (permalink / raw)
  To: Frieder Schrempf
  Cc: netdev, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, linux-kernel, Paolo Abeni, UNGLinuxDriver,
	Vladimir Oltean, Woojung Huh, Frieder Schrempf, Florian Fainelli,
	Jesse Van Gavere, Oleksij Rempel, Pieter Van Trappen,
	Russell King (Oracle), Simon Horman, Tristram Ha, Vadim Fedorenko

Hi Frieder,

> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> The KSZ9477 supports NETIF_F_HW_HSR_FWD to forward packets between
> HSR ports. This is set up when creating the HSR interface via
> ksz9477_hsr_join() and ksz9477_cfg_port_member().
> 
> At the same time ksz_update_port_member() is called on every
> state change of a port and reconfiguring the forwarding to the
> default state which means packets get only forwarded to the CPU
> port.
> 
> If the ports are brought up before setting up the HSR interface
> and then the port state is not changed afterwards, everything works
> as intended:
> 
>   ip link set lan1 up
>   ip link set lan2 up
>   ip link add name hsr type hsr slave1 lan1 slave2 lan2 supervision
> 45 version 1 ip addr add dev hsr 10.0.0.10/24
>   ip link set hsr up
> 
> If the port state is changed after creating the HSR interface, this
> results in a non-working HSR setup:
> 
>   ip link add name hsr type hsr slave1 lan1 slave2 lan2 supervision
> 45 version 1 ip addr add dev hsr 10.0.0.10/24
>   ip link set lan1 up
>   ip link set lan2 up
>   ip link set hsr up
> 
> In this state, packets will not get forwarded between the HSR ports
> and communication between HSR nodes that are not direct neighbours in
> the topology fails.
> 
> To avoid this, we prevent all forwarding reconfiguration requests for
> ports that are part of a HSR setup with NETIF_F_HW_HSR_FWD enabled.
> 
> Fixes: 2d61298fdd7b ("net: dsa: microchip: Enable HSR offloading for
> KSZ9477") Signed-off-by: Frieder Schrempf
> <frieder.schrempf@kontron.de> ---
> I'm posting this as RFC as my knowledge of the driver and the stack in
> general is very limited. Please review thoroughly and provide
> feedback. Thanks!

I don't have the HW at hand at the moment (temporary).

Could you check if this patch works when you create two hsr interfaces
- i.e. hsr1 would use HW offloading from KSZ9744 and hsr2 is just the
  one supporting HSR in software.

> ---
> ---
>  drivers/net/dsa/microchip/ksz_common.c | 11 +++++++++++
>  include/net/dsa.h                      | 12 ++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c
> b/drivers/net/dsa/microchip/ksz_common.c index
> 7c142c17b3f69..56370ecdfe4ee 100644 ---
> a/drivers/net/dsa/microchip/ksz_common.c +++
> b/drivers/net/dsa/microchip/ksz_common.c @@ -2286,6 +2286,17 @@
> static void ksz_update_port_member(struct ksz_device *dev, int port)
> return; 
>  	dp = dsa_to_port(ds, port);
> +
> +	/*
> +	 * HSR ports might use forwarding configured during setup.
> Prevent any
> +	 * modifications as long as the port is part of a HSR setup
> with
> +	 * NETIF_F_HW_HSR_FWD enabled.
> +	 */
> +	if (dev->hsr_dev && dp->user &&
> +	    (dp->user->features & NETIF_F_HW_HSR_FWD) &&
> +	    dsa_is_hsr_port(ds, dev->hsr_dev, port))
> +		return;
> +
>  	cpu_port = BIT(dsa_upstream_port(ds, port));
>  
>  	for (i = 0; i < ds->num_ports; i++) {
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 55e2d97f247eb..846a2cc2f2fc3 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -565,6 +565,18 @@ static inline bool dsa_is_user_port(struct
> dsa_switch *ds, int p) return dsa_to_port(ds, p)->type ==
> DSA_PORT_TYPE_USER; }
>  
> +static inline bool dsa_is_hsr_port(struct dsa_switch *ds, struct
> net_device *hsr, int p) +{
> +	struct dsa_port *hsr_dp;
> +
> +	dsa_hsr_foreach_port(hsr_dp, ds, hsr) {
> +		if (hsr_dp->index == p)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +

I thought that we already had such function implemented. Apparently I
must have been wrong.

>  #define dsa_tree_for_each_user_port(_dp, _dst) \
>  	list_for_each_entry((_dp), &(_dst)->ports, list) \
>  		if (dsa_port_is_user((_dp)))



-- 
Best regards,

Lukasz Majewski

--
Nabla Software Engineering GmbH
HRB 40522 Augsburg
Phone: +49 821 45592596
E-Mail: office@nabladev.com
Geschftsfhrer : Stefano Babic

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] net: dsa: microchip: Prevent overriding of HSR port forwarding
  2025-08-13 15:45 ` Łukasz Majewski
@ 2025-08-13 15:57   ` Frieder Schrempf
  2025-08-14  7:23     ` Łukasz Majewski
  0 siblings, 1 reply; 8+ messages in thread
From: Frieder Schrempf @ 2025-08-13 15:57 UTC (permalink / raw)
  To: Łukasz Majewski, Frieder Schrempf
  Cc: netdev, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, linux-kernel, Paolo Abeni, UNGLinuxDriver,
	Vladimir Oltean, Woojung Huh, Florian Fainelli, Jesse Van Gavere,
	Oleksij Rempel, Pieter Van Trappen, Russell King (Oracle),
	Simon Horman, Tristram Ha, Vadim Fedorenko

Am 13.08.25 um 17:45 schrieb Łukasz Majewski:
> [Sie erhalten nicht h?ufig E-Mails von lukma@nabladev.com. Weitere Informationen, warum dies wichtig ist, finden Sie unter https://aka.ms/LearnAboutSenderIdentification ]
> 
> Hi Frieder,
> 
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> The KSZ9477 supports NETIF_F_HW_HSR_FWD to forward packets between
>> HSR ports. This is set up when creating the HSR interface via
>> ksz9477_hsr_join() and ksz9477_cfg_port_member().
>>
>> At the same time ksz_update_port_member() is called on every
>> state change of a port and reconfiguring the forwarding to the
>> default state which means packets get only forwarded to the CPU
>> port.
>>
>> If the ports are brought up before setting up the HSR interface
>> and then the port state is not changed afterwards, everything works
>> as intended:
>>
>>   ip link set lan1 up
>>   ip link set lan2 up
>>   ip link add name hsr type hsr slave1 lan1 slave2 lan2 supervision
>> 45 version 1 ip addr add dev hsr 10.0.0.10/24
>>   ip link set hsr up
>>
>> If the port state is changed after creating the HSR interface, this
>> results in a non-working HSR setup:
>>
>>   ip link add name hsr type hsr slave1 lan1 slave2 lan2 supervision
>> 45 version 1 ip addr add dev hsr 10.0.0.10/24
>>   ip link set lan1 up
>>   ip link set lan2 up
>>   ip link set hsr up
>>
>> In this state, packets will not get forwarded between the HSR ports
>> and communication between HSR nodes that are not direct neighbours in
>> the topology fails.
>>
>> To avoid this, we prevent all forwarding reconfiguration requests for
>> ports that are part of a HSR setup with NETIF_F_HW_HSR_FWD enabled.
>>
>> Fixes: 2d61298fdd7b ("net: dsa: microchip: Enable HSR offloading for
>> KSZ9477") Signed-off-by: Frieder Schrempf
>> <frieder.schrempf@kontron.de> ---
>> I'm posting this as RFC as my knowledge of the driver and the stack in
>> general is very limited. Please review thoroughly and provide
>> feedback. Thanks!
> 
> I don't have the HW at hand at the moment (temporary).
> 
> Could you check if this patch works when you create two hsr interfaces
> - i.e. hsr1 would use HW offloading from KSZ9744 and hsr2 is just the
>   one supporting HSR in software.

My hardware only has three user ports. So that might get a bit difficult
to test. I will try to configure one unconnected port to set up two HSR
links, but I won't be able to fully test this due to the lack of the
fourth physical link.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] net: dsa: microchip: Prevent overriding of HSR port forwarding
  2025-08-13 15:57   ` Frieder Schrempf
@ 2025-08-14  7:23     ` Łukasz Majewski
  0 siblings, 0 replies; 8+ messages in thread
From: Łukasz Majewski @ 2025-08-14  7:23 UTC (permalink / raw)
  To: Frieder Schrempf
  Cc: Frieder Schrempf, netdev, Andrew Lunn, David S. Miller,
	Eric Dumazet, Jakub Kicinski, linux-kernel, Paolo Abeni,
	UNGLinuxDriver, Vladimir Oltean, Woojung Huh, Florian Fainelli,
	Jesse Van Gavere, Oleksij Rempel, Pieter Van Trappen,
	Russell King (Oracle), Simon Horman, Tristram Ha, Vadim Fedorenko

On Wed, 13 Aug 2025 17:57:02 +0200
Frieder Schrempf <frieder.schrempf@kontron.de> wrote:

> Am 13.08.25 um 17:45 schrieb Łukasz Majewski:
> > [Sie erhalten nicht h?ufig E-Mails von lukma@nabladev.com. Weitere
> > Informationen, warum dies wichtig ist, finden Sie unter
> > https://aka.ms/LearnAboutSenderIdentification ]
> > 
> > Hi Frieder,
> >   
> >> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> >>
> >> The KSZ9477 supports NETIF_F_HW_HSR_FWD to forward packets between
> >> HSR ports. This is set up when creating the HSR interface via
> >> ksz9477_hsr_join() and ksz9477_cfg_port_member().
> >>
> >> At the same time ksz_update_port_member() is called on every
> >> state change of a port and reconfiguring the forwarding to the
> >> default state which means packets get only forwarded to the CPU
> >> port.
> >>
> >> If the ports are brought up before setting up the HSR interface
> >> and then the port state is not changed afterwards, everything works
> >> as intended:
> >>
> >>   ip link set lan1 up
> >>   ip link set lan2 up
> >>   ip link add name hsr type hsr slave1 lan1 slave2 lan2 supervision
> >> 45 version 1 ip addr add dev hsr 10.0.0.10/24
> >>   ip link set hsr up
> >>
> >> If the port state is changed after creating the HSR interface, this
> >> results in a non-working HSR setup:
> >>
> >>   ip link add name hsr type hsr slave1 lan1 slave2 lan2 supervision
> >> 45 version 1 ip addr add dev hsr 10.0.0.10/24
> >>   ip link set lan1 up
> >>   ip link set lan2 up
> >>   ip link set hsr up
> >>
> >> In this state, packets will not get forwarded between the HSR ports
> >> and communication between HSR nodes that are not direct neighbours
> >> in the topology fails.
> >>
> >> To avoid this, we prevent all forwarding reconfiguration requests
> >> for ports that are part of a HSR setup with NETIF_F_HW_HSR_FWD
> >> enabled.
> >>
> >> Fixes: 2d61298fdd7b ("net: dsa: microchip: Enable HSR offloading
> >> for KSZ9477") Signed-off-by: Frieder Schrempf
> >> <frieder.schrempf@kontron.de> ---
> >> I'm posting this as RFC as my knowledge of the driver and the
> >> stack in general is very limited. Please review thoroughly and
> >> provide feedback. Thanks!  
> > 
> > I don't have the HW at hand at the moment (temporary).
> > 
> > Could you check if this patch works when you create two hsr
> > interfaces
> > - i.e. hsr1 would use HW offloading from KSZ9744 and hsr2 is just
> > the one supporting HSR in software.  
> 
> My hardware only has three user ports. So that might get a bit
> difficult to test. I will try to configure one unconnected port to
> set up two HSR links, but I won't be able to fully test this due to
> the lack of the fourth physical link.
> 

Ok, I see...

I was using the Atmel's/Microchip Devel board with 5 ports...

-- 
Best regards,

Lukasz Majewski

--
Nabla Software Engineering GmbH
HRB 40522 Augsburg
Phone: +49 821 45592596
E-Mail: office@nabladev.com
Geschftsfhrer : Stefano Babic

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] net: dsa: microchip: Prevent overriding of HSR port forwarding
  2025-08-13 15:26 [RFC PATCH] net: dsa: microchip: Prevent overriding of HSR port forwarding Frieder Schrempf
  2025-08-13 15:45 ` Łukasz Majewski
@ 2025-08-14 22:59 ` Andrew Lunn
  2025-08-15 18:30   ` Frieder Schrempf
  1 sibling, 1 reply; 8+ messages in thread
From: Andrew Lunn @ 2025-08-14 22:59 UTC (permalink / raw)
  To: Frieder Schrempf
  Cc: netdev, David S. Miller, Eric Dumazet, Jakub Kicinski,
	linux-kernel, Lukasz Majewski, Paolo Abeni, UNGLinuxDriver,
	Vladimir Oltean, Woojung Huh, Frieder Schrempf, Florian Fainelli,
	Jesse Van Gavere, Oleksij Rempel, Pieter Van Trappen,
	Russell King (Oracle), Simon Horman, Tristram Ha, Vadim Fedorenko

On Wed, Aug 13, 2025 at 05:26:12PM +0200, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> The KSZ9477 supports NETIF_F_HW_HSR_FWD to forward packets between
> HSR ports. This is set up when creating the HSR interface via
> ksz9477_hsr_join() and ksz9477_cfg_port_member().
> 
> At the same time ksz_update_port_member() is called on every
> state change of a port and reconfiguring the forwarding to the
> default state which means packets get only forwarded to the CPU
> port.
> 
> If the ports are brought up before setting up the HSR interface
> and then the port state is not changed afterwards, everything works
> as intended:
> 
>   ip link set lan1 up
>   ip link set lan2 up
>   ip link add name hsr type hsr slave1 lan1 slave2 lan2 supervision 45 version 1
>   ip addr add dev hsr 10.0.0.10/24
>   ip link set hsr up
> 
> If the port state is changed after creating the HSR interface, this results
> in a non-working HSR setup:
> 
>   ip link add name hsr type hsr slave1 lan1 slave2 lan2 supervision 45 version 1
>   ip addr add dev hsr 10.0.0.10/24
>   ip link set lan1 up
>   ip link set lan2 up
>   ip link set hsr up

So, restating what i said in a different thread, what happens if only
software was used? No hardware offload.

	Andrew

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] net: dsa: microchip: Prevent overriding of HSR port forwarding
  2025-08-14 22:59 ` Andrew Lunn
@ 2025-08-15 18:30   ` Frieder Schrempf
  2025-08-16  0:53     ` Tristram.Ha
  0 siblings, 1 reply; 8+ messages in thread
From: Frieder Schrempf @ 2025-08-15 18:30 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, David S. Miller, Eric Dumazet, Jakub Kicinski,
	linux-kernel, Lukasz Majewski, Paolo Abeni, UNGLinuxDriver,
	Vladimir Oltean, Woojung Huh, Frieder Schrempf, Florian Fainelli,
	Jesse Van Gavere, Oleksij Rempel, Pieter Van Trappen,
	Russell King (Oracle), Simon Horman, Tristram Ha, Vadim Fedorenko

Am 15.08.25 um 00:59 schrieb Andrew Lunn:
> On Wed, Aug 13, 2025 at 05:26:12PM +0200, Frieder Schrempf wrote:
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> The KSZ9477 supports NETIF_F_HW_HSR_FWD to forward packets between
>> HSR ports. This is set up when creating the HSR interface via
>> ksz9477_hsr_join() and ksz9477_cfg_port_member().
>>
>> At the same time ksz_update_port_member() is called on every
>> state change of a port and reconfiguring the forwarding to the
>> default state which means packets get only forwarded to the CPU
>> port.
>>
>> If the ports are brought up before setting up the HSR interface
>> and then the port state is not changed afterwards, everything works
>> as intended:
>>
>>    ip link set lan1 up
>>    ip link set lan2 up
>>    ip link add name hsr type hsr slave1 lan1 slave2 lan2 supervision 45 version 1
>>    ip addr add dev hsr 10.0.0.10/24
>>    ip link set hsr up
>>
>> If the port state is changed after creating the HSR interface, this results
>> in a non-working HSR setup:
>>
>>    ip link add name hsr type hsr slave1 lan1 slave2 lan2 supervision 45 version 1
>>    ip addr add dev hsr 10.0.0.10/24
>>    ip link set lan1 up
>>    ip link set lan2 up
>>    ip link set hsr up
> 
> So, restating what i said in a different thread, what happens if only
> software was used? No hardware offload.

Sorry, I don't understand what you are aiming at.

Yes, this issue is related to hardware offloading. As far as I know 
there is no option (for the user) to force HSR into SW-only mode. The 
KSZ9477 driver uses hardware offloading up to the capabilities of the HW 
by default.

Yes, if I disable the offloading by modifying the driver code as already 
described in the other thread, the issue can be fixed at the cost of 
loosing the HW acceleration. In this case the driver consistently 
configures the HSR ports to forward any packets to the CPU which then 
forwards them as needed.

With the driver code as-is, there are two conflicting values used for 
the register that configures the forwarding. One is set during the HSR 
setup and makes sure that HSR ports forward packets among each other 
(and not only to the CPU), the other is set while changing the link 
state of the HSR ports and causes the forwarding to only happen between 
each port and the CPU, therefore effectively disabling the HW offloading 
while the driver still assumes it is enabled.

This is obviously a problem that should be fixed in the driver as 
changing the link state of the ports *after* setup of the HSR is a 
completely valid operation that shouldn't break things like it currently 
does.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [RFC PATCH] net: dsa: microchip: Prevent overriding of HSR port forwarding
  2025-08-15 18:30   ` Frieder Schrempf
@ 2025-08-16  0:53     ` Tristram.Ha
  2025-08-18 11:00       ` Frieder Schrempf
  0 siblings, 1 reply; 8+ messages in thread
From: Tristram.Ha @ 2025-08-16  0:53 UTC (permalink / raw)
  To: frieder
  Cc: netdev, davem, edumazet, kuba, linux-kernel, lukma, pabeni,
	UNGLinuxDriver, olteanv, Woojung.Huh, andrew, frieder.schrempf,
	florian.fainelli, jesseevg, o.rempel, pieter.van.trappen,
	rmk+kernel, horms, vadim.fedorenko

> Am 15.08.25 um 00:59 schrieb Andrew Lunn:
> > On Wed, Aug 13, 2025 at 05:26:12PM +0200, Frieder Schrempf wrote:
> >> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> >>
> >> The KSZ9477 supports NETIF_F_HW_HSR_FWD to forward packets between
> >> HSR ports. This is set up when creating the HSR interface via
> >> ksz9477_hsr_join() and ksz9477_cfg_port_member().
> >>
> >> At the same time ksz_update_port_member() is called on every
> >> state change of a port and reconfiguring the forwarding to the
> >> default state which means packets get only forwarded to the CPU
> >> port.
> >>
> >> If the ports are brought up before setting up the HSR interface
> >> and then the port state is not changed afterwards, everything works
> >> as intended:
> >>
> >>    ip link set lan1 up
> >>    ip link set lan2 up
> >>    ip link add name hsr type hsr slave1 lan1 slave2 lan2 supervision 45 version 1
> >>    ip addr add dev hsr 10.0.0.10/24
> >>    ip link set hsr up
> >>
> >> If the port state is changed after creating the HSR interface, this results
> >> in a non-working HSR setup:
> >>
> >>    ip link add name hsr type hsr slave1 lan1 slave2 lan2 supervision 45 version 1
> >>    ip addr add dev hsr 10.0.0.10/24
> >>    ip link set lan1 up
> >>    ip link set lan2 up
> >>    ip link set hsr up
> >
> > So, restating what i said in a different thread, what happens if only
> > software was used? No hardware offload.
> 
> Sorry, I don't understand what you are aiming at.
> 
> Yes, this issue is related to hardware offloading. As far as I know
> there is no option (for the user) to force HSR into SW-only mode. The
> KSZ9477 driver uses hardware offloading up to the capabilities of the HW
> by default.
> 
> Yes, if I disable the offloading by modifying the driver code as already
> described in the other thread, the issue can be fixed at the cost of
> loosing the HW acceleration. In this case the driver consistently
> configures the HSR ports to forward any packets to the CPU which then
> forwards them as needed.
> 
> With the driver code as-is, there are two conflicting values used for
> the register that configures the forwarding. One is set during the HSR
> setup and makes sure that HSR ports forward packets among each other
> (and not only to the CPU), the other is set while changing the link
> state of the HSR ports and causes the forwarding to only happen between
> each port and the CPU, therefore effectively disabling the HW offloading
> while the driver still assumes it is enabled.
> 
> This is obviously a problem that should be fixed in the driver as
> changing the link state of the ports *after* setup of the HSR is a
> completely valid operation that shouldn't break things like it currently
> does.

Here is a simpler fix for this problem.  If that works for you I can
submit the fix.

net: dsa: microchip: Fix HSR port setup issue

ksz9477_hsr_join() is called once to setup the HSR port membership, but
the port can be enabled later, or disabled and enabled back and the port
membership is not set correctly inside ksz_update_port_member().  The
added code always use the correct HSR port membership for HSR port that
is enabled.

Fixes: 2d61298fdd7b ("net: dsa: microchip: Enable HSR offloading for KSZ9477")
Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 4cb14288ff0f..c04d4c895025 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2457,6 +2457,9 @@ static void ksz_update_port_member(struct ksz_device *dev, int port)
 		dev->dev_ops->cfg_port_member(dev, i, val | cpu_port);
 	}
 
+	if (!port_member && p->stp_state == BR_STATE_FORWARDING &&
+	    (dev->hsr_ports & BIT(port)))
+		port_member = dev->hsr_ports;
 	dev->dev_ops->cfg_port_member(dev, port, port_member | cpu_port);
 }


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] net: dsa: microchip: Prevent overriding of HSR port forwarding
  2025-08-16  0:53     ` Tristram.Ha
@ 2025-08-18 11:00       ` Frieder Schrempf
  0 siblings, 0 replies; 8+ messages in thread
From: Frieder Schrempf @ 2025-08-18 11:00 UTC (permalink / raw)
  To: Tristram.Ha, frieder
  Cc: netdev, davem, edumazet, kuba, linux-kernel, lukma, pabeni,
	UNGLinuxDriver, olteanv, Woojung.Huh, andrew, florian.fainelli,
	jesseevg, o.rempel, pieter.van.trappen, rmk+kernel, horms,
	vadim.fedorenko

Hi Tristram,

Am 16.08.25 um 02:53 schrieb Tristram.Ha@microchip.com:
>> Am 15.08.25 um 00:59 schrieb Andrew Lunn:
>>> On Wed, Aug 13, 2025 at 05:26:12PM +0200, Frieder Schrempf wrote:
>>>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>>
>>>> The KSZ9477 supports NETIF_F_HW_HSR_FWD to forward packets between
>>>> HSR ports. This is set up when creating the HSR interface via
>>>> ksz9477_hsr_join() and ksz9477_cfg_port_member().
>>>>
>>>> At the same time ksz_update_port_member() is called on every
>>>> state change of a port and reconfiguring the forwarding to the
>>>> default state which means packets get only forwarded to the CPU
>>>> port.
>>>>
>>>> If the ports are brought up before setting up the HSR interface
>>>> and then the port state is not changed afterwards, everything works
>>>> as intended:
>>>>
>>>>    ip link set lan1 up
>>>>    ip link set lan2 up
>>>>    ip link add name hsr type hsr slave1 lan1 slave2 lan2 supervision 45 version 1
>>>>    ip addr add dev hsr 10.0.0.10/24
>>>>    ip link set hsr up
>>>>
>>>> If the port state is changed after creating the HSR interface, this results
>>>> in a non-working HSR setup:
>>>>
>>>>    ip link add name hsr type hsr slave1 lan1 slave2 lan2 supervision 45 version 1
>>>>    ip addr add dev hsr 10.0.0.10/24
>>>>    ip link set lan1 up
>>>>    ip link set lan2 up
>>>>    ip link set hsr up
>>>
>>> So, restating what i said in a different thread, what happens if only
>>> software was used? No hardware offload.
>>
>> Sorry, I don't understand what you are aiming at.
>>
>> Yes, this issue is related to hardware offloading. As far as I know
>> there is no option (for the user) to force HSR into SW-only mode. The
>> KSZ9477 driver uses hardware offloading up to the capabilities of the HW
>> by default.
>>
>> Yes, if I disable the offloading by modifying the driver code as already
>> described in the other thread, the issue can be fixed at the cost of
>> loosing the HW acceleration. In this case the driver consistently
>> configures the HSR ports to forward any packets to the CPU which then
>> forwards them as needed.
>>
>> With the driver code as-is, there are two conflicting values used for
>> the register that configures the forwarding. One is set during the HSR
>> setup and makes sure that HSR ports forward packets among each other
>> (and not only to the CPU), the other is set while changing the link
>> state of the HSR ports and causes the forwarding to only happen between
>> each port and the CPU, therefore effectively disabling the HW offloading
>> while the driver still assumes it is enabled.
>>
>> This is obviously a problem that should be fixed in the driver as
>> changing the link state of the ports *after* setup of the HSR is a
>> completely valid operation that shouldn't break things like it currently
>> does.
> 
> Here is a simpler fix for this problem.  If that works for you I can
> submit the fix.
> 
> net: dsa: microchip: Fix HSR port setup issue
> 
> ksz9477_hsr_join() is called once to setup the HSR port membership, but
> the port can be enabled later, or disabled and enabled back and the port
> membership is not set correctly inside ksz_update_port_member().  The
> added code always use the correct HSR port membership for HSR port that
> is enabled.
> 
> Fixes: 2d61298fdd7b ("net: dsa: microchip: Enable HSR offloading for KSZ9477")
> Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
> ---
>  drivers/net/dsa/microchip/ksz_common.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 4cb14288ff0f..c04d4c895025 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -2457,6 +2457,9 @@ static void ksz_update_port_member(struct ksz_device *dev, int port)
>  		dev->dev_ops->cfg_port_member(dev, i, val | cpu_port);
>  	}
>  
> +	if (!port_member && p->stp_state == BR_STATE_FORWARDING &&
> +	    (dev->hsr_ports & BIT(port)))
> +		port_member = dev->hsr_ports;
>  	dev->dev_ops->cfg_port_member(dev, port, port_member | cpu_port);
>  }
> 

This looks fantastic! Way better than my approach!

I just tested it and it seems to fix the issue for me. Please send this
as formal patch and put me in Reported-by.

You might also want to add a comment in the code that gives a short
explanation for this.

Thanks
Frieder

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2025-08-18 11:01 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-13 15:26 [RFC PATCH] net: dsa: microchip: Prevent overriding of HSR port forwarding Frieder Schrempf
2025-08-13 15:45 ` Łukasz Majewski
2025-08-13 15:57   ` Frieder Schrempf
2025-08-14  7:23     ` Łukasz Majewski
2025-08-14 22:59 ` Andrew Lunn
2025-08-15 18:30   ` Frieder Schrempf
2025-08-16  0:53     ` Tristram.Ha
2025-08-18 11:00       ` Frieder Schrempf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).