Rely on firmware to get special mkeys

From: Or Har-Toov <[email protected]>

Using query_sepcial_contexts in order to get the correct value of terminate_scatter_list_mkey, as FW will change it in some configurations. This is done one time when the device is loading and the value is being saved in the device context.

Signed-off-by Or Har-Toov <[email protected]>

Reviewed-by Michael Guralnik <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---

.../net/ethernet/mellanox/mlx5/core/en_main.c |  2 +-
.../net/ethernet/mellanox/mlx5/core/main.c    | 27 +++++++++++++++++++
include/linux/mlx5/driver.h                   |  1 +
3 files changed, 29 insertions(+), 1 deletion(-)

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +1 -1

index c76f15505a76..33d7a7095988 100644
@@ -826,7 +826,7 @@ static int mlx5e_alloc_rq(struct mlx5e_params *params,
 			if (rq->wqe.info.num_frags < (1 << rq->wqe.info.log_num_frags)) {
 				wqe->data[f].byte_count = 0;
 				wqe->data[f].lkey =
-					MLX5_TERMINATE_SCATTER_LIST_LKEY;
+					mdev->terminate_scatter_list_mkey;
 				wqe->data[f].addr = 0;
 			}
 		}

drivers/net/ethernet/mellanox/mlx5/core/main.c +3 -0

index 7f5db13e3550..d39d758744a0 100644
@@ -1221,6 +1221,28 @@ static int mlx5_function_teardown(struct mlx5_core_dev *dev, bool boot)
 	return 0;
 }
 
+static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
+{
+	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};

drivers/net/ethernet/mellanox/mlx5/core/main.c +0 -0

 	err = mlx5_irq_table_create(dev);
 	if (err) {
 		mlx5_core_err(dev, "Failed to alloc IRQs\n");

include/linux/mlx5/driver.h +1 -0

index d476255c9a3f..5f2c4038d638 100644
@@ -801,6 +801,7 @@ struct mlx5_core_dev {
 	struct mlx5_rsc_dump    *rsc_dump;
 	u32                      vsc_addr;
 	struct mlx5_hv_vhca	*hv_vhca;
+	__be32			terminate_scatter_list_mkey;
 };
 
 struct mlx5_db {

Signature · 1 line

2.38.1

Quoted 40 lines · Leon Romanovsky

From: Or Har-Toov <[email protected]>

Using query_sepcial_contexts in order to get the correct value of terminate_scatter_list_mkey, as FW will change it in some configurations. This is done one time when the device is loading and the value is being saved in the device context.

Signed-off-by Or Har-Toov <[email protected]>

Reviewed-by Michael Guralnik <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---

.../net/ethernet/mellanox/mlx5/core/en_main.c |  2 +-
.../net/ethernet/mellanox/mlx5/core/main.c    | 27 +++++++++++++++++++
include/linux/mlx5/driver.h                   |  1 +
3 files changed, 29 insertions(+), 1 deletion(-)

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +1 -1

index c76f15505a76..33d7a7095988 100644
@@ -826,7 +826,7 @@ static int mlx5e_alloc_rq(struct mlx5e_params *params,
 			if (rq->wqe.info.num_frags < (1 << rq->wqe.info.log_num_frags)) {
 				wqe->data[f].byte_count = 0;
 				wqe->data[f].lkey =
-					MLX5_TERMINATE_SCATTER_LIST_LKEY;
+					mdev->terminate_scatter_list_mkey;
 				wqe->data[f].addr = 0;
 			}
 		}

drivers/net/ethernet/mellanox/mlx5/core/main.c +3 -0

index 7f5db13e3550..d39d758744a0 100644
@@ -1221,6 +1221,28 @@ static int mlx5_function_teardown(struct mlx5_core_dev *dev, bool boot)
 	return 0;
 }
 
+static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
+{
+	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};

drivers/net/ethernet/mellanox/mlx5/core/main.c static int mlx5_function_teardown(struct mlx5_core_dev *dev, bool boot) · line 1236

Quoted 10 lines

drivers/net/ethernet/mellanox/mlx5/core/main.c +10 -0

+	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
+	int err;
+
+	MLX5_SET(query_special_contexts_in, in, opcode,
+		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
+	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
+	if (err)
+		return err;
+
+	if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {

Why did you execute the command unconditionally if the output is only read conditionally?

early exit before executing the command, older FW might fail and driver will be unusable ..

drivers/net/ethernet/mellanox/mlx5/core/main.c static int mlx5_function_teardown(struct mlx5_core_dev *dev, bool boot) · line 1242

Quoted 7 lines

drivers/net/ethernet/mellanox/mlx5/core/main.c +6 -0

+		dev->terminate_scatter_list_mkey =
+			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
+					     terminate_scatter_list_mkey));
+		return 0;
+	}
+	dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;

