Re: [PATCH v9 5/9] mfd: mt6397: Add support for MT6392 PMIC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Low] Driver-specific header files for MT6392 are placed in the global `include/linux/mfd/` directory.

Pre-existing issues:
- [High] Missing resource teardown for the PM notifier and IRQ domain on device unbind and probe error paths, leading to memory leaks and a Use-After-Free crash.
- [High] The IRQ domain is created with a hardcoded maximum size of 32, truncating the IRQ space for chips like MT6328 that require up to 47 IRQs.
- [High] Race condition on probe failure between irq_domain removal and active threaded IRQ handler.
- [High] The PM notifier modifies hardware IRQ registers without holding the irqlock mutex, potentially breaking system wakeup functionality.
--

--- Patch [5]: [PATCH v9 5/9] mfd: mt6397: Add support for MT6392 PMIC ---
commit b2217cbb3b951021ff7eb4a8b7e1f3c678b7c4bc
Author: Fabien Parent <parent.f@gmail.com>

mfd: mt6397: Add support for MT6392 PMIC

This commit modifies the MT6397 driver to use identifier matching instead
of passing core structures in the data field. It also introduces support
for the MT6392 PMIC, including its regulators, RTC, keys, and pinctrl.

> diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
> index ccd97d66d7f1c..f683e878543e1 100644
> --- a/drivers/mfd/mt6397-core.c
> +++ b/drivers/mfd/mt6397-core.c

[ ... ]

> @@ -278,6 +311,7 @@ static int mt6397_probe(struct platform_device *pdev)
>  	unsigned int id = 0;
>  	struct mt6397_chip *pmic;
>  	const struct chip_data *pmic_core;
> +	int chip_variant;
>  
>  	pmic = devm_kzalloc(&pdev->dev, sizeof(*pmic), GFP_KERNEL);
>  	if (!pmic)
> @@ -293,9 +327,36 @@ static int mt6397_probe(struct platform_device *pdev)
>  	if (!pmic->regmap)
>  		return -ENODEV;
>  
> -	pmic_core = of_device_get_match_data(&pdev->dev);
> -	if (!pmic_core)
> +	chip_variant = (unsigned int)(uintptr_t)device_get_match_data(&pdev->dev);
> +	switch (chip_variant) {

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there a use-after-free race on the
probe error path?

If devm_mfd_add_devices() fails further down in mt6397_probe(), the error
path calls irq_domain_remove(pmic->irq_domain) to explicitly free the
domain memory. However, the devm-managed threaded IRQ handler remains fully
active until the probe function returns. If an interrupt triggers during this
window, could mt6397_irq_thread() call irq_find_mapping() and dereference
the freed irq_domain pointer?

[Severity: High]
This is another pre-existing issue, but does the device unbind path leak
resources and cause a crash?

If devm_mfd_add_devices() fails, or if the driver is unbound, the PM
notifier registered in mt6397_irq_init() via register_pm_notifier() is
never explicitly unregistered because mt6397_driver lacks a .remove callback.
Since devres frees the mt6397_chip structure, would a subsequent suspend or
resume event traverse the global notifier list and dereference this freed
pointer?

> diff --git a/drivers/mfd/mt6397-irq.c b/drivers/mfd/mt6397-irq.c
> index 5d2e5459f7444..80ea5b92d232f 100644
> --- a/drivers/mfd/mt6397-irq.c
> +++ b/drivers/mfd/mt6397-irq.c
> @@ -203,6 +205,12 @@ int mt6397_irq_init(struct mt6397_chip *chip)
>  		chip->int_status[0] = MT6397_INT_STATUS0;
>  		chip->int_status[1] = MT6397_INT_STATUS1;
>  		break;
> +	case MT6392_CHIP_ID:
> +		chip->int_con[0] = MT6392_INT_CON0;
> +		chip->int_con[1] = MT6392_INT_CON1;
> +		chip->int_status[0] = MT6392_INT_STATUS0;
> +		chip->int_status[1] = MT6392_INT_STATUS1;
> +		break;
>  
>  	default:
>  		dev_err(chip->dev, "unsupported chip: 0x%x\n", chip->chip_id);

[Severity: High]
This is a pre-existing issue, but does the hardcoded domain size cause
interrupts to be dropped?

Further down in mt6397_irq_init(), the IRQ domain is created unconditionally
with MT6397_IRQ_NR (which is 32):

    chip->irq_domain = irq_domain_create_linear(dev_fwnode(chip->dev),
                                                MT6397_IRQ_NR,
                                                &mt6397_irq_domain_ops,
                                                chip);

Since the MT6328 chip requires up to 47 IRQs, could mt6397_irq_thread()
fail when calling irq_find_mapping() for interrupts exceeding the allocated
linear domain size of 32?

[Severity: High]
This is also a pre-existing issue, but does the PM notifier overwrite
wake masks without synchronization?

The mt6397_irq_pm_notifier() writes to the hardware interrupt control
registers to set wake masks during PM_SUSPEND_PREPARE without acquiring
chip->irqlock:

    case PM_SUSPEND_PREPARE:
        regmap_write(chip->regmap, chip->int_con[0], chip->wake_mask[0]);

If a child device driver alters an IRQ state during its own suspend phase,
could the bus unlock callback mt6397_irq_sync_unlock() concurrently write
chip->irq_masks_cur to the same registers and overwrite the configured
wake masks?

> diff --git a/include/linux/mfd/mt6392/core.h b/include/linux/mfd/mt6392/core.h
> new file mode 100644
> index 0000000000000..8777b3abf9292
> --- /dev/null
> +++ b/include/linux/mfd/mt6392/core.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2020 MediaTek Inc.
> + * Copyright (c) 2026 Luca Leonardo Scorcia <l.scorcia@gmail.com>
> + * Author: Chen Zhong <chen.zhong@mediatek.com>
> + */
> +
> +#ifndef __MFD_MT6392_CORE_H__
> +#define __MFD_MT6392_CORE_H__

[Severity: Low]
Are these driver-specific header files placed correctly? The MFD subsystem
guidelines state that driver-specific headers should not be placed in the
global include/linux/mfd/ directory if they are only used by the parent
and immediate children. Should these instead be kept local to drivers/mfd/?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820122242.120039-1-l.scorcia@gmail.com?part=5