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@eno2: <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@eno2: <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@eno2: <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@swp0: <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@swp0: <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@swp0: <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@swp2: <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@swp2: <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@swp2: <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@swp2: <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.
On Wed, Sep 13, 2023 at 04:51:02PM +0300, Vladimir Oltean wrote:
> > 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.
Can you please test the attached incremental patch, which applies on top
of your RFC v4 series? It contains an implementation of my own review feedback.
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@eno2: <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@eno2: <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@eno2:
> <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@swp0: <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@swp0: <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@swp0:
> <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@swp2: <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@swp2: <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@swp2:
> <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@swp2: <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: [email protected]
Hi Vladimir,
> On Wed, Sep 13, 2023 at 04:51:02PM +0300, Vladimir Oltean wrote:
> > > 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.
>
> Can you please test the attached incremental patch, which applies on
> top of your RFC v4 series? It contains an implementation of my own
> review feedback.
Thanks for preparing the patch - it clarified all the point from
previous e-mails... (and shed some light on mine understanding of DSA
internals)
It works when applied on top of v4. No performance regressions (with
nuttcp) observed.
I've also tested the scenario when one tried to alter lan1 after HW
offloading enabled. It was not possible to alter the MAC address.
As fair as I understood from the commit message - some part of this
patch needs to be applied before HSR offloading v4.
Hence I will wait for it to be posted and upstreamed.
Only then some of this patch code would be squashed to v5 of hsr
support.
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: [email protected]