Received-SPF: pass (google.com: domain of linux-kernel+bounces-150179-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Casper Andersson <casper.casan@gmail.com>
To: Lukasz Majewski <lukma@denx.de>
Cc: netdev@vger.kernel.org, Paolo Abeni <pabeni@redhat.com>, Andrew Lunn
 <andrew@lunn.ch>, Eric Dumazet <edumazet@google.com>, Vladimir Oltean
 <olteanv@gmail.com>, "David S. Miller" <davem@davemloft.net>, Jakub
 Kicinski <kuba@kernel.org>, Oleksij Rempel <o.rempel@pengutronix.de>,
 Tristram.Ha@microchip.com, Sebastian Andrzej Siewior
 <bigeasy@linutronix.de>, Ravi Gunasekaran <r-gunasekaran@ti.com>, Simon
 Horman <horms@kernel.org>, Nikita Zhandarovich <n.zhandarovich@fintech.ru>,
 Murali Karicheri <m-karicheri2@ti.com>, Jiri Pirko <jiri@resnulli.us>, Dan
 Carpenter <dan.carpenter@linaro.org>, Ziyang Xuan
 <william.xuanziyang@huawei.com>, Shigeru Yoshida <syoshida@redhat.com>,
 "Ricardo B. Marliere" <ricardo@marliere.net>, linux-kernel@vger.kernel.org
Subject: Re: [net-next PATCH v5 1/4] net: hsr: Provide RedBox support (HSR-SAN)
In-Reply-To: <20240416150359.7362c762@wsk>
References: <20240415124928.1263240-1-lukma@denx.de>
 <20240415124928.1263240-2-lukma@denx.de> <86mspt7glf.fsf@gmail.com>
 <20240416150359.7362c762@wsk>
Date: Thu, 18 Apr 2024 15:35:52 +0200
Message-ID: <86bk66hjyf.fsf@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain


Hi,

Sorry for the late reply, I was awaiting confirmation on what I can say
about the hardware I have access to. They won't let me say the name :(
but I can give some details.

On 2024-04-16 15:03 +0200, Lukasz Majewski wrote:
>> On 2024-04-02 10:58 +0200, Lukasz Majewski wrote:
>> > Changes for v3:
>> >
>> > - Modify frame passed Port C (Interlink) to have RedBox's source
>> > address (SA) This fixes issue with connecting L2 switch to
>> > Interlink Port as switches drop frames with SA other than one
>> > registered in their (internal) routing tables.  
>> 
>> > +	/* When HSR node is used as RedBox - the frame received
>> > from HSR ring
>> > +	 * requires source MAC address (SA) replacement to one
>> > which can be
>> > +	 * recognized by SAN devices (otherwise, frames are
>> > dropped by switch)
>> > +	 */
>> > +	if (port->type == HSR_PT_INTERLINK)
>> > +		ether_addr_copy(eth_hdr(skb)->h_source,
>> > +				port->hsr->macaddress_redbox);  
>> 
>> I'm not really understanding the reason for this change. Can you
>> explain it in more detail?
>
> According to the HSR standard [1] the RedBox device shall work as a
> "proxy" [*] between HSR network and SAN (i.e. "normal" ethernet)
> devices.
>
> This particular snippet handles the situation when frame from HSR node
> is supposed to be sent to SAN network. In that case the SA of HSR
> (SA_A) is replaced with SA of RedBox (SA_RB) as the MAC address of
> RedBox is known and used by SAN devices.
>
>
> Node A  hsr1  |======| hsr1 Node Redbox |   |
> (SA_A) [**]   |	     |           eth3   |---| ethX SAN
> 	      |      |        	 (SA_RB)|   |  (e.g switch)
>
>
> (the ====== represents duplicate link - like lan1,lan2)
>
> If the SA_A would be passed to SAN (e.g. switch) the switch could get
> confused as also RedBox MAC address would be used. Hence, all the
> frames going out from "Node Redbox" have SA set to SA_RB.
>
> According to [1] - RedBox shall have the MAC address.
> This is similar to problem from [2].

Thanks for the explanation, but I still don't quite follow in what way
the SAN gets confused. "also RedBox MAC address would be used", when
does this happen? Do you mean that some frames from Node A end up using
the RedBox MAC address so it's best if they all do?

I see there is already some address replacement going on in the HSR
interface, as you pointed out in [2]. And I get your idea of being a
proxy. If no one else is opposed to this then I'm fine with it too.

>> The standard does not say to modify the
>> SA. However, it also does not say to *not* modify it in HSR-SAN mode
>> like it does in other places. In HSR-HSR and HSR-PRP mode modifying
>> SA breaks the duplicate discard.
>
> IMHO, the HSR-SAN shall be regarded as a "proxy" [*] between two types
> (and not fully compatible) networks.
>
>> So keeping the same behavior for all
>> modes would be ideal.
>> 
>> I imagine any HW offloaded solutions will not modify the SA, so if
>> possible the SW should also behave as such.
>
> The HW offloading in most cases works with HSR-HSR setup (i.e. it
> duplicates frames automatically or discards them when recived - like
> ksz9477 [3]).
>
> I think that RedBox HW offloading would be difficult to achieve to
> comply with standard. One "rough" idea would be to configure
> aforementioned ksz9477 to pass all frames in its HW between SAN and HSR
> network (but then it wouldn't filter them).

I don't know anything about ksz9477. The hardware I have access to is
supposed to be compliant with 2016 version in an offloaded situation for
all modes (HSR-SAN, PRP-SAN, HSR-PRP, HSR-HSR). Though, I haven't
verified if the operation is fully according to standard. It does not
modify any addresses in HW.

Does the interlink port also reach the drivers? Does it call
port_hsr_join and try to join as an HSR port? Do we maybe need a
separate path or setting for configuring the interlink in the different
modes (SAN, HSR, PRP interlink)?

> Notes:
>
> [*] - However there is no specific "guidelines" on how the "proxy"
> shall be implemented.
>
> [**] - With current approach - the SAN MAC addresses are added to
> "node table" of Node A. For Node RedBox those are stored in a separate
> ProxyNodeTable. I'm not sure if this is the best possible approach
> [***], as ideally only MAC addresses of HSR "network" nodes shall be
> visible.
>
> [***] - I think that this "improvement" could be addressed when HSR
> support is added to Linux as it is the pre-requisite to add support for
> it to iproute2. Afterwards, the code can be further refined (as it
> would be added to net-next anyway).
>
> [****] - As I'm now "on the topic" - I can share full setup for busybox
> to run tests included to v5 of this patch set.
>
>
> Links:
>
> [1] - IEC 62439-3:2021
>
> [2] -
> https://elixir.bootlin.com/linux/latest/source/net/hsr/hsr_framereg.c#L397
>
> [3] -
> https://elixir.bootlin.com/linux/latest/source/drivers/net/dsa/microchip/ksz9477.c#L1341

BR,
Casper