Message-ID: <20230914224557.0ca0f057@wsk>
Date:   Thu, 14 Sep 2023 22:45:57 +0200
From:   Lukasz Majewski <lukma@...x.de>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>, Tristram.Ha@...rochip.com,
        Eric Dumazet <edumazet@...gle.com>, davem@...emloft.net,
        Woojung Huh <woojung.huh@...rochip.com>,
        Oleksij Rempel <o.rempel@...gutronix.de>,
        Florian Fainelli <f.fainelli@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, UNGLinuxDriver@...rochip.com,
        Oleksij Rempel <linux@...pel-privat.de>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW
 offloading for KSZ9477

Hi Vladimir,

> On Wed, Sep 13, 2023 at 02:15:48PM +0200, Lukasz Majewski wrote:
> > > I thought we were in agreement to program the actual DSA user
> > > ports' MAC addresses to hardware.  
> > 
> > No - v4 of this solution uses HSR net device MAC address, which is
> > supposed to be the same as DSA master.  
> 
> By "in agreement" I mean "as a result of the discussion on v4 [ this
> discussion ], where it became obvious that the DSA master solution is
> not so robust". I hope the 12 emails already exchanged on this patch
> set weren't for nothing.
> 
> > > With KSZ switches, a single CPU port is supported, so all ports
> > > share the same DSA master. So if the contents of
> > > REG_SW_MAC_ADDR_0 is different from the DSA master (the same DSA
> > > master that was used for an earlier HSR offload), why do you
> > > infer that it was altered by WoL? It makes no sense.  
> > 
> > So where is the issue? The only issue is that somebody would change
> > DSA master (and hence HSR) MAC address, so the REG_SW_MAC_ADDR_0
> > would not be changed. Or do I miss something?  
> 
> Changing the DSA master address and changing the HSR MAC address
> (indirectly through the ports' address) are different operations.
> The DSA master's address and the user ports' address are not
> necessarily in sync.
> 
> Each address change is problematic in its own way, and each problem
> needs to be tackled in its own way, depending on which MAC address you
> desire for REG_SW_MAC_ADDR_0 to track.
> 
> But yes, the only issue is that the MAC address can be changed at
> runtime, after the initial offload.
> 
> > > - Changing the MAC address of (a) triggers a pre-existing bug.
> > > That bug can be separated from the HSR offload discussion if the
> > > HSR offload decides to not program the DSA master's MAC address to
> > > hardware, but a different MAC address. The pre-existence of the
> > > DSA bug is not a strong enough argument per se to avoid
> > > programming the DSA master's address to hardware.  
> > 
> > And this is how v4 is implemented. Or not?  
> 
> If A == B initially, then there are 2 ways you can change that
> condition. You can change A, or you can change B. Replace "A" with
> "the DSA master's address" and "B" with "the HSR device's address".
> 
> > > - Changing the MAC address of (c) does not seem directly possible,
> > > but:
> > > 
> > > - Changing the MAC address of (b) also triggers (c) - see
> > > hsr_netdev_notify(). This is because the HSR driver makes sure
> > > that the addresses of port_A, port_B and the HSR device are equal
> > > at all times.  
> > 
> > Why changing HSR port would affect HSR device MAC address?  
> 
> I don't have a simpler answer than "because that's what the code
> does".
> 
> If you don't have time to experiment to convince yourself that this is
> the case, below is a set of commands that should hopefully clarify.
> 
> $ ip link
> 6: eno2: <BROADCAST,MULTICAST> mtu 1508 qdisc noop state DOWN group
> default qlen 1000 link/ether 2a:af:42:b7:73:11 brd ff:ff:ff:ff:ff:ff
>     altname end0
>     altname enp0s0f2
> 7: swp0@...2: <BROADCAST,MULTICAST,M-DOWN> mtu 1504 qdisc noop state
> DOWN group default qlen 1000 link/ether a6:f4:af:fd:fc:73 brd
> ff:ff:ff:ff:ff:ff 8: swp1@...2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500
> qdisc noop state DOWN group default qlen 1000 link/ether
> a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff 9: swp2@...2:
> <BROADCAST,MULTICAST,M-DOWN> mtu 1504 qdisc noop state DOWN group
> default qlen 1000 link/ether a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff
> 10: sw0p0@...0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop
> state DOWN group default qlen 1000 link/ether a6:f4:af:fd:fc:73 brd
> ff:ff:ff:ff:ff:ff 11: sw0p1@...0: <BROADCAST,MULTICAST,M-DOWN> mtu
> 1500 qdisc noop state DOWN group default qlen 1000 link/ether
> a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff 12: sw0p2@...0:
> <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group
> default qlen 1000 link/ether a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff
> 13: sw2p0@...2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop
> state DOWN group default qlen 1000 link/ether a6:f4:af:fd:fc:73 brd
> ff:ff:ff:ff:ff:ff 14: sw2p1@...2: <BROADCAST,MULTICAST,M-DOWN> mtu
> 1500 qdisc noop state DOWN group default qlen 1000 link/ether
> a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff 15: sw2p2@...2:
> <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group
> default qlen 1000 link/ether a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff
> 16: sw2p3@...2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop
> state DOWN group default qlen 1000 link/ether a6:f4:af:fd:fc:73 brd
> ff:ff:ff:ff:ff:ff # Simplified in a table for brevity. The format
> will be kept below $ ip link show ... sw0p0              sw0p1
>       DSA master          hsr0 addr a6:f4:af:fd:fc:73
> a6:f4:af:fd:fc:73  a6:f4:af:fd:fc:73   n/a
> 
> $ ip link add hsr0 type hsr slave1 sw0p0 slave2 sw0p1 version 1
> [   70.033711] sja1105 spi2.0 sw0p0: entered promiscuous mode
> [   70.058066] sja1105 spi2.0 sw0p1: entered promiscuous mode
> Warning: dsa_core: Offloading not supported.
> 
> $ ip link show ...
>      sw0p0              sw0p1              DSA master          hsr0
> addr a6:f4:af:fd:fc:73  a6:f4:af:fd:fc:73  a6:f4:af:fd:fc:73
> a6:f4:af:fd:fc:73
> 
> $ ip link set swp0 address 00:01:02:03:04:05 # DSA master
> 
> $ ip link show ...
>      sw0p0              sw0p1              DSA master          hsr0
> addr a6:f4:af:fd:fc:73  a6:f4:af:fd:fc:73  00:01:02:03:04:05
> a6:f4:af:fd:fc:73
> 
> $ ip link set sw0p0 address 00:01:02:03:04:06
> 
> $ ip link show ...
>      sw0p0              sw0p1              DSA master          hsr0
> addr 00:01:02:03:04:06  a6:f4:af:fd:fc:73  00:01:02:03:04:05
> 00:01:02:03:04:06
> 
> $ ip link set sw0p1 address 00:01:02:03:04:07
> 
> $ ip link show ...
>      sw0p0              sw0p1              DSA master          hsr0
> addr 00:01:02:03:04:06  00:01:02:03:04:07  00:01:02:03:04:05
> 00:01:02:03:04:06
> 
> > Shouldn't it be forbidden, and HSR ports shall inherit MAC address
> > of HSR device (hsr0) ?  
> 
> Since HSR does _actually_ track the MAC address of port_A (sw0p0
> above), I guess it would be best if the API introduced would always
> program REG_SW_MAC_ADDR_0 with the MAC address of the first port that
> joins the HSR (and requests a reference to REG_SW_MAC_ADDR_0). That
> way, the API should work with no changing for WoL as well. Anyway -
> minor difference between first user port and HSR dev_addr.

I've made wrong assumptions - I thought that when we have

hsr0    -> lan1
	-> lan2

it is only possible to adjust hsr0 MAC address as lan1,lan2 are in some
way "slaves" for hsr0.

In other words - I thought that lan1, lan2 "disappear" from "normal"
DSA control as they after "join" are "represented" to DSA by hsr0 (the
below hierarchy).

eth0 	-->  lan3
	-->  lan4
	-->  hsr0	-> lan2
			-> lan1

And then you explained a use case where one can adjust MAC address of
lan1 and it would be propagated to hsr0.

Now it is clear.

> 
> > > The simple matter is: if you program the MAC address of a netdev
> > > (any netdev) to hardware  
> > 
> > Which netdev in this case? lan1, lan2, hsr0 or eth0 ?  
> 
> Any netdev. It is a general statement which had a continuation, which
> you've interrupted.
> 
> > > , then for a correct and robust implementation, you
> > > need to make sure that the hardware will always be in sync with
> > > that address, keeping in mind that the user may change it. Either
> > > you deny changes, or you update the hardware when the address is
> > > updated. 
> > 
> > Why I cannot just:
> > 
> > 1. Check on hsr_join if lan1, lan2, hsr0 and eth0 MAC is equal and
> > write it to REG_SW_MAC_ADDR_0?  
> 
> This is actually unnecessary if you implement the reference scheme on
> REG_SW_MAC_ADDR_0 that I've suggested. Checking the MAC address of
> eth0 is unnecessary in any case, if you don't program it to hardware.
> 
> > 2. Forbid the change of lan1/lan2/eth0/hsr0 if it may affect HSR HW
> > offloading? If user want to change it - then one shall delete hsr0
> > net device, change MAC and create it again.  
> 
> I've been saying since yesterday that you should do this.
> 
> > How point 2 can be achieved (if possible) ?  
> 
> Re-read earlier emails.
> 
> > > If the DSA user port MAC address changes,  
> > 
> > You mean lan1, lan2, which are joined with hsr0?  
> 
> Yup. I've been saying this for a number of emails already:
> https://lore.kernel.org/netdev/20230912092909.4yj4b2b4xrhzdztu@skbuf/
> 
> | The ports which hold a reference on REG_SW_MAC_ADDR_0 cannot have
> their | MAC address changed - and for this, you'd have to add a hook
> to | dsa_switch_ops (and thus to the driver) from
> | dsa_slave_set_mac_address().
> 
> > > and REG_SW_MAC_ADDR_0 was
> > > previously programmed with it, and nothing is done in reaction to
> > > this, then this is a problem with the HSR offload.   
> > 
> > This shall be just forbidden?  
> 
> Yup.
> 
> > > So no, it's not
> > > just a problem with upcoming WoL patches as you imply. You need to
> > > integrate a solution to that problem as part of your HSR patches.
> > >  
> > 
> > I'm really stunned, how much extra work is required to add two
> > callbacks to DSA subsystem (to have already implemented feature)
> > for a single chip IC.  
> 
> Some observations are best kept to yourself. This is only the second
> HSR offload in the entire kernel. To complain that the infrastructure
> needs some extensions, for something that wasn't even needed for the
> first implementation (tracking a MAC address), is unrealistic.




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