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

[Jacob Keller]

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