[PATCH net 1/2] net/mlx5: Bound the command interface drain so teardown cannot hang

[Danielle Costantino]

mlx5_cmd_allowed_opcode() and mlx5_cmd_change_mod() drain the command
interface before updating a field that every in flight command reads, by
taking every unit of cmd->vars.sem:

	for (i = 0; i < cmd->vars.max_reg_cmds; i++)
		down(&cmd->vars.sem);
	down(&cmd->vars.pages_sem);

Since commit 8e715cd613a1 ("net/mlx5: Set command entry semaphore up once
got index free") a unit is handed back from cmd_ent_put(), under the
refcount that reaches zero:

	if (ent->idx >= 0) {
		cmd_free_index(cmd, ent->idx);
		up(ent->page_queue ? &cmd->vars.pages_sem : &cmd->vars.sem);
	}

A command that timed out never gets there.  mlx5_cmd_comp_handler(forced)
deliberately keeps the entry and its index allocated because firmware may
still complete the command, so the entry keeps a reference, the refcount
never reaches zero, and the unit is never returned.  Before that change the
up() ran unconditionally at the end of the completion loop and a timed out
entry did give its unit back; only the index was withheld.

So a single stalled command slot makes both functions block forever.

That is not a corner case, because destroy_async_eqs() calls both while
tearing the device down:

	mlx5_cmd_allowed_opcode(dev, MLX5_CMD_OP_DESTROY_EQ);
	mlx5_cmd_use_polling(dev);		/* mlx5_cmd_change_mod() */
	cleanup_async_eq(dev, &table->cmd_eq, "cmd");
	mlx5_cmd_allowed_opcode(dev, CMD_ALLOWED_OPCODE_ALL);

and that runs from mlx5_eq_table_destroy() <- mlx5_unload(), so a function
with any stalled command cannot be removed:

  mlx5_cmd_allowed_opcode+0x70/0x188
  destroy_async_eqs+0x168/0x440
  mlx5_eq_table_destroy+0x29c/0x2e0
  mlx5_unload+0xa8/0xe8
  mlx5_uninit_one+0xa0/0x190
  remove_one+0x80/0x100
  pci_device_remove+0x9c/0x1c0
  device_release_driver_internal+0x358/0x5a8
  unbind_store+0x14c/0x188

The task stays in D state indefinitely and holds the devlink instance lock
while it does, so concurrent devlink users pile up behind it.  Reboot does
not recover it quickly either, since the same teardown runs on the way
down.

Reproduced by swallowing firmware completions with a kprobe so that
commands take the real -ETIMEDOUT path, then unbinding the function.  With
20 of the 31 register slots stalled, inspecting the hung device shows
cmd->vars.sem drained to 0 while cmd->vars.bitmask still reports 11 slots
free: mlx5_cmd_allowed_opcode() took the 11 units that were available and
then blocked on the 20 that are never coming back.

Bound the wait by the command timeout, which is the longest a command that
is merely in flight can legitimately take, and update the field without a
full drain if it expires.  Give the two callers a shared helper so the
unwind releases exactly what was acquired.  Failing to drain is worth a
warning but not a hang: the entries still holding units have already timed
out, so they are the least likely to be disturbed by the update, and the
alternative is an unrecoverable teardown.

mlx5_cmd_invoke() already bounds the same semaphore this way, see
commit 485d65e13571 ("net/mlx5: Add a timeout to acquire the command
queue semaphore").

Fixes: 8e715cd613a1 ("net/mlx5: Set command entry semaphore up once got index free")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Danielle Costantino <dcostantino@xxxxxxxx>
---
 drivers/net/ethernet/mellanox/mlx5/core/cmd.c | 77 +++++++++++++++----
 1 file changed, 61 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
index 84583dc5eb1c0..571ed540957b1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
@@ -1656,36 +1656,81 @@ static void create_debugfs_files(struct mlx5_core_dev *dev)
 	debugfs_create_file("run", 0200, dbg->dbg_root, dev, &fops);
 }
 
-void mlx5_cmd_allowed_opcode(struct mlx5_core_dev *dev, u16 opcode)
+/* Drain the command interface so that cmd->allowed_opcode and cmd->mode can be
+ * updated without an in flight command straddling the change.  A command that
+ * timed out keeps its index, and with it its semaphore unit, until firmware
+ * completes it - which may never happen - so bound the wait by the command
+ * timeout instead of blocking forever.  Returns the number of cmd->vars.sem
+ * units taken, and reports separately whether the page queue unit was taken;
+ * both have to be handed back by cmd_sem_up_all().
+ */
+static int cmd_sem_down_all(struct mlx5_core_dev *dev, bool *pages_sem)
 {
+	unsigned long end = jiffies + msecs_to_jiffies(mlx5_tout_ms(dev, CMD));
 	struct mlx5_cmd *cmd = &dev->cmd;
+	long left;
 	int i;
 
-	for (i = 0; i < cmd->vars.max_reg_cmds; i++)
-		down(&cmd->vars.sem);
-	down(&cmd->vars.pages_sem);
+	for (i = 0; i < cmd->vars.max_reg_cmds; i++) {
+		left = end - jiffies;
+		if (left <= 0 || down_timeout(&cmd->vars.sem, left))
+			break;
+	}
 
-	cmd->allowed_opcode = opcode;
+	left = end - jiffies;
+	*pages_sem = left > 0 && !down_timeout(&cmd->vars.pages_sem, left);
 
-	up(&cmd->vars.pages_sem);
-	for (i = 0; i < cmd->vars.max_reg_cmds; i++)
+	if (i < cmd->vars.max_reg_cmds)
+		mlx5_core_warn(dev, "command interface did not drain, %d of %d slots still busy\n",
+			       cmd->vars.max_reg_cmds - i, cmd->vars.max_reg_cmds);
+	if (!*pages_sem)
+		mlx5_core_warn(dev, "command interface did not drain, page queue slot still busy\n");
+
+	return i;
+}
+
+static void cmd_sem_up_all(struct mlx5_core_dev *dev, int nr, bool pages_sem)
+{
+	struct mlx5_cmd *cmd = &dev->cmd;
+
+	if (pages_sem)
+		up(&cmd->vars.pages_sem);
+	while (nr--)
 		up(&cmd->vars.sem);
 }
 
-static void mlx5_cmd_change_mod(struct mlx5_core_dev *dev, int mode)
+void mlx5_cmd_allowed_opcode(struct mlx5_core_dev *dev, u16 opcode)
 {
 	struct mlx5_cmd *cmd = &dev->cmd;
-	int i;
+	bool pages_sem;
+	int nr;
 
-	for (i = 0; i < cmd->vars.max_reg_cmds; i++)
-		down(&cmd->vars.sem);
-	down(&cmd->vars.pages_sem);
+	nr = cmd_sem_down_all(dev, &pages_sem);
 
-	cmd->mode = mode;
+	/* Narrowing the set is only safe once the interface has drained.
+	 * mlx5_cmd_comp_handler() reads !opcode_allowed() as "no real
+	 * firmware completion is expected" and releases the entry, so
+	 * narrowing while a command is still posted would hand its mailboxes
+	 * back to dev->cmd.pool with firmware still able to write them.
+	 * Widening back to CMD_ALLOWED_OPCODE_ALL is always safe.
+	 */
+	if (opcode == CMD_ALLOWED_OPCODE_ALL ||
+	    (nr == cmd->vars.max_reg_cmds && pages_sem))
+		cmd->allowed_opcode = opcode;
+	else
+		mlx5_core_warn(dev, "leaving command opcodes unrestricted, interface did not drain\n");
 
-	up(&cmd->vars.pages_sem);
-	for (i = 0; i < cmd->vars.max_reg_cmds; i++)
-		up(&cmd->vars.sem);
+	cmd_sem_up_all(dev, nr, pages_sem);
+}
+
+static void mlx5_cmd_change_mod(struct mlx5_core_dev *dev, int mode)
+{
+	bool pages_sem;
+	int nr;
+
+	nr = cmd_sem_down_all(dev, &pages_sem);
+	dev->cmd.mode = mode;
+	cmd_sem_up_all(dev, nr, pages_sem);
 }
 
 static int cmd_comp_notifier(struct notifier_block *nb,