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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free of PM notifier on device unbind or probe failure
- [High] Use-After-Free of irq_domain in probe error path due to active IRQ thread
- [High] Concurrent hardware state writes in PM notifier
- [High] Lost interrupts due to late Write-1-to-Clear (W1C) in the threaded IRQ handler
--

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

mfd: mt6397: Add support for MT6392 PMIC

This commit aligns the MT6397 PMIC driver to other MFD drivers by passing
an identifier through mt6397_of_match and adds support for the MT6392
PMIC variant along with 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
[ ... ]
> @@ -327,30 +388,15 @@ static int mt6397_probe(struct platform_device *pdev)
>  }

[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free
with the PM notifier during probe failure or device unbind.

In mt6397_irq_init(), the pm_nb notifier is registered globally:

mt6397_irq_init() {
    ...
    register_pm_notifier(&chip->pm_nb);
    return 0;
}

However, there doesn't seem to be a corresponding unregister action if
devm_mfd_add_devices() fails in mt6397_probe(), or when the driver is
unbound (since mt6397_driver lacks a .remove callback). Because the chip
struct is devm-allocated, could this leave a dangling pointer in the
global pm_chain_head, resulting in a use-after-free during the next
suspend/resume cycle?

[Severity: High]
This isn't a bug introduced by this patch, but is there a risk of a
use-after-free involving the irq_domain in the probe error path?

In mt6397_probe(), if devm_mfd_add_devices() fails, the domain is
manually removed:

mt6397_probe() {
    ...
    if (ret) {
        irq_domain_remove(pmic->irq_domain);
        dev_err(&pdev->dev, "failed to add child devices: %d\n", ret);
    }
}

Since the threaded IRQ handler was requested earlier using
devm_request_threaded_irq(), it remains active until the probe function
returns and devres unwinds. If a pending hardware interrupt fires during
this window, would mt6397_irq_thread() execute and call irq_find_mapping()
on the already freed pmic->irq_domain?

>  static const struct of_device_id mt6397_of_match[] = {
[ ... ]
> 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 isn't a bug introduced by this patch, but does the PM notifier
need to acquire the irqlock to prevent hardware state corruption?

In mt6397_irq_pm_notifier(), during PM_SUSPEND_PREPARE:

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

This directly accesses chip->wake_mask and writes to the regmap without
acquiring chip->irqlock. Since PM notifiers run while userspace tasks
are still active, could a concurrent request_irq() or free_irq()
interleave, causing the PM notifier to read torn state or overwrite
hardware registers with stale values?

[Severity: High]
This is a pre-existing issue, but can this interrupt handling sequence
cause lost edge interrupts?

In mt6397_irq_handle_reg(), the driver reads the status, handles the
nested irqs, and then clears the status at the end:

mt6397_irq_handle_reg() {
    ...
    for (i = 0; i < 16; i++) {
        if (status & BIT(i)) {
            irq = irq_find_mapping(mt6397->irq_domain, irqbase + i);
            if (irq)
                handle_nested_irq(irq);
        }
    }
    
    regmap_write(mt6397->regmap, reg, status);
}

Because these are Write-1-to-Clear registers, does writing the status
back after handling the interrupts unintentionally clear any new edge
interrupts that might have arrived while handle_nested_irq() was executing?

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