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: [email protected]
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.
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?
In principle it sounds like a plan. It just needs to be implemented.
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: [email protected]