Re: [PATCH v2 2/3] dt-bindings: mfd: st,stmpe: let interrupt property optional

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The commit message description is not capitalized, violating MFD subsystem guidelines.

Pre-existing issues:
- [High] The driver explicitly breaks `interrupts-extended` DT bindings by manually checking for the literal "interrupts" string and overriding the valid IRQ with -1 if missing.
- [High] Global static structures `i2c_ci` and `spi_ci` are mutated concurrently during probe, leading to data races if multiple devices probe simultaneously.
- [High] Memory and resource leaks exist on error paths in `stmpe_probe` for manually enabled regulators and the dynamically created IRQ domain, leading to a Use-After-Free.
- [High] The driver modifies global shared `mfd_cell` resource arrays at runtime by casting away `const`, leading to race conditions and corrupted IRQ assignments.
--

commit 5a672301bc93caad7ec80c8b4ff3ebf32574e530
Author: Frank Li <Frank.Li@nxp.com>
Subject: dt-bindings: mfd: st,stmpe: let interrupt property optional

This commit updates the device tree binding for the st,stmpe MFD to make
the interrupts property optional. It addresses configurations where not all
boards connect IRQ lines, fixing validation warnings reported by dtbs_check.

> dt-bindings: mfd: st,stmpe: let interrupt property optional

[Severity: Low]
Is the capitalization correct here? MFD subsystem guidelines require
capitalizing the description after the subsystem prefix.

> diff --git a/Documentation/devicetree/bindings/mfd/st,stmpe.yaml b/Documentation/devicetree/bindings/mfd/st,stmpe.yaml
> index 592e8f17659f3..6fde068b526a0 100644
> --- a/Documentation/devicetree/bindings/mfd/st,stmpe.yaml
> +++ b/Documentation/devicetree/bindings/mfd/st,stmpe.yaml
[ ... ]
> @@ -225,7 +225,6 @@ additionalProperties: false
>  required:
>    - compatible
>    - reg
> -  - interrupts
>  
>  examples:
>    - |

While reviewing the interrupt changes, I noticed a few pre-existing
issues in the stmpe driver that might need addressing.

In drivers/mfd/stmpe.c:stmpe_probe():

	stmpe_of_probe(pdata, np);

	if (!of_property_present(np, "interrupts"))
		ci->irq = -1;

	stmpe = devm_kzalloc(ci->dev, sizeof(struct stmpe), GFP_KERNEL);

[Severity: High]
This is a pre-existing issue, but does this logic break the standard
interrupts-extended DT binding? 

If a device tree uses interrupts-extended instead of interrupts, it seems
of_property_present() will erroneously clobber the valid ci->irq to -1, causing
the probe to fail with -ENODEV or fallback to polling.

Also in drivers/mfd/stmpe.c:stmpe_probe():

	if (stmpe->irq >= 0) {
		ret = stmpe_irq_init(stmpe, np);
		if (ret)
			return ret;

		ret = devm_request_threaded_irq(ci->dev, stmpe->irq, NULL,
				stmpe_irq, pdata->irq_trigger | IRQF_ONESHOT,
				"stmpe", stmpe);
		if (ret) {
			dev_err(stmpe->dev, "failed to request IRQ: %d\n",
					ret);
			return ret;
		}
	}

	ret = stmpe_devices_init(stmpe);
	if (!ret)
		return 0;

	dev_err(stmpe->dev, "failed to add children\n");
	mfd_remove_devices(stmpe->dev);

	return ret;

[Severity: High]
This is a pre-existing issue, but are the dynamically created irq_domain
and enabled regulators leaked on these error paths? 

If probe fails after stmpe_irq_init(), returning directly without cleanup
might leave the leaked irq_domain host_data pointing to the devm-freed
stmpe struct, potentially creating a use-after-free condition.

In drivers/mfd/stmpe-i2c.c:stmpe_i2c_probe():

	i2c_ci.data = (void *)id;
	i2c_ci.irq = i2c->irq;
	i2c_ci.client = i2c;
	i2c_ci.dev = &i2c->dev;

And similarly in drivers/mfd/stmpe-spi.c:stmpe_spi_probe():

	spi_ci.irq = spi->irq;
	spi_ci.client = spi;
	spi_ci.dev = &spi->dev;

[Severity: High]
This is a pre-existing issue, but could mutating the global static structures
i2c_ci and spi_ci concurrently during probe cause data races? 

If multiple STMPE devices probe simultaneously, operations like transfers
might be routed to the wrong device or freed on the wrong unbind due to the
dev and client pointers pointing to the incorrect instance.

Finally, in drivers/mfd/stmpe.c:stmpe_devices_init():

		for (j = 0; j < block->cell->num_resources; j++) {
			struct resource *res =
				(struct resource *) &block->cell->resources[j];

			/* Dynamically fill in a variant's IRQ. */
			if (res->flags & IORESOURCE_IRQ)
				res->start = res->end = block->irq + j;
		}

[Severity: High]
This is a pre-existing issue, but is it safe to cast away the const qualifier
on the global mfd_cell resource arrays here? 

Dynamically modifying this shared state could allow concurrent IRQ writes from
multiple probing instances to overwrite each other, potentially corrupting
IRQ assignments across the system.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-dts-stmpe-v2-0-f93d5f080443@nxp.com?part=2