On 2022-07-17 02:47, Vladimir Oltean wrote:

quoted

@@ -1721,11 +1735,11 @@ static int mv88e6xxx_vtu_get(struct 

mv88e6xxx_chip *chip, u16 vid,
 	return err;
 }

 int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port,
 			    bool locked)
 {

@@ -1257,13 +1270,18 @@ int mv88e6xxx_port_set_lock(struct 

mv88e6xxx_chip *chip, int port,
 	if (err)
 		return err;

-	err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR, 
&reg);
-	if (err)
-		return err;
+	if (!locked) {
+		err = mv88e6xxx_atu_locked_entry_flush(chip->ds, port);

Did you re-run the selftest with v4? Because this deadlocks due to the
double reg_lock illustrated below:

I did some selftest, but I didn't experience the problem of the double 
chip lock as it only happens on unlock which is only called where the 
MAB test ends.

+ vrf_name=vlan1
+ ip vrf exec vlan1 ping 192.0.2.2 -c 10 -i 0.1 -w 5
+ check_err 1 'Ping did not work after locking port and adding FDB 
entry'
+ local err=1
+ local 'msg=Ping did not work after locking port and adding FDB entry'
+ [[ 0 -eq 0 ]]
+ [[ 1 -ne 0 ]]
+ RET=1
+ retmsg='Ping did not work after locking port and adding FDB entry'
+ bridge link set dev lan2 locked off
[  733.273994]
[  733.275515] ============================================
[  733.280823] WARNING: possible recursive locking detected
[  733.286133] 5.19.0-rc6-07010-ga9b9500ffaac-dirty #3293 Not tainted
[  733.292311] --------------------------------------------
[  733.297613] kworker/0:0/601 is trying to acquire lock:
[  733.302751] ffff00000213c110 (&chip->reg_lock){+.+.}-{4:4}, at:
mv88e6xxx_atu_locked_entry_purge+0x70/0x1a4
[  733.312524]
[  733.312524] but task is already holding lock:
[  733.318351] ffff00000213c110 (&chip->reg_lock){+.+.}-{4:4}, at:
mv88e6xxx_port_bridge_flags+0x48/0x234
[  733.327674]
[  733.327674] other info that might help us debug this:
[  733.334198]  Possible unsafe locking scenario:
[  733.334198]
[  733.340109]        CPU0
[  733.342549]        ----
[  733.344990]   lock(&chip->reg_lock);
[  733.348567]   lock(&chip->reg_lock);
[  733.352149]
[  733.352149]  *** DEADLOCK ***
[  733.352149]
[  733.358063]  May be due to missing lock nesting notation
[  733.358063]
[  733.364846] 6 locks held by kworker/0:0/601:
[  733.369115]  #0: ffff00000000b748
((wq_completion)events){+.+.}-{0:0}, at: process_one_work+0x1f4/0x6c4
[  733.378541]  #1: ffff80000c43bdc8
(deferred_process_work){+.+.}-{0:0}, at: process_one_work+0x1f4/0x6c4
[  733.387966]  #2: ffff80000b020cb0 (rtnl_mutex){+.+.}-{4:4}, at:
rtnl_lock+0x1c/0x30
[  733.395660]  #3: ffff80000b073370
((switchdev_blocking_notif_chain).rwsem){++++}-{4:4}, at:
blocking_notifier_call_chain+0x34/0xa0
[  733.407432]  #4: ffff00000213c110 (&chip->reg_lock){+.+.}-{4:4},
at: mv88e6xxx_port_bridge_flags+0x48/0x234
[  733.417202]  #5: ffff00000213e0f0 (&p->ale_list_lock){+.+.}-{4:4},
at: mv88e6xxx_atu_locked_entry_flush+0x4c/0xc0
[  733.427495]
[  733.427495] stack backtrace:
[  733.431858] CPU: 0 PID: 601 Comm: kworker/0:0 Not tainted
5.19.0-rc6-07010-ga9b9500ffaac-dirty #3293
[  733.440992] Hardware name: CZ.NIC Turris Mox Board (DT)
[  733.446219] Workqueue: events switchdev_deferred_process_work
[  733.451982] Call trace:
[  733.454424]  dump_backtrace.part.0+0xcc/0xe0
[  733.458702]  show_stack+0x18/0x6c
[  733.462028]  dump_stack_lvl+0x8c/0xb8
[  733.465703]  dump_stack+0x18/0x34
[  733.469029]  __lock_acquire+0x1074/0x1fd0
[  733.473052]  lock_acquire.part.0+0xe4/0x220
[  733.477244]  lock_acquire+0x68/0x8c
[  733.480744]  __mutex_lock+0x9c/0x460
[  733.484332]  mutex_lock_nested+0x40/0x50
[  733.488268]  mv88e6xxx_atu_locked_entry_purge+0x70/0x1a4
[  733.493592]  mv88e6xxx_atu_locked_entry_flush+0x7c/0xc0
[  733.498827]  mv88e6xxx_port_set_lock+0xfc/0x10c
[  733.503374]  mv88e6xxx_port_bridge_flags+0x200/0x234
[  733.508351]  dsa_port_bridge_flags+0x44/0xe0
[  733.512635]  dsa_slave_port_attr_set+0x1ec/0x230
[  733.517268]  __switchdev_handle_port_attr_set+0x58/0x100
[  733.522594]  switchdev_handle_port_attr_set+0x10/0x24
[  733.527659]  dsa_slave_switchdev_blocking_event+0x8c/0xd4
[  733.533074]  blocking_notifier_call_chain+0x6c/0xa0
[  733.537968]  switchdev_port_attr_notify.constprop.0+0x4c/0xb0
[  733.543729]  switchdev_port_attr_set_deferred+0x24/0x80
[  733.548967]  switchdev_deferred_process+0x90/0x164
[  733.553774]  switchdev_deferred_process_work+0x14/0x2c
[  733.558926]  process_one_work+0x28c/0x6c4
[  733.562949]  worker_thread+0x74/0x450
[  733.566623]  kthread+0x118/0x11c
[  733.569860]  ret_from_fork+0x10/0x20
++ mac_get lan1
++ local if_name=lan1
++ jq -r '.[]["address"]'
++ ip -j link show dev lan1

I've tentatively fixed this in my tree by taking the register lock more
localized to the sub-functions of mv88e6xxx_port_bridge_flags(), and 
not
taking it at caller side for mv88e6xxx_port_set_lock(), but rather
letting the callee take it:

Yes, the double chip lock came from calling 
mv88e6xxx_atu_locked_entry_flush() in mv88e6xxx_port_set_lock() that is 
called from mv88e6xxx_port_bridge_flags() whic has the lock taken. I 
have fixed this now, but when I unlock a port with locked entries, I 
cannot have notify the bridge to clear the same locked entries by having 
mv88e6xxx_atu_locked_entry_flush() calling with the notify on, thus 
taking the rtnl lock, as this also results in a double lock.
To remove the locked entries in the bridge FDB it is then necessary as I 
see it, to take the link down or have a userspace daemon clear them. I 
hope that is okay if documented?