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.

Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
---
Changes in v3:
- Correct the order of header files included.
- Add the Coccinelle script field_modify.cocci..
- Convert the opencoded FIELD_MODIFY() variants inside arm64 directory,
  identified by field_modify.cocci.
- Link to v2: https://lore.kernel.org/all/20250410131048.2054791-1-quic_luoj@quicinc.com/

Changes in v2:
- Update the documented example for FIELD_MODIFY().
- Improve the commit message to describe the need for the change.
- Link to v1: https://lore.kernel.org/all/20250318071526.1836194-1-quic_luoj@quicinc.com/

---
Luo Jie (6):
      bitfield: Add FIELD_MODIFY() helper
      coccinelle: misc: Add field_modify script
      arm64: tlb: Convert the opencoded field modify
      arm64: nvhe: Convert the opencoded field modify
      arm64: kvm: Convert the opencoded field modify
      arm64: mm: Convert the opencoded field modify

 arch/arm64/include/asm/tlbflush.h          |  3 +--
 arch/arm64/kvm/hyp/include/nvhe/memory.h   |  3 +--
 arch/arm64/kvm/vgic/vgic-mmio-v3.c         |  6 ++----
 arch/arm64/mm/context.c                    |  3 +--
 include/linux/bitfield.h                   | 21 +++++++++++++++++++--
 scripts/coccinelle/misc/field_modify.cocci | 24 ++++++++++++++++++++++++
 6 files changed, 48 insertions(+), 12 deletions(-)
---
base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
change-id: 20250411-field_modify-83b58db99025

Best regards,
-- 
Luo Jie <quic_luoj@quicinc.com>

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?

	M.

-- 
Without deviation from the norm, progress is not possible.

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.

I think this is a useful additional to bitfield.h.

	Andrew

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

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.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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.
 
> 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/

The _replace_bits() functions return fixed-width values, and intended
for: "manipulating bitfields both in host- and fixed-endian", as the
very first line in the commit message says.

Those using _replace_bits() for something else abuse the API, and
should switch to FIELD_MODIFY().

On Fri, Apr 18, 2025 at 11:08:38AM -0400, Yury Norov wrote:
> The _replace_bits() functions return fixed-width values, and intended
> for: "manipulating bitfields both in host- and fixed-endian", as the
> very first line in the commit message says.
> 
> Those using _replace_bits() for something else abuse the API, and
> should switch to FIELD_MODIFY().

Sorry, but please explain this statement, because it means nothing to
me.

FIELD_MODIFY() replaces bits in host endian. _replace_bits() also
replaces bits, but has a wider range of which encompass FIELD_MODIFY().

I see nothing that precludes using using _replace_bits() with
bitfields.

I see nothing that would differentiate the behaviour, other than maybe
religous ideals about C functions vs macros or upper vs lower case.

Please explain why you think there's a difference between the two
because I really can't see any reason not to use one over the other
apart from asthetics.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

On Wed, Apr 23, 2025 at 06:44:15PM +0100, Russell King (Oracle) wrote:
> On Fri, Apr 18, 2025 at 11:08:38AM -0400, Yury Norov wrote:
> > The _replace_bits() functions return fixed-width values, and intended
> > for: "manipulating bitfields both in host- and fixed-endian", as the
> > very first line in the commit message says.
> > 
> > Those using _replace_bits() for something else abuse the API, and
> > should switch to FIELD_MODIFY().
> 
> Sorry, but please explain this statement, because it means nothing to
> me.
> 
> FIELD_MODIFY() replaces bits in host endian. _replace_bits() also
> replaces bits, but has a wider range of which encompass FIELD_MODIFY().
> 
> I see nothing that precludes using using _replace_bits() with
> bitfields.
> 
> I see nothing that would differentiate the behaviour, other than maybe
> religous ideals about C functions vs macros or upper vs lower case.

Interesting, never heard about religious ideals in C.
 
> Please explain why you think there's a difference between the two
> because I really can't see any reason not to use one over the other
> apart from asthetics.

I explained that in subtread for 4/6 in this series. Shortly it's
about compiler's ability to catch various errors, like overflows,
and (not unlikely) generated code quality.

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.

>  
> > 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().

> The _replace_bits() functions return fixed-width values, and intended
> for: "manipulating bitfields both in host- and fixed-endian", as the
> very first line in the commit message says.
> 
> Those using _replace_bits() for something else abuse the API, and
> should switch to FIELD_MODIFY().

Or not.

	M.

-- 
Jazz isn't dead. It just smells funny.

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

On 4/19/2025 1:04 AM, Yury Norov wrote:
>>>> 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

Thank you for reviewing and discussing this patch series. The newly
added macro FIELD_MODIFY() will be utilized by the Qualcomm PPE (Packet
Processing Engine) Ethernet driver. Regarding the ARM64 core files,
could you please provide guidance on the preferred approach?