Re: [net][hsr] Question regarding HSR RedBox functionality implementation (preferably on KSZ9477)

[Lukasz Majewski <lukma@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 6055 bytes --]

Hi Vladimir, Andrew,

> Hi Vladimir,
> 
> > Hi Lukasz,
> > 
> > On Tue, Jan 09, 2024 at 01:32:34PM +0100, Lukasz Majewski wrote:  
> > > However, I'm wondering how the mainline Linux kernel could handle
> > > HSR RedBox functionality (on document [1], Figure 2. we do have
> > > "bridge" - OSI L2).
> > > 
> > > To be more interesting - br0 can be created between hsr0 and e.g.
> > > lan3. But as expected communication breaks on both directions (to
> > > SAN and to HSR ring).    
> > 
> > Yes, I suppose this is how a RedBox should be modeled. In principle
> > it's identical to how bridging with LAG ports (bond, team) works -
> > either in software or offloaded. 
> > The trouble is that the HSR driver
> > seems to only work with the DANH/DANP roles (as also mentioned in
> > Documentation/networking/dsa/dsa.rst). I don't remember what doesn't
> > work (or if I ever knew at all).  
> 
> In the newest net-next only PRP_TLV_REDBOX_MAC is defined, which seems
> to be REDBOX for DAN P (PRP).
> 
> > It might be the address substitution
> > from hsr_xmit() that masks the MAC address of the SAN side device?
> >   
> 
> This needs to be further investigated.

I've looked into the HSR and Bridge drivers internals.

The creation of hsrX device from command line ends with
hsr_dev_finalize(), which then calls hsr_add_port() for HSR_PT_MASTER,
HSR_PT_SLAVE_A and HSR_PT_SLAVE_B.

Those three ports allows HSR DANH operation.

For Redbox (SANH) it looks like the HSR_PT_INTERLINK shall be used.

When calling:
ip link add name br0 type bridge; ip link set br0 up;
ip link set hsr1 master br0; ip link set lan3 master br0;

(the hsr1 is the SW HSR - NO offloading).

The br_add_if() is called, which calls:
netdev_rx_handler_register(dev, br_get_rx_handler(dev), p);

This setup of RX handler is problematic, as for HSR INTERLINK port the
hsr_handle_frame() shall be used (so proper port->type =
HSR_PT_INTERLINK would be used in the hsr driver).

When the bridge handler is used, all the incoming frames are set as
HSR_PT_MASTER type (and only some frames with "reserved" MAC address
are passed to HSR network).


The problem:
############
Proper setup of rx_handler for network device, so L2 frames are handled
by HSR driver.


I've tried:
###########
1. In dsa_user_prechangeupper() (or ksz_port_bridge_join)
if (netdev_is_rx_handler_busy(dp->user)) {
	rtnl_trylock();
	netdev_rx_handler_unregister(dp->user);
	rtnl_unlock();
}

hsr_add_interlink_port(dev->hsr_dev, dp->user, extack);

But it looks like it is too low-level code to play with it by hand as
several WARN_ONs are triggered.

2. Stop/unlink the bridge slave port (lan3) and then call
hsr_add_interlink_port() for it.

However, there are several warnings as well and this approach may harm
the "bridging" modelling approach as I would use 'unlinked' device for
normal operation.


Idea:
#####

1. In the br_get_rx_handler() I could add check if "sibling" port is the
HSR master one and then set the RX handler for lan3 to this one.

However, this would require "bridge" driver modification for HSR
operation.

2. Maybe the way of HSR port creation shall be augmented [*]?
For example from:
ip link add name hsr1 type hsr slave1 lan4 slave2 lan5 supervision 45

to:
ip link add name hsr1 type hsr slave1 lan4 slave2 lan5 interlink lan3
supervision 45

In that way I could modify only the hsr_dev_finalize() and everything
would be managed by hsr driver?




Or maybe I've overlooked something, and there is easier solution for
this problem?



Note:

[*] - this approach solves also another problem - when we do have e.g 5
ports in the switch how one can know which lanX port shall be added to
HSR network? With the current approach the hsr1 device first needs to
be created and then it is implicitly assumed that the next bridged port
(lan3) shall be the Interlink one for HSR.

> 
> > > Is there a similar functionality already present in the Linux
> > > kernel (so this approach could be reused)?
> > > 
> > > My (very rough idea) would be to extend KSZ9477 bridge join
> > > functions to check if HSR capable interface is "bridged" and then
> > > handle frames in a special way.
> > > 
> > > However, I would like to first ask for as much input as possible -
> > > to avoid any unnecessary work.    
> > 
> > First I'd figure out why the software data path isn't working, and
> > if it can be fixed.   
> 
> +1
> 
> > Then, fix that if possible, and add a new selftest to
> > tools/testing/selftests/net/forwarding/, that should pass using veth
> > interfaces as lower ports.
> > 
> > Then, offloading something that has a clear model in software should
> > be relatively easy, though you might need to add some logic to DSA.
> > This is one place that needs to be edited, there may be others.
> > 
> > 	/* dsa_port_pre_hsr_leave is not yet necessary since hsr
> > devices cannot
> > 	 * meaningfully placed under a bridge yet
> > 	 */
> >   
> 
> Ok, the LAG approach in /net/dsa/user.c can be used as an example.
> 
> Thanks for shedding some light on this issue :-)
> 
> > > 
> > > Thanks in advance for help :-)
> > > 
> > > Link:
> > > 
> > > [1] -
> > > https://ww1.microchip.com/downloads/en/Appnotes/AN3474-KSZ9477-High-Availability-Seamless-Redundancy-Application-Note-00003474A.pdf
> > > 
> > > 
> > > Best regards,
> > > 
> > > Lukasz Majewski    
> 
> 
> 
> 
> 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




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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]