On Wed, Sep 13, 2023 at 10:22:19AM +0200, Lukasz Majewski wrote:
> Why we cannot have even simpler solution - in the HSR/Wol code we read
> content of REG_SW_MAC_ADDR_0 (the actually programmed MAC) and:
>
> - If not programmed - use DSA master one (like done in mine patches)
You said here https://lore.kernel.org/netdev/20230912160326.188e1d13@wsk/
that using the DSA master address is a complication that can be avoided,
no? So why is it now part of the solution that you propose?
I thought we were in agreement to program the actual DSA user ports' MAC
addresses to hardware.
> - If already programmed:
> - check if equal to DSA master - proceed with HSR.
> - if not equal to DSA master (e.g. WoL altered) - exit HSR join
> with information that offloading is not possible
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.
> Then, the content of REG_SW_MAC_ADDR_X would determine what to do with
> it.
>
> > There are probably hundreds of implementations of this idea in the
> > kernel, but the one that comes to my mind is ocelot_mirror_get() +
> > ocelot_mirror_put(). Again, I need to mention that I know that port
> > mirroring != HSR - I'm just talking about the technique.
> >
> > There is one more thing that your reply to my observation fails to
> > address. Even with this refcount thing, you will still need to add
> > code to dsa_slave_set_mac_address() which notifies the ksz driver, so
> > that the driver can refuse MAC address changes, which would break the
> > offloads. Ack?
>
> And the above problem is not related to the DSA slave address change
> discussed earlier?
"Discussed earlier" is a bit imprecise and I don't know what you're
talking about.
There are 3 netdev kinds at play here: (a) DSA master, (b) DSA user port, (c) HSR device.
- 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. But there may be others. Like the fact that DSA user ports may
inherit the DSA master's MAC address, or they may have their own.
Limiting HSR offload and WoL to just the "inherit" case may seem a bit
arbitrary, considering that the self-address filtering from
hsr_handle_frame() looks at the port_A and port_B MAC addresses.
- 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.
The simple matter is: if you program the MAC address of a netdev (any
netdev) to hardware, 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.
It's not quite clear to me that you're making a distinction between
changing (a) and (b).
> > In principle it sounds like a plan. It just needs to be implemented.
>
> To clarify:
>
> 0. It looks like described above prevention from REG_SW_MAC_ADDR_X
> overwriting and DSA slave port MAC address change are needed.
>
> Then questions about time line:
>
> 1. The HSR code is accepted without fixes from 0. and then when other
> user (WoL) patches are posted problems from 0. needs to be addressed.
>
> or
>
> 2. To accept the HSR code you (and other community members? Russell,
> Andrew) require the fixes from 0. first.
If the DSA user port MAC address changes, 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. 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.
Hi Vladimir,
> On Wed, Sep 13, 2023 at 10:22:19AM +0200, Lukasz Majewski wrote:
> > Why we cannot have even simpler solution - in the HSR/Wol code we
> > read content of REG_SW_MAC_ADDR_0 (the actually programmed MAC) and:
> >
> > - If not programmed - use DSA master one (like done in mine
> > patches)
>
> You said here
> https://lore.kernel.org/netdev/20230912160326.188e1d13@wsk/ that
> using the DSA master address is a complication that can be avoided,
> no? So why is it now part of the solution that you propose?
The patch v4 uses DSA master (HSR device) MAC address as you advised.
My suggestion was to just use the value (single) if already written
there. If it differs at join time - then just bail out.
>
> 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.
>
> > - If already programmed:
> > - check if equal to DSA master - proceed with HSR.
> > - if not equal to DSA master (e.g. WoL altered) - exit HSR
> > join with information that offloading is not possible
>
> 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?
>
> > Then, the content of REG_SW_MAC_ADDR_X would determine what to do
> > with it.
> >
> > > There are probably hundreds of implementations of this idea in the
> > > kernel, but the one that comes to my mind is ocelot_mirror_get() +
> > > ocelot_mirror_put(). Again, I need to mention that I know that
> > > port mirroring != HSR - I'm just talking about the technique.
> > >
> > > There is one more thing that your reply to my observation fails to
> > > address. Even with this refcount thing, you will still need to add
> > > code to dsa_slave_set_mac_address() which notifies the ksz
> > > driver, so that the driver can refuse MAC address changes, which
> > > would break the offloads. Ack?
> >
> > And the above problem is not related to the DSA slave address change
> > discussed earlier?
>
> "Discussed earlier" is a bit imprecise and I don't know what you're
> talking about.
>
> There are 3 netdev kinds at play here: (a) DSA master, (b) DSA user
> port, (c) HSR device.
>
> - 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?
> But there may be others. Like the
> fact that DSA user ports may inherit the DSA master's MAC address, or
> they may have their own. Limiting HSR offload and WoL to just the
> "inherit" case may seem a bit arbitrary, considering that the
> self-address filtering from hsr_handle_frame() looks at the port_A
> and port_B MAC addresses.
As described earlier - the self-address mac filtering to work must have
lan1, lan2, HSR, eth0 MAC address equal. HSR HW offloading will just not
work if they differ.
>
> - 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? Shouldn't it
be forbidden, and HSR ports shall inherit MAC address of HSR device
(hsr0) ?
>
> 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 ?
> , 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?
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.
How point 2 can be achieved (if possible) ?
> It's not quite clear to me that you're making a distinction between
> changing (a) and (b).
>
> > > In principle it sounds like a plan. It just needs to be
> > > implemented.
> >
> > To clarify:
> >
> > 0. It looks like described above prevention from REG_SW_MAC_ADDR_X
> > overwriting and DSA slave port MAC address change are needed.
> >
> > Then questions about time line:
> >
> > 1. The HSR code is accepted without fixes from 0. and then when
> > other user (WoL) patches are posted problems from 0. needs to be
> > addressed.
> >
> > or
> >
> > 2. To accept the HSR code you (and other community members? Russell,
> > Andrew) require the fixes from 0. first.
>
> If the DSA user port MAC address changes,
You mean lan1, lan2, which are joined with hsr0?
> 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?
> 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.
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]