Re: [PATCH v2 06/16] nvmem: add mt6323 PMIC EFUSE driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unsafe device topology assumption leads to a NULL pointer dereference and kernel panic.
- [Medium] Direct 16-bit assignments to the output buffer cause endianness corruption and potential unaligned memory access.
- [Low] Kconfig help text documents an incorrect module name.
--

commit 4bbd96ffdcf57f8479b893cecdcd10f45fef30fa
Author: Roman Vivchar <rva333@protonmail.com>

nvmem: add mt6323 PMIC EFUSE driver

This commit adds support for the EFUSE controller found in the Mediatek
MT6323 PMIC. It registers an nvmem device to provide access to 24 bytes
of hardware-related data, such as thermal sensor calibration values.

> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 74ddbd0f79b0e..db248a3c4e879 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
[ ... ]
> @@ -227,6 +227,17 @@ config NVMEM_MTK_EFUSE
>  	  This driver can also be built as a module. If so, the module
>  	  will be called efuse-mtk.
>  
> +config NVMEM_MT6323_EFUSE
> +	tristate "Mediatek MT6323 PMIC EFUSE support"
> +	depends on ARCH_MEDIATEK || COMPILE_TEST
> +	depends on MFD_MT6397
> +	help
> +	  This is a driver to access hardware related data like sensor
> +	  calibration, etc.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called efuse-mt6323.

This isn't a bug, but the help text says the module will be called
efuse-mt6323, while the Makefile specifies the build target as
nvmem_mt6323-efuse.o.

[ ... ]
> diff --git a/drivers/nvmem/mt6323-efuse.c b/drivers/nvmem/mt6323-efuse.c
> new file mode 100644
> index 0000000000000..52db62784adda
> --- /dev/null
> +++ b/drivers/nvmem/mt6323-efuse.c
> @@ -0,0 +1,86 @@
[ ... ]
> +static int mt6323_efuse_read(void *context, unsigned int offset, void *val,
> +			     size_t bytes)
> +{
> +	struct regmap *map = context;
> +	u32 tmp;
> +	u16 *buf = val;
> +	int ret;
> +
> +	/*
> +	 * Manual regmap_read with loop is needed, because PWRAP is not
> +	 * a continuous MMIO space, but rather FSM which doesn't implement
> +	 * necessary read callback for the regmap_read_raw and regmap_read_bulk
> +	 * functions.
> +	 */
> +	for (size_t i = 0; i < bytes; i += sizeof(*buf)) {
> +		ret = regmap_read(map, MT6323_EFUSE_DOUT_BASE + offset + i, &tmp);
> +		if (ret)
> +			return ret;
> +
> +		*buf++ = (u16)tmp;

Does this assignment cause alignment faults or endianness corruption?

The val pointer passed by the NVMEM API is a generic void pointer. Casting it
to a u16 pointer and dereferencing it directly can trigger alignment faults on
architectures that do not support unaligned memory access.

Additionally, this writes the data in the CPU's native byte order, which
results in swapped bytes on big-endian architectures compared to little-endian
architectures. Downstream consumers might expect a consistent data layout.

Could this use put_unaligned_le16() instead to ensure safe memory access and
consistent byte ordering?

> +	}
> +
> +	return 0;
> +}
> +
> +static int mt6323_efuse_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct nvmem_config config = {
> +		.name = "mt6323-efuse",
> +		.stride = 2,
> +		.word_size = 2,
> +		.size = MT6323_EFUSE_SIZE,
> +		.reg_read = mt6323_efuse_read,
> +	};
> +	struct nvmem_device *nvmem;
> +	struct regmap *regmap;
> +
> +	/* efuse -> mfd -> pwrap */
> +	regmap = dev_get_regmap(dev->parent->parent, NULL);

Can dev->parent->parent evaluate to NULL and cause a NULL pointer dereference
here?

The driver registers an of_match_table, meaning it can be instantiated
directly from a Device Tree node. If a malformed or overlay Device Tree places
the node at the root level instead of within the expected PMIC/MFD hierarchy,
the device is instantiated by the top-level platform bus.

In that case, dev->parent is the platform bus, which has no parent, so
dev->parent->parent would be NULL. Passing a NULL device pointer to
dev_get_regmap() will cause an immediate NULL pointer dereference when
devres_find() attempts to acquire the lock.

Would it be safer to verify the existence of the expected parent hierarchy
before dereferencing it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512-mt6323-v2-0-3efcba579e88@protonmail.com?part=6