Another concern, what happens with old driver that is using the hardcoded value with newer fw ? will it fail ? will it be accepted ?

The commit message doesn't explain what's going on here very well, Let's discuss internally before you submit V2.

Quoted 8 lines

drivers/net/ethernet/mellanox/mlx5/core/main.c +3 -0

+	return 0;
+}
+
 static int mlx5_load(struct mlx5_core_dev *dev)
 {
 	int err;

drivers/net/ethernet/mellanox/mlx5/core/main.c static int mlx5_load(struct mlx5_core_dev *dev) · line 1262

Quoted 6 lines

drivers/net/ethernet/mellanox/mlx5/core/main.c +3 -0

@@ -1235,6 +1257,11 @@ static int mlx5_load(struct mlx5_core_dev *dev)
 	mlx5_events_start(dev);
 	mlx5_pagealloc_start(dev);
 
+	err = mlx5_get_terminate_scatter_list_mkey(dev);
+	if (err) {
+		mlx5_core_err(dev, "Failed to query special contexts\n");

print the err;

drivers/net/ethernet/mellanox/mlx5/core/main.c static int mlx5_load(struct mlx5_core_dev *dev) · line 1264

Quoted 2 lines

drivers/net/ethernet/mellanox/mlx5/core/main.c +2 -0

+		goto err_irq_table;
+	}

you are querying FW too soon, no issue here but better if you move it after mlx5_eq_table_create() to use the command EQ rather than the primitive cmd interface.

Quoted 19 lines

drivers/net/ethernet/mellanox/mlx5/core/main.c +0 -0

 	err = mlx5_irq_table_create(dev);
 	if (err) {
 		mlx5_core_err(dev, "Failed to alloc IRQs\n");

include/linux/mlx5/driver.h +1 -0

index d476255c9a3f..5f2c4038d638 100644
@@ -801,6 +801,7 @@ struct mlx5_core_dev {
 	struct mlx5_rsc_dump    *rsc_dump;
 	u32                      vsc_addr;
 	struct mlx5_hv_vhca	*hv_vhca;
+	__be32			terminate_scatter_list_mkey;
 };
 
 struct mlx5_db {

Signature · 1 line

2.38.1

Quoted 55 lines · Saeed Mahameed

Quoted 40 lines · Leon Romanovsky

From: Or Har-Toov <[email protected]>

Using query_sepcial_contexts in order to get the correct value of terminate_scatter_list_mkey, as FW will change it in some configurations. This is done one time when the device is loading and the value is being saved in the device context.

Signed-off-by Or Har-Toov <[email protected]>

Reviewed-by Michael Guralnik <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---

.../net/ethernet/mellanox/mlx5/core/en_main.c |  2 +-
.../net/ethernet/mellanox/mlx5/core/main.c    | 27 +++++++++++++++++++
include/linux/mlx5/driver.h                   |  1 +
3 files changed, 29 insertions(+), 1 deletion(-)

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +1 -1

index c76f15505a76..33d7a7095988 100644
@@ -826,7 +826,7 @@ static int mlx5e_alloc_rq(struct mlx5e_params *params,
 			if (rq->wqe.info.num_frags < (1 << rq->wqe.info.log_num_frags)) {
 				wqe->data[f].byte_count = 0;
 				wqe->data[f].lkey =
-					MLX5_TERMINATE_SCATTER_LIST_LKEY;
+					mdev->terminate_scatter_list_mkey;
 				wqe->data[f].addr = 0;
 			}
 		}

drivers/net/ethernet/mellanox/mlx5/core/main.c +3 -0

index 7f5db13e3550..d39d758744a0 100644
@@ -1221,6 +1221,28 @@ static int mlx5_function_teardown(struct mlx5_core_dev *dev, bool boot)
 	return 0;
 }
 
+static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
+{
+	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};

drivers/net/ethernet/mellanox/mlx5/core/main.c static int mlx5_function_teardown(struct mlx5_core_dev *dev, bool boot) · line 1236

Quoted 10 lines

drivers/net/ethernet/mellanox/mlx5/core/main.c +10 -0

+	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
+	int err;
+
+	MLX5_SET(query_special_contexts_in, in, opcode,
+		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
+	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
+	if (err)
+		return err;
+
+	if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {

Why did you execute the command unconditionally if the output is only read conditionally?

Quoted 2 lines

early exit before executing the command, older FW might fail and driver will be unusable ..

According to the documentation, this functionality was forever, but you are presenting valid concern.

Quoted 10 lines

drivers/net/ethernet/mellanox/mlx5/core/main.c static int mlx5_function_teardown(struct mlx5_core_dev *dev, bool boot) · line 1242

Quoted 7 lines

drivers/net/ethernet/mellanox/mlx5/core/main.c +6 -0

+		dev->terminate_scatter_list_mkey =
+			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
+					     terminate_scatter_list_mkey));
+		return 0;
+	}
+	dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;

Another concern, what happens with old driver that is using the hardcoded value with newer fw ? will it fail ? will it be accepted ?

It will be accepted and everything will work without glitches. FW keeps backward compatibility and use MLX5_TERMINATE_SCATTER_LIST_LKEY internally as a default.

drivers/infiniband/hw/mlx5/cmd.c +9 -12

-int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
-{
-	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
-	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
-	int err;
+	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
+	if (err)
+		return err;
 
-	MLX5_SET(query_special_contexts_in, in, opcode,
-		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
-	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
-	if (!err)
-		*null_mkey = MLX5_GET(query_special_contexts_out, out,
-				      null_mkey);
-	return err;
+	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
+		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
+						     out, dump_fill_mkey);
+
+	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
+		dev->mkeys.null_mkey = cpu_to_be32(

Quoted 27 lines · Leon Romanovsky

drivers/infiniband/hw/mlx5/cmd.c +14 -12

-int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
-{
-	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
-	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
-	int err;
+	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
+	if (err)
+		return err;
 
-	MLX5_SET(query_special_contexts_in, in, opcode,
-		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
-	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
-	if (!err)
-		*null_mkey = MLX5_GET(query_special_contexts_out, out,
-				      null_mkey);
-	return err;
+	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
+		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
+						     out, dump_fill_mkey);
+
+	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
+		dev->mkeys.null_mkey = cpu_to_be32(
+			MLX5_GET(query_special_contexts_out, out, null_mkey));
+
+	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
+		dev->mkeys.terminate_scatter_list_mkey =
+			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,

drivers/infiniband/hw/mlx5/cmd.c int mlx5_cmd_dump_fill_mkey(struct mlx5_core_dev *dev, u32 *mkey) · line 35

Quoted 5 lines

drivers/infiniband/hw/mlx5/cmd.c +5 -0

+					     terminate_scatter_list_mkey));
+		return 0;
+	}
+	dev->mkeys.terminate_scatter_list_mkey =
+		MLX5_TERMINATE_SCATTER_LIST_LKEY;

This is already stored in the core dev, why are you recalculating it here?

Jason

Quoted 28 lines · Jason Gunthorpe

Quoted 27 lines · Leon Romanovsky

drivers/infiniband/hw/mlx5/cmd.c +14 -12

-int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
-{
-	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
-	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
-	int err;
+	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
+	if (err)
+		return err;
 
-	MLX5_SET(query_special_contexts_in, in, opcode,
-		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
-	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
-	if (!err)
-		*null_mkey = MLX5_GET(query_special_contexts_out, out,
-				      null_mkey);
-	return err;
+	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
+		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
+						     out, dump_fill_mkey);
+
+	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
+		dev->mkeys.null_mkey = cpu_to_be32(
+			MLX5_GET(query_special_contexts_out, out, null_mkey));
+
+	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
+		dev->mkeys.terminate_scatter_list_mkey =
+			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,

Quoted 8 lines

drivers/infiniband/hw/mlx5/cmd.c int mlx5_cmd_dump_fill_mkey(struct mlx5_core_dev *dev, u32 *mkey) · line 35

Quoted 5 lines

drivers/infiniband/hw/mlx5/cmd.c +5 -0

+					     terminate_scatter_list_mkey));
+		return 0;
+	}
+	dev->mkeys.terminate_scatter_list_mkey =
+		MLX5_TERMINATE_SCATTER_LIST_LKEY;

