Date: Tue, 12 Sep 2023 17:06:41 +0200
From: Lukasz Majewski <lukma@...x.de>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Tristram.Ha@...rochip.com, Eric Dumazet <edumazet@...gle.com>, Andrew
 Lunn <andrew@...n.ch>, 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 04:03:26PM +0200, Lukasz Majewski wrote:
> > > In any case, as long as it's the DSA master's address that we
> > > program to hardware, then I see it as inevitable to add a new
> > > struct dsa_switch_ops :: master_addr_change() function  
> > 
> > Please correct my understanding. The above change would affect the
> > whole DSA subsystem. It would require to have the core DSA modified
> > and would affect its operation?  
> 
> Uhm, yes, that would be the idea. If we were going to track changes to
> the DSA master's MAC address, we should do it from the DSA framework
> which has the existing netdev notifier listener infrastructure in
> place.
> 
> > > Or you can argue that dragging the DSA master into the discussion
> > > about how we should program REG_SW_MAC_ADDR_0 is a complication.  
> > 
> > Yes, it is IMHO the complication.  
> 
> Ok, it's a point of view.
> 
> > git grep -n "REG_SW_MAC_ADDR_0"
> > drivers/net/dsa/microchip/ksz8795_reg.h:326:#define
> > REG_SW_MAC_ADDR_0 0x68 
> > drivers/net/dsa/microchip/ksz9477.c:1194:
> >      ksz_write8(dev, REG_SW_MAC_ADDR_0 + i,
> > 
> > drivers/net/dsa/microchip/ksz9477_reg.h:169:#define
> > REG_SW_MAC_ADDR_0 0x0302
> > 
> > In the current net-next the REG_SW_MAC_ADDR_0 is altered used (the
> > only usage are now with mine patches on ksz9477).
> > 
> > To sum up:
> > 
> > 1. Up till now in (net-next) REG_SW_MAC_ADDR_0 is ONLY declared for
> > Microchip switches. No risk for access - other than HSR patches.  
> 
> I know (except for Oleksij's WoL patches, which will eventually be
> resubmitted).

Are we debating about some possible impact on patches which were posted
and (in a near future?) would be reposted?

> 
> > 2. The MAC address alteration of DSA master and propagation to
> > slaves is a generic DSA bug.  
> 
> Which can be/will be fixed. The diff I've included in the question to
> Jakub closes it, in fact.

Ok.

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


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