Re: [PATCH 09/11] vfio: selftests: Add mlx5 driver - HW init and command interface

[Manuel Ebner <manuelebner@mailbox.org>]

Hi Jason,

i've gone through your patch, just some minor and some optional things.

On Thu, 2026-04-30 at 21:08 -0300, Jason Gunthorpe wrote:

> [...]
> diff --git a/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c
> b/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c
> new file mode 100644
> index 00000000000000..0ab941bad7a66c
> --- /dev/null
> +++ b/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c
> @@ -0,0 +1,1406 @@
> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> +/*
> + * mlx5 VFIO selftest driver
> + *
> + * Programs mlx5 ConnectX VFs and PFs through the bare-metal command
> interface
> + * and RDMA Write self-loopback to perform DMA.  Implements

Write -> write
else it feels like there is a new sentence 

> + * (probe/init/remove) and plugs into the VFIO selftest framework.
> + */
> +#include <stdint.h>
> +#include <stdbool.h>
> +#include <string.h>
> +#include <time.h>
> +#include <sched.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +
> +#include <linux/errno.h>
> +#include <linux/io.h>
> +#include <linux/log2.h>
> +#include <linux/pci_regs.h>

sort álphabetically: 

+#include <sched.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdlib.h>~
+#include <string.h>
+#include <time.h>
+#include <unistd.h>

+#include <linux/errno.h>
+#include <linux/io.h>
+#include <linux/log2.h>
+#include <linux/pci_regs.h>

> +
> +#include <libvfio.h>
> +
> +#include "mlx5_hw.h"

why are these two lines grouped individually?


> +/* Forward declaration — cmd_exec polls events during command wait */
> +static void mlx5st_process_events(struct mlx5st_device *dev);
> +
> +static const char *mlx5st_cmd_name(u16 opcode)
> +{
> +	switch (opcode) {
> +	case MLX5_CMD_OP_QUERY_HCA_CAP: return "QUERY_HCA_CAP";
> +	case MLX5_CMD_OP_INIT_HCA: return "INIT_HCA";
> +	case MLX5_CMD_OP_TEARDOWN_HCA: return "TEARDOWN_HCA";
> +	case MLX5_CMD_OP_ENABLE_HCA: return "ENABLE_HCA";
> +	case MLX5_CMD_OP_DISABLE_HCA: return "DISABLE_HCA";
> +	[...]
> +	}
> +}

Maybe line them up like:
Depends on your taste.

+	switch (opcode) {
+	case MLX5_CMD_OP_QUERY_HCA_CAP:	return "QUERY_HCA_CAP";
+	case MLX5_CMD_OP_INIT_HCA: 	return "INIT_HCA";
+	case MLX5_CMD_OP_TEARDOWN_HCA: 	return "TEARDOWN_HCA";
+	case MLX5_CMD_OP_ENABLE_HCA: 	return "ENABLE_HCA";
+	case MLX5_CMD_OP_DISABLE_HCA: 	return "DISABLE_HCA";
+	case MLX5_CMD_OP_QUERY_PAGES: 	return "QUERY_PAGES";
+ 	[...]

> +
> +	/*
> +	 * Compute signatures: mailbox blocks first, then cmd_queue_entry
> last.
> +	 * The sig must cover the final state including ownership=0x1, but
> +	 * we must not set ownership until after the sig is in place —
> +	 * XOR in the 0x1 without storing it to memory.
> +	 */

- " last"

+	/*
+	 * Compute signatures: mailbox blocks first, then cmd_queue_entry.

Thanks
 Manuel