On Wed, Jul 6, 2022 at 4:33 PM Vladimir Oltean [off-list ref] wrote:

quoted

quoted

quoted

@@ -919,6 +920,9 @@ static void mv88e6xxx_mac_link_down(struct dsa_switch *ds, int port,
      if (err)
              dev_err(chip->dev,
                      "p%d: failed to force MAC link down\n", port);
+     else
+             if (mv88e6xxx_port_is_locked(chip, port, true))
+                     mv88e6xxx_atu_locked_entry_flush(ds, port);

This is superfluous, is it not? The bridge will transition a port whose
link goes down to BR_STATE_DISABLED, which will make dsa_port_set_state()
fast-age the dynamic FDB entries on the port, which you've already
handled below.

I removed this code, but then on link down the locked entries were not
cleared out. Something not as thought?

What was the port's STP state before the link down event, and did it
change after the link down?

The stp state is FORWARDING.

If the STP state wasn't LEARNING or FORWARDING, there weren't supposed
to be dynamic FDB entries on the port in the first place, so DSA says
there's nothing to flush, and doesn't call dsa_port_fast_age().
Are there dynamic FDB entries being installed on a port that isn't
in a state that's supposed to learn? I guess the answer is yes.
Is that what you want, or should the locked entries be recorded only in
the LEARNING or FORWARDING states, otherwise discarded?

Learning is off as has been discussed, and I do want the locked
entries to be dynamic in the sense that the driver removes them after
the system ageing time has passed.

What you actually want to say is: "mv88e6xxx_port_set_lock() is also
called when the DSA port joins a bridge, due to the switchdev attribute
replay logic present in dsa_port_switchdev_sync_attrs()".

Which, by the way, is logic that you've added yourself, in commit
b9e8b58fd2cb ("net: dsa: Include BR_PORT_LOCKED in the list of synced
brport flags") ;)

You are free to return early from mv88e6xxx_port_set_lock() if nothing has
changed. The DSA layer doesn't keep track of the locked state of the
port so it cannot deduce whether propagating to the switch driver is
necessary or not.

I think I can safely call mv88e6xxx_atu_locked_entry_flush() from
mv88e6xxx_port_set_lock() when locked is off as the port setup for the
respective port must have been completed successfully.

quoted

When added they are added with bridge FDB flags: extern_learn offload
locked, with a SWITCHDEV_FDB_ADD_TO_BRIDGE event. So they are owned by
the driver.
When the driver deletes the locked entry the bridge FDB entry is
removes by the SWITCHDEV_FDB_DEL_TO_BRIDGE event from the driver. That
seems quite fair?

I'm just pointing out that you left other (probably unintended) code
paths for which the SWITCHDEV_FDB_DEL_TO_BRIDGE notifier is quite
useless. I haven't yet looked at your newest revision to see what
changed there.

I guess I should add a boolean to tell if
mv88e6xxx_atu_locked_entry_purge() should send a notification or not.
So that port_fdb_del() will not cause a SWITCHDEV_FDB_DEL_TO_BRIDGE
event.

quoted

quoted

quoted

quoted

Why is the rtnl_unlock() outside the switch statement but the rtnl_lock() inside?
Not to mention, the dsa_port_to_bridge_port() call needs to be under rtnl_lock().

Just a small optimization as I also have another case of the switch
(only one switch case if
you didn't notice) belonging to the next patch set regarding dynamic
ATU entries.

What kind of optimization are you even talking about? Please get rid of
coding patterns like this, sorry.

Right!

Right what? I'm genuinely curious what optimization are you talking about.

I am just confirming that what you wrote is correct, e.g. the
"Right!". So I have fixed that. :-)

Just out of curiosity, are you even trying, are you looking at the
difference using a monospace font?

quoted

Another issue...

I have removed the timers as they are superfluous and now just use the
worker and jiffies. But I have found that the whole ageing time seems
to be broken on the 5.17 kernel I am running. I don't know if it has
been fixed, but the ageing timeout is supposed to be given in seconds.
Here is the output from various functions after the command "ip link
set dev br0 type bridge ageing_time 1500" (that is nominally 1500
seconds according to man page!):

dsa_switch_ageing_time: ageing_time 10000, ageing_time_min 1000,
ageing_time_max 3825000
mv88e6xxx_set_ageing_time: set ageing time to 10000
br0: failed (err=-34) to set attribute (id=6)
dsa_switch_ageing_time: ageing_time 15000, ageing_time_min 1000,
ageing_time_max 3825000
mv88e6xxx_set_ageing_time: set ageing time to 15000

The 15000 set corresponds to 150 seconds! (I hardcoded the dsa
ageing_time_min to 1000)

Are you talking about this known problem, that the ageing time values in
seconds need to be scaled up by a factor of USER_HZ when passed to the
kernel?
https://www.spinics.net/lists/netdev/msg672070.html
https://www.spinics.net/lists/netdev/msg567332.html

It might be so, but there is another factor 10 which might be
regarding topology change as I understand. If I want a ageing timeout
of say 15 or 30 seconds, that hardly seems possible?