Message-ID: <20230913135102.hoyl4tifyf77kdo2@skbuf>
Date:   Wed, 13 Sep 2023 16:51:02 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Lukasz Majewski <lukma@...x.de>
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

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.

> > 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.

Powered by blists - more mailing lists