Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Date:   Tue, 12 Sep 2023 12:29:09 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Lukasz Majewski <lukma@denx.de>
Cc:     Tristram.Ha@microchip.com, Eric Dumazet <edumazet@google.com>,
        Andrew Lunn <andrew@lunn.ch>, davem@davemloft.net,
        Woojung Huh <woojung.huh@microchip.com>,
        Oleksij Rempel <o.rempel@pengutronix.de>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, UNGLinuxDriver@microchip.com,
        Oleksij Rempel <linux@rempel-privat.de>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW
 offloading for KSZ9477
Message-ID: <20230912092909.4yj4b2b4xrhzdztu@skbuf>
References: <20230906152801.921664-1-lukma@denx.de>
 <20230911165848.0741c03c@wsk>
 <20230911160501.5vc4nttz6fnww56h@skbuf>
 <20230912101748.0ca4eec8@wsk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230912101748.0ca4eec8@wsk>
Precedence: bulk

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.


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


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.