This is already stored in the core dev, why are you recalculating it here?

It is not recalculating but setting default value. In core dev, we will have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey) is true.

Quoted 38 lines · Leon Romanovsky

Quoted 28 lines · Jason Gunthorpe

Quoted 27 lines · Leon Romanovsky

drivers/infiniband/hw/mlx5/cmd.c +14 -12

-int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
-{
-	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
-	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
-	int err;
+	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
+	if (err)
+		return err;
 
-	MLX5_SET(query_special_contexts_in, in, opcode,
-		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
-	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
-	if (!err)
-		*null_mkey = MLX5_GET(query_special_contexts_out, out,
-				      null_mkey);
-	return err;
+	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
+		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
+						     out, dump_fill_mkey);
+
+	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
+		dev->mkeys.null_mkey = cpu_to_be32(
+			MLX5_GET(query_special_contexts_out, out, null_mkey));
+
+	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
+		dev->mkeys.terminate_scatter_list_mkey =
+			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,

Quoted 8 lines

drivers/infiniband/hw/mlx5/cmd.c int mlx5_cmd_dump_fill_mkey(struct mlx5_core_dev *dev, u32 *mkey) · line 35

Quoted 5 lines

drivers/infiniband/hw/mlx5/cmd.c +5 -0

+					     terminate_scatter_list_mkey));
+		return 0;
+	}
+	dev->mkeys.terminate_scatter_list_mkey =
+		MLX5_TERMINATE_SCATTER_LIST_LKEY;

