On Wed, Jul 6, 2022 at 10:56 AM Vladimir Oltean [off-list ref] wrote:

On Tue, Jun 28, 2022 at 02:26:43PM +0200, Hans S wrote:

quoted

quoted

Dumb question: if you only flush the locked entries at fast age if the
port is locked, then what happens with the existing locked entries if
the port becomes unlocked before an FDB flush takes place?
Shouldn't mv88e6xxx_port_set_lock() call mv88e6xxx_atu_locked_entry_flush()
too?

BTW:

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?

quoted

That was my first thought too, but the way the flags are handled with the mask etc, does so that
mv88e6xxx_port_set_lock() is called when other flags change. It could be done by the transition
from locked->unlocked by checking if the port is locked already.

Why does mv88e6xxx_port_set_lock() get called when other flags change?

That is what seems to happen, but maybe I am wrong. Looking at the dsa
functions dsa_port_inherit_brport_flags() and
dsa_port_clear_brport_flags(), they set the mask for which underlying
function is called, and it looks to me that they call once for all the
flags: BR_LEARNING, BR_FLOOD, BR_MCAST_FLOOD, BR_BCAST_FLOOD,
BR_PORT_LOCKED?

quoted

On the other hand, the timers will timeout and the entries will be removed anyhow.

quoted

quoted

quoted

+static void mv88e6xxx_atu_locked_entry_timer_work(struct atu_locked_entry *ale)

Please find a more adequate name for this function.

Any suggestions?

Not sure. It depends on whether you leave just the logic to delete a
locked ATU entry, or also the switchdev FDB_DEL_TO_BRIDGE notifier.
In any case, pick a name that reflects what it does. Something with
locked_entry_delete() can't be too wrong.

quoted

quoted

From the discussion with Ido and Nikolay I get the impression that
you're not doing the right thing here either, notifying a
SWITCHDEV_FDB_DEL_TO_BRIDGE from what is effectively the
SWITCHDEV_FDB_DEL_TO_DEVICE handler (port_fdb_del).

Hmm, my experience tells me that much is opposite the normal
conventions when dealing with
locked ports, as there was never switchdev notifications from the
driver to the bridge before, but
that is needed to keep ATU and FDB entries in sync.

On delete you mean? So the bridge signals switchdev a deletion of a
locked FDB entry (as I pointed out, this function gets indirectly called
from port_fdb_del), but it won't get deleted until switchdev signals it
back, is what you're saying?

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?

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!

quoted

quoted

Please, no "if (chiplock) mutex_lock()" hacks. Just lockdep_assert_held(&chip->reg_lock),
which serves both for documentation and for validation purposes, ensure
the lock is always taken at the caller (which in this case is super easy)
and move on.

As I am calling the function in if statement checks, it would make
that code more messy, while with
this approach the function can be called from anywhere. I also looked
at having two functions, with
one being a wrapper function taking the lock and calling the other...

There are many functions in mv88e6xxx that require the reg_lock to be
held, there's nothing new or special here.

Now I take the lock in the function regardless. No boolean.

quoted

quoted

quoted

+
+     if (mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_CTL0, &reg))
+             goto out;

It would be good to actually propagate the error to the caller and
"locked" via a pass-by-reference bool pointer argument, not just say
that I/O errors mean that the port is unlocked.

Again the wish to be able to call it from if statement checks,.

quoted

quoted

+     reg &= MV88E6XXX_PORT_ASSOC_VECTOR_PAV_MASK;
+     if (locked) {
+             reg |= MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG |
+                     MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT |
+                     MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT |
+                     MV88E6XXX_PORT_ASSOC_VECTOR_HOLD_AT_1;

I'd suggest aligning these macros vertically.

They are according to the Linux kernel coding standard wrt indentation afaik.

Compare:

                reg |= MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG |
                        MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT |
                        MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT |
                        MV88E6XXX_PORT_ASSOC_VECTOR_HOLD_AT_1;

with:

                reg |= MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG |
                       MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT |
                       MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT |
                       MV88E6XXX_PORT_ASSOC_VECTOR_HOLD_AT_1;

I cannot see the difference here...?

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)