Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: qcom: add msm8660-mpm

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing memory barrier and doorbell notification to RPM after clearing pending interrupts.
- [Medium] The `qcom,mpm-pin-map` property is required by the driver but missing from the YAML schema's required list, while `qcom,mpm-pin-count` is strictly required by the schema but ignored by the driver.
- [Medium] The device tree binding defines a `reg` property that explicitly overlaps with the RPM control block, an MFD/syscon architectural anti-pattern.
- [Medium] The binding and driver omit hardware wake pins from the DT mapping, relying instead on a custom driver C API.
--

commit ff9b33b822acb8b5cceef507bbe942c3ff3618b7
Author: Herman van Hazendonk <github.com@herrie.org>

dt-bindings: interrupt-controller: qcom: add msm8660-mpm

Add the binding for the Modem Power Manager (MPM) interrupt
controller on the MSM8x60 family (MSM8260/MSM8660/APQ8060). The MPM
is a small wake-capable interrupt aggregator that lets the SoC stay
in low-power states while a small set of GIC SPIs continues to be
monitored and can wake the system on an edge.

> diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,msm8660-mpm.yaml b/Documentation/devicetree/bindings/interrupt-controller/qcom,msm8660-mpm.yaml
> new file mode 100644
> index 0000000000000..1288e83d75d6b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,msm8660-mpm.yaml

[ ... ]

> +properties:
> +  compatible:
> +    const: qcom,msm8660-mpm
> +
> +  reg:
> +    maxItems: 1
> +    description: |
> +      vMPM register window inside the RPM control block. Must cover
> +      the request registers at offset 0 of the window and the status
> +      registers at offset 0x420 of the window. A length of 0x440 is
> +      sufficient for the four-word request banks and the status bank.

[Severity: Medium]
Does this explicit overlap violate standard Device Tree architecture?

Overlapping hardware resources should typically be modeled as child nodes of
an MFD sharing a regmap, or accessed via a syscon phandle. By directly using
devm_ioremap() in the driver to bypass standard resource reservation, this
introduces an architectural anti-pattern:

drivers/irqchip/irq-msm8660-mpm.c:msm8660_mpm_probe() {
	/*
	 * Use a non-exclusive mapping: the qcom,rpm driver maps the
	 * surrounding RPM control block via its own platform resource,
	 * and our vMPM sub-region overlaps that mapping. devm_ioremap()
	 * does not call request_mem_region() so there is no conflict.
	 */
	mpm->base = devm_ioremap(dev, res->start, resource_size(res));
}

[ ... ]

> +  interrupts:
> +    maxItems: 1
> +    description:
> +      IPC IRQ raised by MPM when one of the enabled wake sources fires.
> +      On MSM8x60 this is wired to GIC SPI 2.

[Severity: High]
Does the interrupt handler in the driver lack a memory barrier and doorbell
notification after clearing pending interrupts?

Looking at drivers/irqchip/irq-msm8660-mpm.c:msm8660_mpm_irq(), the CLEAR
register is written using a relaxed IO accessor:

drivers/irqchip/irq-msm8660-mpm.c:msm8660_mpm_irq() {
...
		if (pending[i])
			msm8660_mpm_write(mpm,
				MSM8660_MPM_REG_CLEAR + i * 4, pending[i]);
...
}

Since the RPM caches the vMPM request registers, won't it miss the CLEAR
command without a doorbell notification, causing it to continue asserting the
IPC wake interrupt?

Also, without a memory barrier, this might reorder against the subsequent
parent GIC EOI. Could this cause an immediate spurious interrupt storm since
the parent IPC interrupt is level-triggered?

[ ... ]

> +  qcom,mpm-pin-map:
> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +    items:
> +      items:
> +        - description: MPM pin number
> +        - description: GIC SPI number this pin maps to
> +    description: |
> +      List of (MPM-pin, GIC-SPI) tuples for wake sources that have a
> +      corresponding GIC IRQ. Consumers route their interrupts through
> +      the MPM hierarchical irqdomain to take advantage of these
> +      mappings.
> +
> +      Raw wake pins like SDC3/4 DATx (pins 21-24) are NOT listed
> +      here - they have no GIC SPI mapping and are accessed through
> +      the msm8660_mpm_set_pin_wake() / msm8660_mpm_enable_pin() C API
> +      by consumer drivers.

[Severity: Medium]
Is it correct to omit hardware wake pins from the device tree mapping and rely
on a custom C API?

Device tree bindings should describe hardware resources completely and
independently of OS implementation. Omitting pins just because they don't map
to a GIC SPI forces consumers to be tightly coupled to this specific driver
implementation via C symbol exports:

drivers/irqchip/irq-msm8660-mpm.c:msm8660_mpm_enable_pin()

Would it be better to list these pins and avoid bypassing standard kernel
subsystems like dummy IRQ domains or pinctrl frameworks?

[ ... ]

> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - mboxes
> +  - interrupt-controller
> +  - '#interrupt-cells'
> +  - qcom,mpm-pin-count

[Severity: Medium]
Are the required properties synchronized with the driver's expectations?

The schema enforces qcom,mpm-pin-count, but the driver ignores this property
and hardcodes the pin count to 64:

drivers/irqchip/irq-msm8660-mpm.c:
#define MSM8660_MPM_PIN_COUNT		64

Conversely, the driver's probe unconditionally fails if qcom,mpm-pin-map is
missing, returning -EINVAL:

drivers/irqchip/irq-msm8660-mpm.c:msm8660_mpm_probe() {
...
	ret = of_property_count_u32_elems(np, "qcom,mpm-pin-map");
	if (ret < 0 || ret % 2)
		return dev_err_probe(dev, -EINVAL,
				     "invalid qcom,mpm-pin-map\n");
...
}

Should qcom,mpm-pin-map be added to the required list to prevent invalid device
trees from silently passing schema validation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1780195817.git.github.com@herrie.org?part=1