Re: [PATCH net-next v3 2/2] net/mlx5: implement swp_l4_csum_mode via devlink params

Jiri Pirko
2025-11-09T10:46:42Z

Sat, Nov 08, 2025 at 07:14:45AM +0100, saeed@kernel.org wrote:
>On 07 Nov 12:43, Daniel Zahka wrote:
>> swp_l4_csum_mode controls how L4 transmit checksums are computed when
>> using Software Parser (SWP) hints for header locations.
>>
>> Supported values:
>> 1. device_default: use device default setting.
>> 2. full_csum: calculate L4 checksum with the pseudo-header.
>> 3. l4_only: calculate L4 checksum without the pseudo-header. Only
>> available when swp_l4_csum_mode_l4_only is set in
>> mlx5_ifc_nv_sw_offload_cap_bits.
>>
>> The l4_only setting is a dependency for PSP initialization in
>> mlx5e_psp_init().
>>
>> Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
>> ---
>>
>> Notes:
>> v2:
>> - use extack in mlx5_nv_param_devlink_swp_l4_csum_mode_get()
>> - fix indentation issue in mlx5.rst entry
>>
>> Documentation/networking/devlink/mlx5.rst | 9 +
>> .../net/ethernet/mellanox/mlx5/core/devlink.h | 3 +-
>> .../mellanox/mlx5/core/lib/nv_param.c | 161 ++++++++++++++++++
>> 3 files changed, 172 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/networking/devlink/mlx5.rst b/Documentation/networking/devlink/mlx5.rst
>> index 0e5f9c76e514..675b5a1ec625 100644
>> --- a/Documentation/networking/devlink/mlx5.rst
>> +++ b/Documentation/networking/devlink/mlx5.rst
>> @@ -218,6 +218,15 @@ parameters.
>> * ``balanced`` : Merges fewer CQEs, resulting in a moderate compression ratio but maintaining a balance between bandwidth savings and performance
>> * ``aggressive`` : Merges more CQEs into a single entry, achieving a higher compression rate and maximizing performance, particularly under high traffic loads
>>
>> + * - ``swp_l4_csum_mode``
>> + - string
>> + - permanent
>> + - Configure how the L4 checksum is calculated by the device when using
>> + Software Parser (SWP) hints for header locations.
>> + * ``device_default`` : Use the device's default checksum calculation mode
>
>Let's omit the device, just "default".
>
>So, I checked a couple of flows internally, and it seems this allows
>some flexibility in the FW to decide later on which mode to pick,
>based on other parameters, which practically means
>"user has no preference on this param". Driver can only find out
>after boot, when it reads the runtime capabilities, but still
>this is a bug, by the time the driver reads this (in devlink), the
>default value should've already been determined by FW, so FW must
>return the actual runtime value. Which can only be one of the following

I don't think it is correct to expose the "default" as a value.

On read, user should see the configured value, either "full_csum" or
"l4_only". Reporting "default" to the user does not make any sense.
On write, user should pass either "full_csum" or "l4_only". Why we would
ever want to pass "default"?

Regardless this patch, since this is param to be reflected on fw reboot
(permanent cmode), I think it would be nice to expose indication if
param value passed to user currently affects the fw, or if it is going
to be applied after fw reboot. Perhaps a simple bool attr would do?

>
>> + * ``full_csum`` : Calculate full checksum including the pseudo-header
>> + * ``l4_only`` : Calculate L4-only checksum, excluding the pseudo-header …