Re: [PATCH v5 2/3] platform: arm64: dell-xps-ec: new driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] platform: dell-xps-ec: interrupt storm caused by returning IRQ_HANDLED without clearing HW state
- [High] platform: dell-xps-ec: transient I2C failure during resume permanently disables thermal polling
- [Medium] platform: dell-xps-ec: mismatch between static thermistor profile and runtime update IDs
--

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

platform: arm64: dell-xps-ec: new driver

Introduces a new embedded controller driver for the Dell XPS 13 9345.
The primary function of this driver is to unblock the EC's thermal
management by feeding it temperature information over an I2C interface.
This enables the EC to control device fans and peripheral power.

> diff --git a/drivers/platform/arm64/dell-xps-ec.c b/drivers/platform/arm64/dell-xps-ec.c
> new file mode 100644
> index 0000000000000..cf2dd41b1aa68
> --- /dev/null
> +++ b/drivers/platform/arm64/dell-xps-ec.c

[ ... ]

> +static const u8 dell_xps13_9345_thermistor_profile[] = {
> +	0xff, 0x54,
> +	0x01, 0x00, 0x2b,	/* sys_therm0 */
> +	0x02, 0x44, 0x2a,	/* sys_therm1 */
> +	0x03, 0x44, 0x2b,	/* sys_therm2 */
> +	0x04, 0x44, 0x28,	/* sys_therm3 */
> +	0x05, 0x55, 0x2a,	/* sys_therm4 */
> +	0x06, 0x44, 0x26,	/* sys_therm5 */
> +	0x07, 0x44, 0x2b,	/* sys_therm6 */
> +};
> +
> +static const struct dell_xps_ec_therm dell_xps13_9345_therms[] = {
> +	/* TODO: 0x01 is sent only occasionally, likely TZ98 or TZ4. */
> +	{ "sys_therm0", 0x02 },
> +	{ "sys_therm1", 0x03 },

[Severity: Medium]
Will this mismatch between the initialization profile and the runtime mapping
cause issues for the EC?

The static profile initializes the first thermistor (sys_therm0) as ID 0x01,
but the runtime mapping array expects sys_therm0 to be updated using ID 0x02.

This off-by-one discrepancy appears to cause the driver to send periodic
temperature updates for sys_therm6 to ID 0x08, which was never initialized,
while leaving ID 0x01 without any temperature updates.

[ ... ]

> +static irqreturn_t dell_xps_ec_irq_handler(int irq, void *data)
> +{
> +	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;
> +}

[Severity: Critical]
Can this cause an interrupt storm?

The threaded handler unconditionally logs a debug message and returns
IRQ_HANDLED, but performs no I2C operations to acknowledge or clear the
event on the EC.

Since the interrupt is level-triggered active-low, unmasking the line
without clearing the hardware state will cause the interrupt to immediately
fire again. This could completely consume a CPU core and hang or severely
degrade the system.

[ ... ]

> +static int dell_xps_ec_resume(struct device *dev)
> +{
> +	struct dell_xps_ec *ec = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = dell_xps_ec_suspend_cmd(ec, false);
> +	if (ret)
> +		return ret;
> +
> +	schedule_delayed_work(&ec->temp_work, DELL_XPS_EC_TEMP_POLL_JIFFIES);
> +
> +	return 0;
> +}

[Severity: High]
If the I2C transaction fails during resume, will this permanently disable
thermal polling?

The early return bypasses re-arming the periodic work in ec->temp_work.

The PM core ignores errors returned from resume callbacks, so the system
wakes up successfully, but the cyclic temperature polling timer remains
disabled for the rest of the session. Should the delayed work be scheduled
even if the suspend command fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011-dell-xps-9345-ec-v5-0-2ab1258b84c1@vinarskis.com?part=2