Message-ID: <20230913141548.70658940@wsk>
Date:   Wed, 13 Sep 2023 14:15:48 +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 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: lukma@...x.de

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists