Re: [PATCH net-next 4/4] net/mlx5: Add sensor name to temperature event message

On 19 Feb 07:28, Jakub Kicinski wrote:

On Wed, 19 Feb 2025 15:00:57 +0200 Tariq Toukan wrote:

quoted

quoted

quoted

If you have to respin for some other reason, please consider limiting lines
to 80 columns wide or less here and elsewhere in this patch where it
doesn't reduce readability (subjective I know).

+1, please try to catch such situations going forward

This was not missed.
This is not a new thing...
We've been enforcing a max line length of 100 chars in mlx5 driver for
the past few years.
I don't have the full image now, but I'm convinced that this dates back
to an agreement between the mlx5 and netdev maintainers at that time.

80 chars could be too restrictive, especially with today's large
monitors, while 100-chars is still highly readable.
This is subjective of course...

If you don't have a strong preference, we'll keep the current 100 chars
limit. Otherwise, just let me know and we'll start enforcing the
80-chars limit for future patches.

Right, I think mlx5 is the only exception to the 80 column guidance.
I don't think it's resulting in more readable code, so yes, my
preference is to end this experiment.

The reason in mlx5 was that we wanted to preserve the official HW spec
auto-generated fields names and they are really long.
100 chars worked very well with us for example the following sequence of
code setting up a FW command buffer would have to be broken in every line
if we were to restrict 80 chars per line.

  MLX5_SET(modify_vhca_state_in, in, opcode, MLX5_CMD_OP_MODIFY_VHCA_STATE);
  MLX5_SET(modify_vhca_state_in, in, vhca_state_field_select.sw_function_id, 1);
  MLX5_SET(modify_vhca_state_in, in, vhca_state_context.sw_function_id, sw_fn_id);
  MLX5_SET(modify_vhca_state_in, in, vhca_state_context.arm_change_event, 1);
  MLX5_SET(modify_vhca_state_in, in, vhca_state_field_select.arm_change_event, 1);

But I believe the driver grow larger than caring about those lines too
much, I just did a quick check and it seems less than 2% of the lines are
actually > 80, not sure this is due to being more strict in the past few
years or that we don't really need more than 80 lines.

I also check the interesting cases with macros such
MLX5_SET/MLX5_GET/MLX5_CAP and also the percentile of long lines was very
minor just about 5% in all cases.. 

So I kinda agree mlx5 doesn't need be so special anymore. 
Tariq up to you, you are the main  reviewer now.

Thanks
Saeed.