Message-ID: <20230913102219.773e38f8@wsk>
Date:   Wed, 13 Sep 2023 10:22:19 +0200
From:   Lukasz Majewski <lukma@...x.de>
To:     Vladimir Oltean <olteanv@...il.com>, Andrew Lunn <andrew@...n.ch>
Cc:     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 Tue, Sep 12, 2023 at 05:06:41PM +0200, Lukasz Majewski wrote:
> > Are we debating about some possible impact on patches which were
> > posted and (in a near future?) would be reposted?  
> 
> We are discussing the ways in which a multi-purpose register should be
> programmed. Not "the impact on patches" per se, because Oleksij will
> have to adapt no matter what you do, but rather the options that
> remain available to him, after the first feature that makes use of the
> multi-purpose register makes its way to mainline.
> 
> > > > Considering the above - the HSR implementation is safe (to the
> > > > extend to the whole DSA subsystem current operation). Am I
> > > > correct? 
> > > 
> > > If we exclude the aforementioned bug (which won't be a bug
> > > forever), there still exists the case where the MAC address of a
> > > DSA user port can be changed. The HSR driver has a
> > > NETDEV_CHANGEADDR handler for this as well, and updates its own
> > > MAC address to follow the port. If that is allowed to happen
> > > after the offload, currently it will break the offload.  
> > 
> > But then we can have struct ksz_device extended with bitmask -
> > hw_mac_addr_ports, which could be set to ports (WoL or HSR) when
> > REG_MAC_ADDR_0 is written.
> > 
> > If WoL would like to alter it after it was written by HSR, then the
> > error is presented (printed) to the user and we return.
> > 
> > The same would be with HSR altering the WoL's MAC in-device setup.
> > 
> > 
> > The HSR or WoL can be added without issues (the first one which is
> > accepted).
> > 
> > Then the second feature would need to implement this check.  
> 
> This is more or less a rehash of what I proposed as option 2, except
> for the fact that you suggest a port mask and I suggest a proper
> refcount_t. And the reason why I suggest that is to allow the
> "WoL+HSR on the same port" to work. With your proposal, both the HSR
> and WoL code paths would set the same bit in hw_mac_addr_ports, which
> would become problematic when the time comes to unset it. Not so much
> when every port calls refcount_inc() per feature. With WoL+HSR on the
> same port, the MAC address would have a refcount of 2, and you could
> call port_hsr_leave() and that refcount would just drop to 1 instead
> of letting go.
> 

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)

- 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

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?

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


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