netdev - Re: [PATCH v2 00/14] Add Microchip ZL3073x support (part 1)

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250411155816.GA3300495-robh@kernel.org>
Date: Fri, 11 Apr 2025 10:58:16 -0500
From: Rob Herring <robh@...nel.org>
To: Michal Schmidt <mschmidt@...hat.com>
Cc: Lee Jones <lee@...nel.org>, Ivan Vecera <ivecera@...hat.com>,
	netdev@...r.kernel.org, Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
	Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
	Jiri Pirko <jiri@...nulli.us>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Prathosh Satish <Prathosh.Satish@...rochip.com>,
	Kees Cook <kees@...nel.org>, Andy Shevchenko <andy@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2 00/14] Add Microchip ZL3073x support (part 1)

On Fri, Apr 11, 2025 at 04:27:08PM +0200, Michal Schmidt wrote:
> On Fri, Apr 11, 2025 at 9:26 AM Lee Jones <lee@...nel.org> wrote:
> > On Wed, 09 Apr 2025, Ivan Vecera wrote:
> > > Add support for Microchip Azurite DPLL/PTP/SyncE chip family that
> > > provides DPLL and PTP functionality. This series bring first part
> > > that adds the common MFD driver that provides an access to the bus
> > > that can be either I2C or SPI.
> > > [...]
> >
> > Not only are all of the added abstractions and ugly MACROs hard to read
> > and troublesome to maintain, they're also completely unnecessary at this
> > (driver) level.  Nicely authored, easy to read / maintain code wins over
> > clever code 95% of the time.
> 
> Hello Lee,
> 
> IMHO defining the registers with the ZL3073X_REG*_DEF macros is both
> clever and easy to read / maintain. On one line I can see the register
> name, size and address. For the indexed registers also their count and
> the stride. It's almost like looking at a datasheet. And the
> type-checking for accessing the registers using the correct size is
> nice. I even liked the paranoid WARN_ON for checking the index
> overflows.

If this is much better, define (and upstream) something for everyone to 
use rather than a one-off in some driver. It doesn't matter how great it 
is if it is different from everyone else. The last thing I want to do is 
figure out how register accesses work when looking at an unfamilar 
driver.

> The weak point is the non-obvious usage in call sites. Seeing:
>   rc = zl3073x_read_id(zldev, &id);
> can be confusing. One will not find the function with cscope or grep.

Exactly.

> Nothing immediately suggests that there's macro magic behind it.
> What if usage had to be just slightly more explicit?:
>   rc = ZL3073X_READ(id, zldev, &id);
> 
> I could immediately see that ZL3073X_READ is a macro. Its definition
> would be near the definitions of the ZL3073X_REG*_DEF macros, so I
> could correctly guess these things are related.
> The 1st argument of the ZL3073X_READ macro is the register name.
> (There would be a ZL3073X_READ_IDX with one more argument for indexed
> registers.)
> In vim, having the cursor on the 1st argument (id) and pressing gD
> takes me to the corresponding ZL3073X_REG16_DEF line.
> 
> Would it still be too ugly in your view?

If you have opinions on how register accesses should look, how to do 
them in rust is being defined now.

Rob