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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing unregister_pm_notifier() causes Use-After-Free of PM notifier block on probe error and driver unbind.
- [High] The IRQ domain is created with a hardcoded size of MT6397_IRQ_NR (32), which breaks PMICs like MT6328 that use a third interrupt register (hwirqs 32-47).
- [High] `mt6397_irq_pm_notifier` modifies IRQ mask registers concurrently with running tasks, causing race conditions.
--

commit 15d5fe2891c82970874377c126d0244fd87f5699
Author: Fabien Parent <parent.f@gmail.com>

mfd: mt6397: Add support for MT6392 PMIC

Align the MT6397 PMIC driver to other MFD drivers by passing only an
identifier through mt6397_of_match[*].data and add support for the MT6392
PMIC and its regulator, RTC, keys and pinctrl devices.

> 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) {
[ ... ]
> +	case MT6392_CHIP_ID:
> +		pmic_core = &mt6392_core;
> +		break;
[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but looking at the error path of
mt6397_probe() in drivers/mfd/mt6397-core.c:

	ret = pmic_core->irq_init(pmic);
	if (ret)
		return ret;

	ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
				   pmic_core->cells, pmic_core->cell_size,
				   NULL, 0, pmic->irq_domain);
	if (ret) {
		irq_domain_remove(pmic->irq_domain);
		dev_err(&pdev->dev, "failed to add child devices: %d\n", ret);
	}

	return ret;

Since mt6397_irq_init() registers a PM notifier with register_pm_notifier(),
if devm_mfd_add_devices() fails (or when the driver is unbound), the devres
cleanup will automatically free the pmic structure containing the pm_nb
notifier block.

Because unregister_pm_notifier() is never called in this error path or in a
remove callback, will this leave a dangling pointer in the PM subsystem that
causes a Use-After-Free during the next suspend/resume cycle?


> 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 further down in mt6397_irq_init() in
drivers/mfd/mt6397-irq.c, the linear IRQ domain is allocated with a hardcoded
size:

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

Since MT6397_IRQ_NR is 32, but PMICs like MT6328 use a third interrupt
status register for hardware IRQs 32-47:

	if (mt6397->int_status[2])
		mt6397_irq_handle_reg(mt6397, mt6397->int_status[2], 32);

Won't irq_find_mapping() silently fail to find a mapping for any interrupts
at index 32 or higher, breaking functionality for those higher-numbered
interrupts?


[Severity: High]
This is also a pre-existing issue, but mt6397_irq_pm_notifier() in
drivers/mfd/mt6397-irq.c writes directly to the PMIC's interrupt control
registers without locking:

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

Because PM_SUSPEND_PREPARE runs before user tasks are frozen, running tasks
can concurrently request or enable an IRQ. When this happens,
mt6397_irq_sync_unlock() will acquire irqlock and write irq_masks_cur
to the same registers:

	regmap_write(mt6397->regmap, mt6397->int_con[0],
		     mt6397->irq_masks_cur[0]);

Could this concurrent modification overwrite the suspend configuration
established by the notifier, leading to lost wakeup sources or spurious
interrupts?

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