Message-ID: <20230912160326.188e1d13@wsk>
Date:   Tue, 12 Sep 2023 16:03:26 +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 10:17:48AM +0200, Lukasz Majewski wrote:
> > IMHO, somebody who will use HSR will not fiddle with mac addresses
> > of LAN1 and ETH0. It will be setup by savvy user once at boot up.  
> 
> The point is, it has to work in all configurations that are accepted
> by the kernel.
> 
> > Please correct me if I'm wrong, but the above issue (with lack of
> > sync of mac address change in DSA master and its ports) seems to be
> > affecting HSR support in a minimal way (when considering the
> > above).  
> 
> It's a different (and very old) bug for sure. But it has impact upon
> the v4 patch set as you've presented it here.
> 
> > If I may ask - what is your suggestion to have the HSR join feature
> > merged for KSZ9477 SoC ?  
> 
> Anything that makes sense and works is worth considering.
> 
> For example, one can argue that since we already have this pattern in
> 2 places in net/dsa/slave.c:
> 
> 	/* If the port doesn't have its own MAC address and relies on
> the DSA
> 	 * master's one, inherit it again from the new DSA master.
> 	 */
> 	if (is_zero_ether_addr(dp->mac))
> 		eth_hw_addr_inherit(dev, master);
> 
> then the consistent way to react to NETDEV_CHANGEADDR events on the
> master is to change the user ports' MAC address yet again, to track
> the master.
> 
> 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, similar to
> master_state_change(). The driver would always be notified of the
> current (even initial) MAC address, and it could update the hardware
> registers (for WoL, pause frames and HSR self-address filtering, in
> this case).
> 
> The above 2 changes could be one way to ensure that if a HSR device
> was accepted for offload initially, it will remain in a configuration
> that will keep working.
> 

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?

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

> An
> API internal to the microchip ksz driver could be added, where the
> user ports on which the various specialty features are enabled (HSR,
> WoL) take a reference on the REG_SW_MAC_ADDR_0 with their MAC address.
> If the reference on REG_SW_MAC_ADDR_0 gets bumped from 0 to 1, the
> hardware is programmed with the requesting port's MAC address. If the
> reference is already elevated, then a request to increase it, coming
> from another port, is accepted or denied, depending on whether the MAC
> address of that port is equal to what's programmed into
> REG_SW_MAC_ADDR_0 or not.
> The refusal gets propagated to the user,
> together with an informative extack message. 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().
> 

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.

2. The MAC address alteration of DSA master and propagation to slaves
is a generic DSA bug.

Considering the above - the HSR implementation is safe (to the extend
to the whole DSA subsystem current operation). Am I correct?


> 
> So, there are some options to pick from.
> 
> > Will the above problem block the HSR offloading support mainlining,
> > even when the self mac address filtering is one of four HW based
> > features for this SoC?  
> 
> I don't know, that depends on you.




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