Extend the TIR API and use it in mlx5e_modify_tirs_lb() instead of the
explicit modify_tir code.
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
Reviewed-by: Carolina Jubran <cjubran@nvidia.com>
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
---
.../net/ethernet/mellanox/mlx5/core/en/tir.c | 29 +++++++++++++++++--
.../net/ethernet/mellanox/mlx5/core/en/tir.h | 3 ++
.../ethernet/mellanox/mlx5/core/en_common.c | 29 +++++--------------
3 files changed, 37 insertions(+), 24 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tir.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tir.c
index 19499072f67f..0b55e77f19c8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tir.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tir.c
@@ -146,6 +146,31 @@ void mlx5e_tir_builder_build_direct(struct mlx5e_tir_builder *builder)
MLX5_SET(tirc, tirc, rx_hash_fn, MLX5_RX_HASH_FN_INVERTED_XOR8);
}
+static void mlx5e_tir_context_self_lb_block(void *tirc, bool enable_uc_lb,
+ bool enable_mc_lb)
+{
+ u8 lb_flags = 0;
+
+ if (enable_uc_lb)
+ lb_flags = MLX5_TIRC_SELF_LB_BLOCK_BLOCK_UNICAST;
+ if (enable_mc_lb)
+ lb_flags |= MLX5_TIRC_SELF_LB_BLOCK_BLOCK_MULTICAST;
+
+ MLX5_SET(tirc, tirc, self_lb_block, lb_flags);
+}
+
+void mlx5e_tir_builder_build_self_lb_block(struct mlx5e_tir_builder *builder,
+ bool enable_uc_lb,
+ bool enable_mc_lb)
+{
+ void *tirc = mlx5e_tir_builder_get_tirc(builder);
+
+ if (builder->modify)
+ MLX5_SET(modify_tir_in, builder->in, bitmask.self_lb_en, 1);
+
+ mlx5e_tir_context_self_lb_block(tirc, enable_uc_lb, enable_mc_lb);
+}
+
void mlx5e_tir_builder_build_tls(struct mlx5e_tir_builder *builder)
{
void *tirc = mlx5e_tir_builder_get_tirc(builder);
@@ -153,9 +178,7 @@ void mlx5e_tir_builder_build_tls(struct mlx5e_tir_builder *builder)
WARN_ON(builder->modify);
MLX5_SET(tirc, tirc, tls_en, 1);
- MLX5_SET(tirc, tirc, self_lb_block,
- MLX5_TIRC_SELF_LB_BLOCK_BLOCK_UNICAST |
- MLX5_TIRC_SELF_LB_BLOCK_BLOCK_MULTICAST);
+ mlx5e_tir_context_self_lb_block(tirc, true, true);
}
int mlx5e_tir_init(struct mlx5e_tir *tir, struct mlx5e_tir_builder *builder,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tir.h b/drivers/net/ethernet/mellanox/mlx5/core/en/tir.h
index e8df3aaf6562..958eeb959a19 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tir.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tir.h
@@ -35,6 +35,9 @@ void mlx5e_tir_builder_build_rss(struct mlx5e_tir_builder *builder,
const struct mlx5e_rss_params_traffic_type *rss_tt,
bool inner);
void mlx5e_tir_builder_build_direct(struct mlx5e_tir_builder *builder);
+void mlx5e_tir_builder_build_self_lb_block(struct mlx5e_tir_builder *builder,
+ bool enable_uc_lb,
+ bool enable_mc_lb);
void mlx5e_tir_builder_build_tls(struct mlx5e_tir_builder *builder);
struct mlx5_core_dev;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_common.c b/drivers/net/ethernet/mellanox/mlx5/core/en_common.c
index 376a018b2db1..fad6b761f622 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_common.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_common.c
@@ -250,43 +250,30 @@ void mlx5e_destroy_mdev_resources(struct mlx5_core_dev *mdev)
int mlx5e_modify_tirs_lb(struct mlx5_core_dev *mdev, bool enable_uc_lb,
bool enable_mc_lb)
{
+ struct mlx5e_tir_builder *builder;
struct mlx5e_tir *tir;
- u8 lb_flags = 0;
int err = 0;
- u32 tirn = 0;
- int inlen;
- void *in;
- inlen = MLX5_ST_SZ_BYTES(modify_tir_in);
- in = kvzalloc(inlen, GFP_KERNEL);
- if (!in)
+ builder = mlx5e_tir_builder_alloc(true);
+ if (!builder)
return -ENOMEM;
- if (enable_uc_lb)
- lb_flags = MLX5_TIRC_SELF_LB_BLOCK_BLOCK_UNICAST;
-
- if (enable_mc_lb)
- lb_flags |= MLX5_TIRC_SELF_LB_BLOCK_BLOCK_MULTICAST;
-
- if (lb_flags)
- MLX5_SET(modify_tir_in, in, ctx.self_lb_block, lb_flags);
-
- MLX5_SET(modify_tir_in, in, bitmask.self_lb_en, 1);
+ mlx5e_tir_builder_build_self_lb_block(builder, enable_uc_lb,
+ enable_mc_lb);
mutex_lock(&mdev->mlx5e_res.hw_objs.td.list_lock);
list_for_each_entry(tir, &mdev->mlx5e_res.hw_objs.td.tirs_list, list) {
- tirn = tir->tirn;
- err = mlx5_core_modify_tir(mdev, tirn, in);
+ err = mlx5e_tir_modify(tir, builder);
if (err)
break;
}
mutex_unlock(&mdev->mlx5e_res.hw_objs.td.list_lock);
- kvfree(in);
+ mlx5e_tir_builder_free(builder);
if (err)
mlx5_core_err(mdev,
"modify tir(0x%x) enable_lb uc(%d) mc(%d) failed, %d\n",
- tirn,
+ mlx5e_tir_get_tirn(tir),
enable_uc_lb, enable_mc_lb, err);
return err;
--
2.31.1On Thu, Oct 23, 2025 at 09:43:35AM +0300, Tariq Toukan wrote:
...
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_common.c b/drivers/net/ethernet/mellanox/mlx5/core/en_common.c
> index 376a018b2db1..fad6b761f622 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_common.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_common.c
> @@ -250,43 +250,30 @@ void mlx5e_destroy_mdev_resources(struct mlx5_core_dev *mdev)
> int mlx5e_modify_tirs_lb(struct mlx5_core_dev *mdev, bool enable_uc_lb,
> bool enable_mc_lb)
...
> list_for_each_entry(tir, &mdev->mlx5e_res.hw_objs.td.tirs_list, list) {
> - tirn = tir->tirn;
> - err = mlx5_core_modify_tir(mdev, tirn, in);
> + err = mlx5e_tir_modify(tir, builder);
> if (err)
> break;
> }
> mutex_unlock(&mdev->mlx5e_res.hw_objs.td.list_lock);
>
> - kvfree(in);
> + mlx5e_tir_builder_free(builder);
> if (err)
> mlx5_core_err(mdev,
> "modify tir(0x%x) enable_lb uc(%d) mc(%d) failed, %d\n",
> - tirn,
> + mlx5e_tir_get_tirn(tir),
Sorry, for not noticing this before sending my previous email.
Coccinelle complains about the line above like this:
.../en_common.c:276:28-31: ERROR: invalid reference to the index variable of the iterator on line 265
I think this is a false positive because the problem only occurs if
the list iteration runs to completion. But err guards against
tir being used in that case.
But, perhaps, to be on the safe side, it would be good practice
to stash tir somewhere?
> enable_uc_lb, enable_mc_lb, err);
>
> return err;
> --
> 2.31.1
>
>
On 23/10/2025 16:31, Simon Horman wrote:
> On Thu, Oct 23, 2025 at 09:43:35AM +0300, Tariq Toukan wrote:
>
> ...
>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_common.c b/drivers/net/ethernet/mellanox/mlx5/core/en_common.c
>> index 376a018b2db1..fad6b761f622 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_common.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_common.c
>> @@ -250,43 +250,30 @@ void mlx5e_destroy_mdev_resources(struct mlx5_core_dev *mdev)
>> int mlx5e_modify_tirs_lb(struct mlx5_core_dev *mdev, bool enable_uc_lb,
>> bool enable_mc_lb)
>
> ...
>
>> list_for_each_entry(tir, &mdev->mlx5e_res.hw_objs.td.tirs_list, list) {
>> - tirn = tir->tirn;
>> - err = mlx5_core_modify_tir(mdev, tirn, in);
>> + err = mlx5e_tir_modify(tir, builder);
>> if (err)
>> break;
>> }
>> mutex_unlock(&mdev->mlx5e_res.hw_objs.td.list_lock);
>>
>> - kvfree(in);
>> + mlx5e_tir_builder_free(builder);
>> if (err)
>> mlx5_core_err(mdev,
>> "modify tir(0x%x) enable_lb uc(%d) mc(%d) failed, %d\n",
>> - tirn,
>> + mlx5e_tir_get_tirn(tir),
>
> Sorry, for not noticing this before sending my previous email.
>
> Coccinelle complains about the line above like this:
>
> .../en_common.c:276:28-31: ERROR: invalid reference to the index variable of the iterator on line 265
>
> I think this is a false positive because the problem only occurs if
> the list iteration runs to completion. But err guards against
> tir being used in that case.
>
Exactly.
> But, perhaps, to be on the safe side, it would be good practice
> to stash tir somewhere?
>
I tried to keep the error print out of the critical lock section.
It's not time-sensitive so optimization is not really needed.
I can simply move the print inside where it belongs.
>> enable_uc_lb, enable_mc_lb, err);
>>
>> return err;
>> --
>> 2.31.1
>>
>>
>
On 10/26/25 1:50 PM, Tariq Toukan wrote:
> On 23/10/2025 16:31, Simon Horman wrote:
>> On Thu, Oct 23, 2025 at 09:43:35AM +0300, Tariq Toukan wrote:
>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_common.c b/drivers/net/ethernet/mellanox/mlx5/core/en_common.c
>>> index 376a018b2db1..fad6b761f622 100644
>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_common.c
>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_common.c
>>> @@ -250,43 +250,30 @@ void mlx5e_destroy_mdev_resources(struct mlx5_core_dev *mdev)
>>> int mlx5e_modify_tirs_lb(struct mlx5_core_dev *mdev, bool enable_uc_lb,
>>> bool enable_mc_lb)
>>
>> ...
>>
>>> list_for_each_entry(tir, &mdev->mlx5e_res.hw_objs.td.tirs_list, list) {
>>> - tirn = tir->tirn;
>>> - err = mlx5_core_modify_tir(mdev, tirn, in);
>>> + err = mlx5e_tir_modify(tir, builder);
>>> if (err)
>>> break;
>>> }
>>> mutex_unlock(&mdev->mlx5e_res.hw_objs.td.list_lock);
>>>
>>> - kvfree(in);
>>> + mlx5e_tir_builder_free(builder);
>>> if (err)
>>> mlx5_core_err(mdev,
>>> "modify tir(0x%x) enable_lb uc(%d) mc(%d) failed, %d\n",
>>> - tirn,
>>> + mlx5e_tir_get_tirn(tir),
>>
>> Sorry, for not noticing this before sending my previous email.
>>
>> Coccinelle complains about the line above like this:
>>
>> .../en_common.c:276:28-31: ERROR: invalid reference to the index variable of the iterator on line 265
>>
>> I think this is a false positive because the problem only occurs if
>> the list iteration runs to completion. But err guards against
>> tir being used in that case.
>>
>
> Exactly.
>
>> But, perhaps, to be on the safe side, it would be good practice
>> to stash tir somewhere?
>>
>
> I tried to keep the error print out of the critical lock section.
> It's not time-sensitive so optimization is not really needed.
> I can simply move the print inside where it belongs.
Yes please!
Also the printk is performed on error, should not affect the path
critical path performances even if that would be time-sensitive.
/P
On Thu, Oct 23, 2025 at 09:43:35AM +0300, Tariq Toukan wrote:
...
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tir.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tir.c
> index 19499072f67f..0b55e77f19c8 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tir.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tir.c
> @@ -146,6 +146,31 @@ void mlx5e_tir_builder_build_direct(struct mlx5e_tir_builder *builder)
> MLX5_SET(tirc, tirc, rx_hash_fn, MLX5_RX_HASH_FN_INVERTED_XOR8);
> }
>
> +static void mlx5e_tir_context_self_lb_block(void *tirc, bool enable_uc_lb,
> + bool enable_mc_lb)
> +{
> + u8 lb_flags = 0;
> +
> + if (enable_uc_lb)
> + lb_flags = MLX5_TIRC_SELF_LB_BLOCK_BLOCK_UNICAST;
> + if (enable_mc_lb)
> + lb_flags |= MLX5_TIRC_SELF_LB_BLOCK_BLOCK_MULTICAST;
> +
> + MLX5_SET(tirc, tirc, self_lb_block, lb_flags);
> +}
> +
> +void mlx5e_tir_builder_build_self_lb_block(struct mlx5e_tir_builder *builder,
> + bool enable_uc_lb,
> + bool enable_mc_lb)
> +{
> + void *tirc = mlx5e_tir_builder_get_tirc(builder);
> +
> + if (builder->modify)
> + MLX5_SET(modify_tir_in, builder->in, bitmask.self_lb_en, 1);
> +
> + mlx5e_tir_context_self_lb_block(tirc, enable_uc_lb, enable_mc_lb);
> +}
> +
...
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_common.c b/drivers/net/ethernet/mellanox/mlx5/core/en_common.c
> index 376a018b2db1..fad6b761f622 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_common.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_common.c
> @@ -250,43 +250,30 @@ void mlx5e_destroy_mdev_resources(struct mlx5_core_dev *mdev)
> int mlx5e_modify_tirs_lb(struct mlx5_core_dev *mdev, bool enable_uc_lb,
> bool enable_mc_lb)
...
> - if (enable_uc_lb)
> - lb_flags = MLX5_TIRC_SELF_LB_BLOCK_BLOCK_UNICAST;
> -
> - if (enable_mc_lb)
> - lb_flags |= MLX5_TIRC_SELF_LB_BLOCK_BLOCK_MULTICAST;
> -
> - if (lb_flags)
> - MLX5_SET(modify_tir_in, in, ctx.self_lb_block, lb_flags);
> -
> - MLX5_SET(modify_tir_in, in, bitmask.self_lb_en, 1);
> + mlx5e_tir_builder_build_self_lb_block(builder, enable_uc_lb,
> + enable_mc_lb);
Hi,
Maybe I'm reading this wrong, and possibly it is not important,
but it seems to me that the update above reverses the
...
order of the MLX5_SET() invocations.
On 23/10/2025 16:02, Simon Horman wrote:
> On Thu, Oct 23, 2025 at 09:43:35AM +0300, Tariq Toukan wrote:
>
> ...
>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tir.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tir.c
>> index 19499072f67f..0b55e77f19c8 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tir.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tir.c
>> @@ -146,6 +146,31 @@ void mlx5e_tir_builder_build_direct(struct mlx5e_tir_builder *builder)
>> MLX5_SET(tirc, tirc, rx_hash_fn, MLX5_RX_HASH_FN_INVERTED_XOR8);
>> }
>>
>> +static void mlx5e_tir_context_self_lb_block(void *tirc, bool enable_uc_lb,
>> + bool enable_mc_lb)
>> +{
>> + u8 lb_flags = 0;
>> +
>> + if (enable_uc_lb)
>> + lb_flags = MLX5_TIRC_SELF_LB_BLOCK_BLOCK_UNICAST;
>> + if (enable_mc_lb)
>> + lb_flags |= MLX5_TIRC_SELF_LB_BLOCK_BLOCK_MULTICAST;
>> +
>> + MLX5_SET(tirc, tirc, self_lb_block, lb_flags);
>> +}
>> +
>> +void mlx5e_tir_builder_build_self_lb_block(struct mlx5e_tir_builder *builder,
>> + bool enable_uc_lb,
>> + bool enable_mc_lb)
>> +{
>> + void *tirc = mlx5e_tir_builder_get_tirc(builder);
>> +
>> + if (builder->modify)
>> + MLX5_SET(modify_tir_in, builder->in, bitmask.self_lb_en, 1);
>> +
>> + mlx5e_tir_context_self_lb_block(tirc, enable_uc_lb, enable_mc_lb);
>> +}
>> +
>
> ...
>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_common.c b/drivers/net/ethernet/mellanox/mlx5/core/en_common.c
>> index 376a018b2db1..fad6b761f622 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_common.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_common.c
>> @@ -250,43 +250,30 @@ void mlx5e_destroy_mdev_resources(struct mlx5_core_dev *mdev)
>> int mlx5e_modify_tirs_lb(struct mlx5_core_dev *mdev, bool enable_uc_lb,
>> bool enable_mc_lb)
>
> ...
>
>> - if (enable_uc_lb)
>> - lb_flags = MLX5_TIRC_SELF_LB_BLOCK_BLOCK_UNICAST;
>> -
>> - if (enable_mc_lb)
>> - lb_flags |= MLX5_TIRC_SELF_LB_BLOCK_BLOCK_MULTICAST;
>> -
>> - if (lb_flags)
>> - MLX5_SET(modify_tir_in, in, ctx.self_lb_block, lb_flags);
>> -
>> - MLX5_SET(modify_tir_in, in, bitmask.self_lb_en, 1);
>> + mlx5e_tir_builder_build_self_lb_block(builder, enable_uc_lb,
>> + enable_mc_lb);
>
> Hi,
>
> Maybe I'm reading this wrong, and possibly it is not important,
> but it seems to me that the update above reverses the
>
> ...
> order of the MLX5_SET() invocations.
>
The order here is not important.
The FW command is filled in any order and only then FW is called.
© 2016 - 2026 Red Hat, Inc.