Message-ID: <d8603413-d410-4cc9-ab3a-da9c6d868eca@kontron.de>
Date: Thu, 30 Jan 2025 11:19:55 +0100
From: Frieder Schrempf <frieder.schrempf@...tron.de>
To: Lukasz Majewski <lukma@...x.de>
Cc: Andrew Lunn <andrew@...n.ch>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: KSZ9477 HSR Offloading

Hi Lukasz,

On 29.01.25 11:52 PM, Lukasz Majewski wrote:
> Hi Frieder,
> 
>> On 29.01.25 2:58 PM, Lukasz Majewski wrote:
>>> Hi Frieder,
>>>   
>>>> Hi Lukasz,
>>>>
>>>> On 29.01.25 12:17 PM, Lukasz Majewski wrote:  
>>>>> Hi Frieder,
>>>>>     
>>>>>> On 29.01.25 8:24 AM, Frieder Schrempf wrote:    
>>>>>>> Hi Andrew,
>>>>>>>
>>>>>>> On 28.01.25 6:51 PM, Andrew Lunn wrote:      
>>>>>>>> On Tue, Jan 28, 2025 at 05:14:46PM +0100, Frieder Schrempf
>>>>>>>> wrote:      
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> I'm trying out HSR support on KSZ9477 with v6.12. My setup
>>>>>>>>> looks like this:
>>>>>>>>>
>>>>>>>>> +-------------+         +-------------+
>>>>>>>>> |             |         |             |
>>>>>>>>> |   Node A    |         |   Node D    |
>>>>>>>>> |             |         |             |
>>>>>>>>> |             |         |             |
>>>>>>>>> | LAN1   LAN2 |         | LAN1   LAN2 |
>>>>>>>>> +--+-------+--+         +--+------+---+
>>>>>>>>>    |       |               |      |
>>>>>>>>>    |       +---------------+      |
>>>>>>>>>    |                              |
>>>>>>>>>    |       +---------------+      |
>>>>>>>>>    |       |               |      |
>>>>>>>>> +--+-------+--+         +--+------+---+
>>>>>>>>> | LAN1   LAN2 |         | LAN1   LAN2 |
>>>>>>>>> |             |         |             |
>>>>>>>>> |             |         |             |
>>>>>>>>> |   Node B    |         |   Node C    |
>>>>>>>>> |             |         |             |
>>>>>>>>> +-------------+         +-------------+
>>>>>>>>>
>>>>>>>>> On each device the LAN1 and LAN2 are added as HSR slaves.
>>>>>>>>> Then I try to do ping tests between each of the HSR
>>>>>>>>> interfaces.
>>>>>>>>>
>>>>>>>>> The result is that I can reach the neighboring nodes just
>>>>>>>>> fine, but I can't reach the remote node that needs packages
>>>>>>>>> to be forwarded through the other nodes. For example I can't
>>>>>>>>> ping from node A to C.
>>>>>>>>>
>>>>>>>>> I've tried to disable HW offloading in the driver and then
>>>>>>>>> everything starts working.
>>>>>>>>>
>>>>>>>>> Is this a problem with HW offloading in the KSZ driver, or am
>>>>>>>>> I missing something essential?      
>>>>>
>>>>> Thanks for looking and testing such large scale setup.
>>>>>     
>>>>>>>>
>>>>>>>> How are IP addresses configured? I assume you have a bridge,
>>>>>>>> LAN1 and LAN2 are members of the bridge, and the IP address is
>>>>>>>> on the bridge interface?      
>>>>>>>
>>>>>>> I have a HSR interface on each node that covers LAN1 and LAN2 as
>>>>>>> slaves and the IP addresses are on those HSR interfaces. For
>>>>>>> node A:
>>>>>>>
>>>>>>> ip link add name hsr type hsr slave1 lan1 slave2 lan2
>>>>>>> supervision 45 version 1
>>>>>>> ip addr add 172.20.1.1/24 dev hsr
>>>>>>>
>>>>>>> The other nodes have the addresses 172.20.1.2/24, 172.20.1.3/24
>>>>>>> and 172.20.1.4/24 respectively.
>>>>>>>
>>>>>>> Then on node A, I'm doing:
>>>>>>>
>>>>>>> ping 172.20.1.2 # neighboring node B works
>>>>>>> ping 172.20.1.4 # neighboring node D works
>>>>>>> ping 172.20.1.3 # remote node C works only if I disable
>>>>>>> offloading      
>>>>>>
>>>>>> BTW, it's enough to disable the offloading of the forwarding for
>>>>>> HSR frames to make it work.
>>>>>>
>>>>>> --- a/drivers/net/dsa/microchip/ksz9477.c
>>>>>> +++ b/drivers/net/dsa/microchip/ksz9477.c
>>>>>> @@ -1267,7 +1267,7 @@ int ksz9477_tc_cbs_set_cinc(struct
>>>>>> ksz_device *dev, int port, u32 val)
>>>>>>   * Moreover, the NETIF_F_HW_HSR_FWD feature is also enabled, as
>>>>>> HSR frames
>>>>>>   * can be forwarded in the switch fabric between HSR ports.
>>>>>>   */
>>>>>> -#define KSZ9477_SUPPORTED_HSR_FEATURES (NETIF_F_HW_HSR_DUP |
>>>>>> NETIF_F_HW_HSR_FWD)
>>>>>> +#define KSZ9477_SUPPORTED_HSR_FEATURES (NETIF_F_HW_HSR_DUP)
>>>>>>
>>>>>>  void ksz9477_hsr_join(struct dsa_switch *ds, int port, struct
>>>>>> net_device *hsr)
>>>>>>  {
>>>>>> @@ -1279,16 +1279,6 @@ void ksz9477_hsr_join(struct dsa_switch
>>>>>> *ds, int port, struct net_device *hsr)
>>>>>>         /* Program which port(s) shall support HSR */
>>>>>>         ksz_rmw32(dev, REG_HSR_PORT_MAP__4, BIT(port),
>>>>>> BIT(port));
>>>>>>
>>>>>> -       /* Forward frames between HSR ports (i.e. bridge together
>>>>>> HSR ports) */
>>>>>> -       if (dev->hsr_ports) {
>>>>>> -               dsa_hsr_foreach_port(hsr_dp, ds, hsr)
>>>>>> -                       hsr_ports |= BIT(hsr_dp->index);
>>>>>> -
>>>>>> -               hsr_ports |= BIT(dsa_upstream_port(ds, port));
>>>>>> -               dsa_hsr_foreach_port(hsr_dp, ds, hsr)
>>>>>> -                       ksz9477_cfg_port_member(dev,
>>>>>> hsr_dp->index, hsr_ports);
>>>>>> -       }
>>>>>> -
>>>>>>         if (!dev->hsr_ports) {
>>>>>>                 /* Enable discarding of received HSR frames */
>>>>>>                 ksz_read8(dev, REG_HSR_ALU_CTRL_0__1, &data);    
>>>>>
>>>>> This means that KSZ9477 forwarding is dropping frames when HW
>>>>> acceleration is used (for non "neighbour" nodes).
>>>>>
>>>>> On my setup I only had 2 KSZ9477 devel boards.
>>>>>
>>>>> And as you wrote - the SW based one works, so extending
>>>>> https://elixir.bootlin.com/linux/v6.12-rc2/source/tools/testing/selftests/net/hsr
>>>>>
>>>>> would not help in this case.    
>>>>
>>>> I see. With two boards you can't test the accelerated forwarding.
>>>> So how did you test the forwarding at all? Or are you telling me,
>>>> that this was added to the driver without prior testing (which
>>>> seems a bit bold and unusual)?  
>>>
>>> The packet forwarding is for generating two frames copies on two HSR
>>> coupled ports on a single KSZ9477:  
>>
>> Isn't that what duplication aka NETIF_F_HW_HSR_DUP is for?
> 
> As I mentioned - the NETIF_F_HW_HSR_DUP is to remove duplicated frames.
> 
> NETIF_F_HW_HSR_FWD is to in-hw generate frame copy for HSR port to be
> sent:
> https://elixir.bootlin.com/linux/v6.13/source/drivers/net/dsa/microchip/ksz9477.c#L1252

Ok, so you are using a different definition for the "forwarding". What
puzzles me is the explanation for the HSR feature flags here [1]. They
seem to suggest the following, which differs from your explanation:

Forwarding (aka NETIF_F_HW_HSR_FWD):

"Forwarding involves automatically forwarding between redundant ports in
an HSR."

This sounds more like the "forwarding" of a HSR frame within the Ring,
between two HSR ports, that I was thinking of.

Duplication (aka NETIF_F_HW_HSR_DUP):

"Duplication involves the switch automatically sending a single frame
from the CPU port to both redundant ports."

This is contradictory to what you wrote above and sounds more
reasonable. This is what you instead call forwarding above.

Are you mixing things up, here? Am I completely on the wrong track? I'm
just trying to understand the basics here.

>>
>>>
>>> https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ApplicationNotes/ApplicationNotes/AN3474-KSZ9477-High-Availability-Seamless-Redundancy-Application-Note-00003474A.pdf
>>>
>>> The KSZ9477 chip also supports RX packet duplication removal, but
>>> cannot guarantee 100% success (so as a fallback it is done in SW).
>>>
>>> The infrastructure from:
>>> https://elixir.bootlin.com/linux/v6.13/source/tools/testing/selftests/net/hsr/hsr_redbox.sh#L50
>>>
>>> is enough to test HW accelerated forwarding (of KSZ9477) from NS1
>>> and NS2.  
>>
>> I'm not really sure if I get it. In this setup NS1 and NS2 are
>> connected via HSR link (two physical links). On one side packets are
>> sent duplicated on both physical ports. On the receiving side the
>> duplication is removed and one packet is forwarded to the CPU.
>>
>> Where is forwarding involved here? 
> 
> In-HW forwarding is when KSZ9477 duplicates frame to be send on second
> HSR aware port.
> 
> (only 2 of them can be coupled to have in-hw support for duplication
> and forwarding. Creating more hsr "interfaces" would just use SW).
> 
>> Isn't forwarding only for cases
>> with one intermediate node between the sending and receiving node?
> 
> This kind of "forwarding" is done in software in the hsr driver.

But according to the official explanation of the flags [1] this kind of
forwarding is exactly what NETIF_F_HW_HSR_FWD seems to be about.

> 
>>
>>>   
>>>>
>>>> Anyway, do you have any suggestions for debugging this?
>>>> Unfortunately I know almost nothing about this topic. But I can
>>>> offer to test on my setup, at least for now. I don't know how long
>>>> I will still have access to the hardware.  
>>>
>>> For some reason only frames to neighbours are delivered.
>>>
>>> So those are removed at some point (either in KSZ9477 HW or in HSR
>>> driver itself).
>>>
>>> Do you have some dumps from tshark/wireshark to share?
>>>   
>>>>
>>>> If we can't find a proper solution in the long run, I will probably
>>>> send a patch to disable the accelerated forwarding to at least make
>>>> HSR work by default.  
>>>
>>> As I've noted above - the HW accelerated forwarding is in the
>>> KSZ9477 chip.  
>>
>> Yeah, but if the HW accelerated forwarding doesn't work
> 
> The "forwarding" in KSZ9477 IC works OK, as frames are duplicated (i.e.
> forwarded) to both HSR coupled ports.

No, I don't think duplication and forwarding refer to the same thing
here. At least it doesn't make sense for me.

> The problem is with dropping frames travelling in connected KSZ9477
> devices.

I'm not really sure.

> 
>> it would be
>> better to use no acceleration and have it work in SW at least by
>> default, right?
> 
> IMHO, it would be best to fix the code.

Agreed.

>>
>>>
>>> The code which you uncomment, is following what I've understood from
>>> the standard (and maybe the bug is somewhere there).  
>>
>> Ok, thanks for explaining. I will see if I can find some time to
>> gather some more information on the problem.
> 
> That would be very helpful. Thanks in advance for it.
One more information I just found out: I can leave ksz9477_hsr_join()
untouched and only remove the feature flags from
KSZ9477_SUPPORTED_HSR_FEATURES to make things work.

Broken:

#define KSZ9477_SUPPORTED_HSR_FEATURES (NETIF_F_HW_HSR_DUP |
NETIF_F_HW_HSR_FWD)

Works:

#define KSZ9477_SUPPORTED_HSR_FEATURES (NETIF_F_HW_HSR_DUP)

Works:

#define KSZ9477_SUPPORTED_HSR_FEATURES (NETIF_F_HW_HSR_FWD)

Works:

#define KSZ9477_SUPPORTED_HSR_FEATURES (0)

Thanks
Frieder

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=dcf0cd1cc58b8e88793ad6531db9b3a47324ca09

Powered by blists - more mailing lists