This is already stored in the core dev, why are you recalculating it here?

Quoted 3 lines

It is not recalculating but setting default value. In core dev, we will have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey) is true.

No, it has the identical code:

Jason

Quoted 45 lines · Jason Gunthorpe

Quoted 38 lines · Leon Romanovsky

Quoted 28 lines · Jason Gunthorpe

Quoted 27 lines · Leon Romanovsky

drivers/infiniband/hw/mlx5/cmd.c +14 -12

-int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
-{
-	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
-	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
-	int err;
+	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
+	if (err)
+		return err;
 
-	MLX5_SET(query_special_contexts_in, in, opcode,
-		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
-	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
-	if (!err)
-		*null_mkey = MLX5_GET(query_special_contexts_out, out,
-				      null_mkey);
-	return err;
+	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
+		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
+						     out, dump_fill_mkey);
+
+	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
+		dev->mkeys.null_mkey = cpu_to_be32(
+			MLX5_GET(query_special_contexts_out, out, null_mkey));
+
+	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
+		dev->mkeys.terminate_scatter_list_mkey =
+			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,

Quoted 8 lines

drivers/infiniband/hw/mlx5/cmd.c int mlx5_cmd_dump_fill_mkey(struct mlx5_core_dev *dev, u32 *mkey) · line 35

Quoted 5 lines

drivers/infiniband/hw/mlx5/cmd.c +5 -0

+					     terminate_scatter_list_mkey));
+		return 0;
+	}
+	dev->mkeys.terminate_scatter_list_mkey =
+		MLX5_TERMINATE_SCATTER_LIST_LKEY;

This is already stored in the core dev, why are you recalculating it here?

Quoted 3 lines

It is not recalculating but setting default value. In core dev, we will have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey) is true.

No, it has the identical code:

Review

Quoted 9 lines

