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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z7erLQwDBHYoCV7X@x130>
Date: Thu, 20 Feb 2025 14:22:37 -0800
From: Saeed Mahameed <saeed@...nel.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Tariq Toukan <ttoukan.linux@...il.com>,
	Tariq Toukan <tariqt@...dia.com>, Simon Horman <horms@...nel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	Shahar Shitrit <shshitrit@...dia.com>,
	Gal Pressman <gal@...dia.com>, Saeed Mahameed <saeedm@...dia.com>,
	Leon Romanovsky <leon@...nel.org>, netdev@...r.kernel.org,
	linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org,
	Carolina Jubran <cjubran@...dia.com>
Subject: 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:
>> >> 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.