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.
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: [email protected]
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).
> 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.
> 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.