(quoted diff) +9 -0

+static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
+{
+       if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
+               dev->terminate_scatter_list_mkey =
+                       cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
+                                            terminate_scatter_list_mkey));
+               return 0;
+       }
+       dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;

Ahh, you are talking about that.
terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS, which is needed to get other mkeys. So instead of doing special logic
for the terminate_scatter_list_mkey, we decided to use same pattern as
for other mkeys, which don't belong to core.

Quoted 46 lines · Leon Romanovsky

Quoted 45 lines · Jason Gunthorpe

Quoted 38 lines · Leon Romanovsky

Quoted 28 lines · Jason Gunthorpe

Quoted 27 lines · Leon Romanovsky

drivers/infiniband/hw/mlx5/cmd.c +14 -12

-int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
-{
-	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
-	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
-	int err;
+	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
+	if (err)
+		return err;
 
-	MLX5_SET(query_special_contexts_in, in, opcode,
-		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
-	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
-	if (!err)
-		*null_mkey = MLX5_GET(query_special_contexts_out, out,
-				      null_mkey);
-	return err;
+	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
+		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
+						     out, dump_fill_mkey);
+
+	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
+		dev->mkeys.null_mkey = cpu_to_be32(
+			MLX5_GET(query_special_contexts_out, out, null_mkey));
+
+	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
+		dev->mkeys.terminate_scatter_list_mkey =
+			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,

Quoted 8 lines

drivers/infiniband/hw/mlx5/cmd.c int mlx5_cmd_dump_fill_mkey(struct mlx5_core_dev *dev, u32 *mkey) · line 35

Quoted 5 lines

drivers/infiniband/hw/mlx5/cmd.c +5 -0

+					     terminate_scatter_list_mkey));
+		return 0;
+	}
+	dev->mkeys.terminate_scatter_list_mkey =
+		MLX5_TERMINATE_SCATTER_LIST_LKEY;

This is already stored in the core dev, why are you recalculating it here?

Quoted 3 lines

It is not recalculating but setting default value. In core dev, we will have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey) is true.

No, it has the identical code:

Quoted 15 lines

Review

Quoted 9 lines

(quoted diff) +9 -0

+static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
+{
+       if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
+               dev->terminate_scatter_list_mkey =
+                       cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
+                                            terminate_scatter_list_mkey));
+               return 0;
+       }
+       dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;

Ahh, you are talking about that.
terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS, which is needed to get other mkeys. So instead of doing special logic
for the terminate_scatter_list_mkey, we decided to use same pattern as
for other mkeys, which don't belong to core.

Regardless, don't duplicate the code and maybe don't even duplicate the storage of the terminate_scatter_list_mkey

Jason

Quoted 63 lines · Jason Gunthorpe

Quoted 61 lines · Leon Romanovsky

Quoted 45 lines · Jason Gunthorpe

Quoted 38 lines · Leon Romanovsky

Quoted 28 lines · Jason Gunthorpe

Quoted 27 lines · Leon Romanovsky

(quoted diff) +14 -12

-int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
-{
-	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
-	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
-	int err;
+	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
+	if (err)
+		return err;
 
-	MLX5_SET(query_special_contexts_in, in, opcode,
-		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
-	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
-	if (!err)
-		*null_mkey = MLX5_GET(query_special_contexts_out, out,
-				      null_mkey);
-	return err;
+	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
+		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
+						     out, dump_fill_mkey);
+
+	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
+		dev->mkeys.null_mkey = cpu_to_be32(
+			MLX5_GET(query_special_contexts_out, out, null_mkey));
+
+	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
+		dev->mkeys.terminate_scatter_list_mkey =
+			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+					     terminate_scatter_list_mkey));
+		return 0;
+	}
+	dev->mkeys.terminate_scatter_list_mkey =
+		MLX5_TERMINATE_SCATTER_LIST_LKEY;

This is already stored in the core dev, why are you recalculating it here?

Quoted 3 lines

It is not recalculating but setting default value. In core dev, we will have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey) is true.

No, it has the identical code:

Review

Quoted 9 lines

(quoted diff) +9 -0

