Re: [PATCH v7 3/3] arm64: dts: qcom: x1e80100-dell-xps13-9345: introduce EC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] arm64: dts: qcom: level-low EC IRQ with stub dell_xps_ec_irq_handler()
--

commit bdc3996f914e2f3bd3df5f76f45237ab26fd449b
Author: Aleksandrs Vinarskis <alex@vinarskis.com>

arm64: dts: qcom: x1e80100-dell-xps13-9345: introduce EC

This commit enables i2c5 and adds the embedded controller node along with its
interrupt and thermal ADC channels for the Dell XPS 13 9345. It also adds
labels for the known PMK8550 VADC thermal channels.

> diff --git a/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts b/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts
> index f48e6bc350063..3f0c17c845b3e 100644
> --- a/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts
> +++ b/arch/arm64/boot/dts/qcom/x1e80100-dell-xps13-9345.dts
> @@ -759,8 +759,32 @@ retimer_ss0_con_sbu_out: endpoint {
>  
>  &i2c5 {
>  	clock-frequency = <100000>;
> -	status = "disabled";
> -	/* EC @0x3b */
> +	status = "okay";
> +
> +	embedded-controller@3b {
> +		compatible = "dell,xps13-9345-ec";
> +		reg = <0x3b>;
> +
> +		interrupts-extended = <&tlmm 66 IRQ_TYPE_LEVEL_LOW>;

[Severity: High]
Can configuring this interrupt as IRQ_TYPE_LEVEL_LOW cause an unrecoverable
interrupt storm when the EC asserts gpio66 low (such as on lid close)?

In drivers/platform/arm64/dell-xps-ec.c, dell_xps_ec_probe() requests and
enables client->irq as a oneshot threaded interrupt:

drivers/platform/arm64/dell-xps-ec.c:dell_xps_ec_probe() {
	...
	/* Request IRQ for EC events */
	ret = devm_request_threaded_irq(dev, client->irq, NULL,
					dell_xps_ec_irq_handler,
					IRQF_ONESHOT, dev_name(dev), ec);
	if (ret)
		return dev_err_probe(dev, ret, "Failed to request IRQ\n");
}

However, dell_xps_ec_irq_handler() is currently a stub that logs a debug
message and returns IRQ_HANDLED without performing an I2C read to acknowledge
or clear the pending event on the EC:

drivers/platform/arm64/dell-xps-ec.c:dell_xps_ec_irq_handler() {
	struct dell_xps_ec *ec = data;

	/*
	 * TODO: IRQ is fired on lid-close. Follow Windows example to read out
	 *       the thermistor thresholds and potentially fan speeds.
	 */
	dev_dbg(ec->dev, "IRQ triggered (irq=%d)\n", irq);

	return IRQ_HANDLED;
}

Because the interrupt is level-triggered (IRQ_TYPE_LEVEL_LOW) with
IRQF_ONESHOT, irq_finalize_oneshot() unmasks the line as soon as
dell_xps_ec_irq_handler() returns. If the EC continues holding gpio66 low
because the event was not cleared over I2C, the interrupt will immediately
re-fire in a tight loop.

In addition, because dell_xps_ec_irq_handler() returns IRQ_HANDLED rather
than IRQ_NONE, note_interrupt() in kernel/irq/spurious.c never increments
desc->irqs_unhandled, preventing the spurious IRQ detector from disabling the
interrupt line.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-dell-xps-9345-ec-v7-0-828ce301b839@vinarskis.com?part=3