Re: [PATCH v3 0/6] Add FIELD_MODIFY() helper

[Yury Norov <yury.norov@gmail.com>]

On Fri, Apr 18, 2025 at 04:35:22PM +0100, Marc Zyngier wrote:
> On Fri, 18 Apr 2025 16:08:38 +0100,
> Yury Norov <yury.norov@gmail.com> wrote:
> > 
> > On Thu, Apr 17, 2025 at 06:45:12PM +0100, Marc Zyngier wrote:
> > > On Thu, 17 Apr 2025 18:22:29 +0100,
> > > Andrew Lunn <andrew@lunn.ch> wrote:
> > > > 
> > > > On Thu, Apr 17, 2025 at 12:10:54PM +0100, Marc Zyngier wrote:
> > > > > On Thu, 17 Apr 2025 11:47:07 +0100,
> > > > > Luo Jie <quic_luoj@quicinc.com> wrote:
> > > > > > 
> > > > > > Add the helper FIELD_MODIFY() to the FIELD_XXX family of bitfield
> > > > > > macros. It is functionally similar as xxx_replace_bits(), but adds
> > > > > > the compile time checking to catch incorrect parameter type errors.
> > > > > > 
> > > > > > This series also converts the four instances of opencoded FIELD_MODIFY()
> > > > > > that are found in the core kernel files, to instead use the new
> > > > > > FIELD_MODIFY() macro. This is achieved with Coccinelle, by adding
> > > > > > the script field_modify.cocci.
> > > > > > 
> > > > > > The changes are validated on IPQ9574 SoC which uses ARM64 architecture.
> > > > > 
> > > > > We already have the *_replace_bits() functions (see
> > > > > include/linux/bitfield.h).
> > > > > 
> > > > > Why do we need extra helpers?
> > > > 
> > > > If you look at bitfield.h, the *_replace_bits() seem to be
> > > > undocumented internal macro magic, not something you are expected to
> > > > use. What you are expected to use in that file is however well
> > > > documented. The macro magic also means that cross referencing tools
> > > > don't find them.
> > > 
> > > $ git grep _replace_bits|  wc -l
> > > 1514
> > 
> > FIELD_PREP() only is used 10 times more.
> 
> And? I'm sure that if you count "+", you'll find it to be yet a few
> order of magnitudes more.

And nothing. It seems that you like statistics, so I shared some more.

> > > I think a bunch of people have found them, tooling notwithstanding.
> > > 
> > > As for the documentation, the commit message in 00b0c9b82663ac would
> > > be advantageously promoted to full-fledged kernel-doc.
> > 
> > The FIELD_MODIFY() and uxx_replace_bits() are simply different things.
> > 
> > FIELD_MODIFY() employs __BF_FIELD_CHECK(), which allows strict
> > parameters checking at compile time. And people like it. See
> > recent fixed-size GENMASK() series:
> > 
> > https://patchwork.kernel.org/comment/26283604/
> 
> I don't care much for what people like or not. I don't see this
> FIELD_MODIFY() as a particular improvement on *_replace_bits().

Sad to hear that. Those people are all kernel engineers, and they
deserve some respect.

We are talking about tooling here. People use tools only if they like
them. Luo likes FIELD_MODIFY() over (yes, undocumented and ungreppable)
xx_replace_bits() for the reasons he described very clearly. He's going
to use it in his driver shortly, and this arm64 detour has been made
after my request.

From my perspective, both functions have their right to live in kernel.
They are similar but not identical.

I'll take patch #1 in my branch. Regarding ARM64 part - it's up to you
either to switch to FIELD_MODIFY(), _replace_bits(), or do nothing.

Thanks,
Yury