+static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
+{
+       if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
+               dev->terminate_scatter_list_mkey =
+                       cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
+                                            terminate_scatter_list_mkey));
+               return 0;
+       }
+       dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;

Ahh, you are talking about that.
terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS, which is needed to get other mkeys. So instead of doing special logic
for the terminate_scatter_list_mkey, we decided to use same pattern as
for other mkeys, which don't belong to core.

Quoted 2 lines

Regardless, don't duplicate the code and maybe don't even duplicate the storage of the terminate_scatter_list_mkey

ok, will update and resend.

Quoted 64 lines · Leon Romanovsky

Quoted 63 lines · Jason Gunthorpe

Quoted 61 lines · Leon Romanovsky

Quoted 45 lines · Jason Gunthorpe

Quoted 38 lines · Leon Romanovsky

Quoted 28 lines · Jason Gunthorpe

Quoted 27 lines · Leon Romanovsky

(quoted diff) +14 -12

-int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
-{
-	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
-	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
-	int err;
+	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
+	if (err)
+		return err;
 
-	MLX5_SET(query_special_contexts_in, in, opcode,
-		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
-	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
-	if (!err)
-		*null_mkey = MLX5_GET(query_special_contexts_out, out,
-				      null_mkey);
-	return err;
+	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
+		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
+						     out, dump_fill_mkey);
+
+	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
+		dev->mkeys.null_mkey = cpu_to_be32(
+			MLX5_GET(query_special_contexts_out, out, null_mkey));
+
+	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
+		dev->mkeys.terminate_scatter_list_mkey =
+			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+					     terminate_scatter_list_mkey));
+		return 0;
+	}
+	dev->mkeys.terminate_scatter_list_mkey =
+		MLX5_TERMINATE_SCATTER_LIST_LKEY;

This is already stored in the core dev, why are you recalculating it here?

Quoted 3 lines

It is not recalculating but setting default value. In core dev, we will have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey) is true.

No, it has the identical code:

Review

Quoted 9 lines

(quoted diff) +9 -0

+static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
+{
+       if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
+               dev->terminate_scatter_list_mkey =
+                       cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
+                                            terminate_scatter_list_mkey));
+               return 0;
+       }
+       dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;

Ahh, you are talking about that.
terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS, which is needed to get other mkeys. So instead of doing special logic
for the terminate_scatter_list_mkey, we decided to use same pattern as
for other mkeys, which don't belong to core.

Quoted 5 lines

Quoted 2 lines

Regardless, don't duplicate the code and maybe don't even duplicate the storage of the terminate_scatter_list_mkey

ok, will update and resend.

Please provide a helper mlx5_get_xyz function, avoid assuming mlx5_core will store it in dev->xyz.

Quoted 65 lines · Saeed Mahameed

Quoted 64 lines · Leon Romanovsky

Quoted 63 lines · Jason Gunthorpe

Quoted 61 lines · Leon Romanovsky

Quoted 45 lines · Jason Gunthorpe

Quoted 38 lines · Leon Romanovsky

Quoted 28 lines · Jason Gunthorpe

Quoted 27 lines · Leon Romanovsky

(quoted diff) +14 -12

-int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
-{
-	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
-	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
-	int err;
+	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
+	if (err)
+		return err;
 
-	MLX5_SET(query_special_contexts_in, in, opcode,
-		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
-	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
-	if (!err)
-		*null_mkey = MLX5_GET(query_special_contexts_out, out,
-				      null_mkey);
-	return err;
+	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
+		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
+						     out, dump_fill_mkey);
+
+	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
+		dev->mkeys.null_mkey = cpu_to_be32(
+			MLX5_GET(query_special_contexts_out, out, null_mkey));
+
+	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
+		dev->mkeys.terminate_scatter_list_mkey =
+			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+					     terminate_scatter_list_mkey));
+		return 0;
+	}
+	dev->mkeys.terminate_scatter_list_mkey =
+		MLX5_TERMINATE_SCATTER_LIST_LKEY;

This is already stored in the core dev, why are you recalculating it here?

Quoted 3 lines

