Message-ID: <20240215125156.0d9f3cb7@wsk>
Date: Thu, 15 Feb 2024 12:51:56 +0100
From: Lukasz Majewski <lukma@...x.de>
To: Vladimir Oltean <olteanv@...il.com>, Andrew Lunn <andrew@...n.ch>
Cc: Tristram.Ha@...rochip.com, Oleksij Rempel <o.rempel@...gutronix.de>,
 UNGLinuxDriver@...rochip.com, netdev@...r.kernel.org, Sebastian Andrzej
 Siewior <bigeasy@...utronix.de>, George McCollister
 <george.mccollister@...il.com>, Eric Dumazet <edumazet@...gle.com>, Jakub
 Kicinski <kuba@...nel.org>, <davem@...emloft.net>
Subject: Re: [net][hsr] Question regarding HSR RedBox functionality
 implementation (preferably on KSZ9477)

Dear Community,

> 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

Please find the draft proposal of iproute2 patch to add support for HSR
interlink passing network device:

https://github.com/lmajewski/iproute2/commit/5edf2ab77786ab49419712a4defa42a600fe47c2

> 
> 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@...x.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@...x.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@...x.de

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists