diff mbox series

[mlx5-next,3/4] net/mlx5: Use query_special_contexts for mkeys

Message ID 849b3e708a147a3e2fc94277b805f5cc388f16ab.1672819469.git.leonro@nvidia.com (mailing list archive)
State Superseded
Headers show
Series Rely on firmware to get special mkeys | expand

Commit Message

Leon Romanovsky Jan. 4, 2023, 8:11 a.m. UTC 
From: Or Har-Toov <ohartoov@nvidia.com>

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 <ohartoov@nvidia.com>
Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 .../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(-)

Comments

Saeed Mahameed Jan. 6, 2023, 12:58 a.m. UTC | #1 
On 04 Jan 10:11, Leon Romanovsky wrote:
>From: Or Har-Toov <ohartoov@nvidia.com>
>
>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 <ohartoov@nvidia.com>
>Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
>Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>---
> .../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(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>index c76f15505a76..33d7a7095988 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>@@ -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;
> 			}
> 		}
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>index 7f5db13e3550..d39d758744a0 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>@@ -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)] = {};
>+	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 .. 

>+		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.

>+	return 0;
>+}
>+
> static int mlx5_load(struct mlx5_core_dev *dev)
> {
> 	int err;
>@@ -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;
>+		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. 

> 	err = mlx5_irq_table_create(dev);
> 	if (err) {
> 		mlx5_core_err(dev, "Failed to alloc IRQs\n");
>diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
>index d476255c9a3f..5f2c4038d638 100644
>--- a/include/linux/mlx5/driver.h
>+++ b/include/linux/mlx5/driver.h
>@@ -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 {
>-- 
>2.38.1
>
Leon Romanovsky Jan. 8, 2023, 10:32 a.m. UTC | #2 
On Thu, Jan 05, 2023 at 04:58:22PM -0800, Saeed Mahameed wrote:
> On 04 Jan 10:11, Leon Romanovsky wrote:
> > From: Or Har-Toov <ohartoov@nvidia.com>
> > 
> > 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 <ohartoov@nvidia.com>
> > Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> > .../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(-)
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > index c76f15505a76..33d7a7095988 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > @@ -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;
> > 			}
> > 		}
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > index 7f5db13e3550..d39d758744a0 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > @@ -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)] = {};
> > +	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 ..

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

> 
> > +		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.

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

NP.

> 
> > +	return 0;
> > +}
> > +
> > static int mlx5_load(struct mlx5_core_dev *dev)
> > {
> > 	int err;
> > @@ -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;
> > +		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.

I'll take a look.

Thanks

> 
> > 	err = mlx5_irq_table_create(dev);
> > 	if (err) {
> > 		mlx5_core_err(dev, "Failed to alloc IRQs\n");
> > diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
> > index d476255c9a3f..5f2c4038d638 100644
> > --- a/include/linux/mlx5/driver.h
> > +++ b/include/linux/mlx5/driver.h
> > @@ -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 {
> > -- 
> > 2.38.1
> >
Saeed Mahameed Jan. 9, 2023, 10:31 p.m. UTC | #3 
On 08 Jan 12:32, Leon Romanovsky wrote:
>On Thu, Jan 05, 2023 at 04:58:22PM -0800, Saeed Mahameed wrote:
>> On 04 Jan 10:11, Leon Romanovsky wrote:
>> > From: Or Har-Toov <ohartoov@nvidia.com>
>> >
>> > 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 <ohartoov@nvidia.com>
>> > Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
>> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>> > ---
>> > .../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(-)
>> >
>> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> > index c76f15505a76..33d7a7095988 100644
>> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> > @@ -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;
>> > 			}
>> > 		}
>> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> > index 7f5db13e3550..d39d758744a0 100644
>> > --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> > @@ -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)] = {};
>> > +	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 ..
>
>According to the documentation, this functionality was forever, but you
>are presenting valid concern.
>
>>
>> > +		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.
>

Then please don't change mlx5e to use  mdev->terminate_scatter_list_mkey in
this patch, just keep it as is and have a separate patch to change mlx5e.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index c76f15505a76..33d7a7095988 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -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;
 			}
 		}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 7f5db13e3550..d39d758744a0 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -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)] = {};
+	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)) {
+		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;
+	return 0;
+}
+
 static int mlx5_load(struct mlx5_core_dev *dev)
 {
 	int err;
@@ -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");
+		goto err_irq_table;
+	}
 	err = mlx5_irq_table_create(dev);
 	if (err) {
 		mlx5_core_err(dev, "Failed to alloc IRQs\n");
diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
index d476255c9a3f..5f2c4038d638 100644
--- a/include/linux/mlx5/driver.h
+++ b/include/linux/mlx5/driver.h
@@ -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 {