It is not recalculating but setting default value. In core dev, we will have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey) is true.

No, it has the identical code:

Review

Quoted 9 lines

(quoted diff) +9 -0

+static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
+{
+       if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
+               dev->terminate_scatter_list_mkey =
+                       cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
+                                            terminate_scatter_list_mkey));
+               return 0;
+       }
+       dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;

Ahh, you are talking about that.
terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS, which is needed to get other mkeys. So instead of doing special logic
for the terminate_scatter_list_mkey, we decided to use same pattern as
for other mkeys, which don't belong to core.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Regardless, don't duplicate the code and maybe don't even duplicate the storage of the terminate_scatter_list_mkey

ok, will update and resend.

Please provide a helper mlx5_get_xyz function, avoid assuming mlx5_core will store it in dev->xyz.

This helper was used in this version of patch and it is:
MLX5_GET(query_special_contexts_out, out, terminate_scatter_list_mkey)) Which was dropped in favor of Jason's request to rely on mlx5_core_dev as this value already known:
https://lore.kernel.org/all/[email protected]

I don't like the idea to add obfuscation function just for the sake of obfuscation. If you don't want access to mlx5_core_dev, please argue with Jason to accept first variant.

Quoted 81 lines · Leon Romanovsky

Quoted 65 lines · Saeed Mahameed

Quoted 64 lines · Leon Romanovsky

Quoted 63 lines · Jason Gunthorpe

Quoted 61 lines · Leon Romanovsky

Quoted 45 lines · Jason Gunthorpe

Quoted 38 lines · Leon Romanovsky

Quoted 28 lines · Jason Gunthorpe

Quoted 27 lines · Leon Romanovsky

(quoted diff) +14 -12

-int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
-{
-	u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
-	u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
-	int err;
+	err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
+	if (err)
+		return err;
 
-	MLX5_SET(query_special_contexts_in, in, opcode,
-		 MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
-	err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
-	if (!err)
-		*null_mkey = MLX5_GET(query_special_contexts_out, out,
-				      null_mkey);
-	return err;
+	if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
+		dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
+						     out, dump_fill_mkey);
+
+	if (MLX5_CAP_GEN(dev->mdev, null_mkey))
+		dev->mkeys.null_mkey = cpu_to_be32(
+			MLX5_GET(query_special_contexts_out, out, null_mkey));
+
+	if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
+		dev->mkeys.terminate_scatter_list_mkey =
+			cpu_to_be32(MLX5_GET(query_special_contexts_out, out,

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+					     terminate_scatter_list_mkey));
+		return 0;
+	}
+	dev->mkeys.terminate_scatter_list_mkey =
+		MLX5_TERMINATE_SCATTER_LIST_LKEY;

This is already stored in the core dev, why are you recalculating it here?

Quoted 3 lines

It is not recalculating but setting default value. In core dev, we will have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey) is true.

No, it has the identical code:

Review

Quoted 9 lines

(quoted diff) +9 -0

+static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
+{
+       if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
+               dev->terminate_scatter_list_mkey =
+                       cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
+                                            terminate_scatter_list_mkey));
+               return 0;
+       }
+       dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;

Ahh, you are talking about that.
terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS, which is needed to get other mkeys. So instead of doing special logic
for the terminate_scatter_list_mkey, we decided to use same pattern as
for other mkeys, which don't belong to core.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Regardless, don't duplicate the code and maybe don't even duplicate the storage of the terminate_scatter_list_mkey

ok, will update and resend.

Please provide a helper mlx5_get_xyz function, avoid assuming mlx5_core will store it in dev->xyz.

This helper was used in this version of patch and it is:
MLX5_GET(query_special_contexts_out, out, terminate_scatter_list_mkey)) Which was dropped in favor of Jason's request to rely on mlx5_core_dev as this value already known:
https://lore.kernel.org/all/[email protected]

Quoted 3 lines

I don't like the idea to add obfuscation function just for the sake of obfuscation. If you don't want access to mlx5_core_dev, please argue with Jason to accept first variant.

I think you are confused, 1st version duplicated the logic that already existed in mlx5 core, didn't just access with MLX5_GET.