Re: [PATCH v2 0/2] nvmem: fix a const-unsoundness in reg_write

[Link Mauve <linkmauve@linkmauve.fr>]

[-- Attachment #1: Type: text/plain, Size: 3385 bytes --]

On Thu, Jul 16, 2026 at 12:03:17PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 15, 2026 at 09:55:16PM +0200, Link Mauve wrote:
> > This callback used to take a mutable void * for no reason, which causes
> > the compiler to be unaware that the val buffer should never be modified
> > by the callback.
> > 
> > This was found while drafting the nvmem-provider Rust abstraction.
> > 
> > Thanks to the guidance of Andy Shevchenko, this now introduces a new
> > callback and deprecates the existing one, with the goal of renaming the
> > new one into the old one once no user remains in the kernel.
> 
> You forgot to use --base. It's unclear against what should be this applied.
> I tried Linux Next (next-20260715), and it fails.
> 
> Yes, it applies against v7.2-rc3, but it means that this won't be applied on
> top of maintainer's tree (which has something already that you have to take
> into consideration).

Indeed I developed against Linus’s master, I’ll rebase on linux-next for
v3.

> 
> For the record, the first version of the series was no go as the first patch
> there breaks the things, like
> 
> drivers/nvmem/qfprom.c:446:22: error: incompatible function pointer types assigning to 'nvmem_reg_write_t' (aka 'int (*)(void *, unsigned int, const void *, unsigned long)') from 'int (void *, unsigned int, void *, size_t)' (aka 'int (void *, unsigned int, void *, unsigned long)') [-Wincompatible-function-pointer-types]
>   446 |                         econfig.reg_write = qfprom_reg_write;
>       |                                           ^ ~~~~~~~~~~~~~~~~
> 1 error generated.
> 
> This version doesn't have this issue (at least with my smoke build tests
> on x86_64).

Yup, I enabled the drivers but didn’t select COMPILE_TEST, so they
didn’t actually build and I assumed everything was correct…  This won’t
happen again.

> 
> Now, what catches me is that regmap_bulk_read() proto used for both cases in
> drivers/nvmem/apple-spmi-nvmem.c without any changes. Which makes me think
> that the approach can be done in a simpler way, id est converting users first
> to use const specifiers in their callbacks first. But this trick is done with
> using (void *) casting (?) which makes warning to disappear, which is
> interesting case. So I think the Apple driver should actually use proper
> protos and hence wrappers, otherwise it makes compiler blind, which is not
> good. TL;DR: you should fix the Apple driver (and might more if any of them
> use that dirty trick).

This series only affects the reg_write callback, not reg_read, so it’s
to be expected that regmap_bulk_read() keeps its void * parameter as it
will modify it, it’s regmap_bulk_write() which correctly takes const
void *.  The Apple driver completely removes all function pointer safety
by casting the function into void *, although that’s not due to const
incompatibility but due to the first argument being struct regmap *
instead of void *.

I’ve attached a patch fixing this particular issue, I will include it in
my v3 if that’s ok with you (or it could go as a different series, I
don’t care much).

I think I’ve found another bug in that driver, it says .max_register =
0xffff but .size = 0xffff, one or the other is probably off-by-one, and
I suspect .size should be 0x10000 instead.

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Thanks!

-- 
Link Mauve

[-- Attachment #2: 0001-nvmem-apple-spmi-improve-calling-safety-with-wrapper.patch --]
[-- Type: text/plain, Size: 1873 bytes --]

From ec85ce97c6c835d99aa28b11b78af2744b45dc49 Mon Sep 17 00:00:00 2001
From: Link Mauve <linkmauve@linkmauve.fr>
Date: Thu, 16 Jul 2026 12:59:13 +0200
Subject: [PATCH] nvmem: apple-spmi: improve calling safety with wrapper
 functions

This driver used to cast the regmap_bulk_*() functions to void *,
bypassing any compiler safety around incompatible function pointers.

With two small helpers, which just convert the void * priv parameter
into the wanted struct regmap *, we can remove the void * cast
altogether.

Signed-off-by: Link Mauve <linkmauve@linkmauve.fr>
---
 drivers/nvmem/apple-spmi-nvmem.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/nvmem/apple-spmi-nvmem.c b/drivers/nvmem/apple-spmi-nvmem.c
index cbf25c53d048..81d9e242c836 100644
--- a/drivers/nvmem/apple-spmi-nvmem.c
+++ b/drivers/nvmem/apple-spmi-nvmem.c
@@ -18,6 +18,20 @@ static const struct regmap_config apple_spmi_regmap_config = {
 	.max_register	= 0xffff,
 };
 
+static int apple_spmi_nvmem_read(void *priv, unsigned int offset,
+				 void *val, size_t bytes)
+{
+	struct regmap *regmap = priv;
+	return regmap_bulk_read(regmap, offset, val, bytes);
+}
+
+static int apple_spmi_nvmem_write(void *priv, unsigned int offset,
+				  const void *val, size_t bytes)
+{
+	struct regmap *regmap = priv;
+	return regmap_bulk_write(regmap, offset, val, bytes);
+}
+
 static int apple_spmi_nvmem_probe(struct spmi_device *sdev)
 {
 	struct regmap *regmap;
@@ -28,8 +42,8 @@ static int apple_spmi_nvmem_probe(struct spmi_device *sdev)
 		.word_size = 1,
 		.stride = 1,
 		.size = 0xffff,
-		.reg_read = (void *)regmap_bulk_read,
-		.reg_write_const = (void *)regmap_bulk_write,
+		.reg_read = apple_spmi_nvmem_read,
+		.reg_write_const = apple_spmi_nvmem_write,
 	};
 
 	regmap = devm_regmap_init_spmi_ext(sdev, &apple_spmi_regmap_config);
-- 
2.55.0


[-- Attachment #3: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic