Re: [PATCH v7 10/10] mfd: nct6694: Add Host Interface (HIF) eSPI transport driver

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Requesting a shared, oneshot threaded IRQ without a primary handler will be rejected by the IRQ core and degrade system performance if bypassed.
- [High] The IRQ handler is unregistered after the IRQ domain is destroyed, creating a window for a Use-After-Free of the domain pointer during teardown.
--

--- Patch [10]: [PATCH v7 10/10] mfd: nct6694: Add Host Interface (HIF) eSPI transport driver ---
commit fca991565f4ea259d4b5653b4e67f56e5a45bdd9
Author: Ming Yu <[email protected]>

mfd: nct6694: Add Host Interface (HIF) eSPI transport driver

This commit adds support for the Host Interface (HIF) transport via eSPI
for the Nuvoton NCT6694 peripheral expander. It initializes the Super-I/O
and provides a regmap_bus implementation.

> diff --git a/drivers/mfd/nct6694-hif.c b/drivers/mfd/nct6694-hif.c
> new file mode 100644
> index 0000000000000..198838d3e7d87
> --- /dev/null
> +++ b/drivers/mfd/nct6694-hif.c
[ ... ]
> @@ -546,12 +546,12 @@ static int nct6694_hif_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, nct6694);
>  
>  	/* Request the interrupt first so that devres releases it last */
>  	ret = devm_request_threaded_irq(dev, irq, NULL, nct6694_hif_irq_handler,
>  					IRQF_ONESHOT | IRQF_SHARED,
>  					dev_name(dev), nct6694);

[Severity: High]
Will this probe unconditionally fail with -EINVAL?

The IRQ subsystem typically rejects devm_request_threaded_irq() with
IRQF_SHARED | IRQF_ONESHOT if there is no primary handler, because it cannot
determine if the interrupt originated from this device before masking the
shared line.

[Severity: High]
Can this devres ordering lead to a use-after-free of nct6694->domain
during driver unbind?

Because the threaded IRQ is requested before the core is probed, devres will
release it last. During unbind, devres will execute actions in reverse order:

1. nct6694_hif_irq_disable() executes, masking the interrupt but not clearing
   pending status.
2. nct6694_hif_core_remove_action() executes, destroying the IRQ domain:

drivers/mfd/nct6694-core.c:nct6694_core_remove() {
    ...
    irq_domain_remove(nct6694->domain);
    ...
}

If another device sharing this IRQ line triggers an interrupt before devres
finally unregisters this handler, won't nct6694_hif_irq_handler() execute,
see a pending status, and dereference the freed nct6694->domain in
irq_find_mapping()?

>  	if (ret)
>  		return ret;
>  
>  	ret = nct6694_core_probe(dev, nct6694, nct6694_hif_devs,
>  				 ARRAY_SIZE(nct6694_hif_devs));
>  	if (ret)
>  		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10