netdev - Re: [PATCH net-next 00/15] net: sparx5: prepare for lan969x switch driver

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: <63e7cced-5eaf-43ba-bb2c-b7a8609bedd7@intel.com>
Date: Wed, 2 Oct 2024 14:44:12 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Daniel Machon <daniel.machon@...rochip.com>
CC: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
	<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Lars Povlsen <lars.povlsen@...rochip.com>, "Steen
 Hegelund" <Steen.Hegelund@...rochip.com>, <horatiu.vultur@...rochip.com>,
	<jensemil.schulzostergaard@...rochip.com>, <UNGLinuxDriver@...rochip.com>,
	Richard Cochran <richardcochran@...il.com>, <horms@...nel.org>,
	<justinstitt@...gle.com>, <gal@...dia.com>, <aakash.r.menon@...il.com>,
	<netdev@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 00/15] net: sparx5: prepare for lan969x switch
 driver



On 10/2/2024 12:47 AM, Daniel Machon wrote:
> 
> Hi Jakob,
> 
> First off, thank you for your reviews - I really appreciate it.
> 
> Let me address your "variable scope" conerns:
> 
> I had the feeling that this could pontentially be somewhat contentious.
> 
> Basically, this comes down to making the least invasive changes to the
> existing driver code. With this approach:
> 
>     For the SPX5_CONST macro this means shorter lines, and less 80 char
>     wrapping.
> 
>     For the "*regs" variable this means not having to pass in the sparx5
>     pointer to *all* register macros, which requires changes *all* over
>     the code.
> 
> I thought the solution with an in-scope implicit variable was less
> invasive (and maybe even more readable?). Just my opinion, given the
> alternative.
>

Obviously there is style preference here, and someone working
day-in/day-out on the code is likely to know which macros have which
variable dependencies. As an external reviewer, I would not know that,
so I would find it surprising when looking at some code which passes a
parameter which is never directly used.

> Obviously I did a bit of research on this upfront, and I can point to
> *many* places where drivers do the exact same (not justifying the use,
> just pointing that out). Here is an intel driver that does the same [1]
> (look at the *hw variable)

Yea, I'm sure a lot of the old Intel drivers have bad examples :D I've
spent a career trying to improve this.

> 
> I am willing to come up with something different, if you really think
> this is a no-go. Let me hear your thoughts. I think this applies to your
> comments on #2, #3 and #6 as well.
> 

It seems that Jakub Kicinski wants the entire macro removed, and his
opinion as maintainer matters more than mine.

Personally, I'm not opposed to a macro itself especially if the direct
access starts to get very long. However, I think the parameter being
accessed should be, well, a parameter of the macro.

> /Daniel
> 
> [1] https://elixir.bootlin.com/linux/v6.12-rc1/source/drivers/net/ethernet/intel/igb/igb_main.c#L4746
> 
> 

As an example of *why* I don't like this practice: It took me a while to
realize the hw variable was implicit to wr32. And I worked on this driver.

Thanks,
Jake