Re: [PATCH v6 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:
- [High] platform/arm64: dell-xps-ec: interrupt storm due to un-cleared level IRQ
- [Medium] pm: early return in resume on I2C failure permanently disables thermal polling
- [Medium] platform: off-by-one mismatch between thermistor profile IDs and update IDs
--

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

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

Introduce EC driver for Dell XPS 13 9345 (codename 'tributo') which may
partially of fully compatible with Snapdragon-based Dell Latitude,
Inspiron ('thena'). Primary function of this driver is unblock EC's
thermal management, specifically to provide it with necessary
information to control device fans, peripherals 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
> @@ -0,0 +1,308 @@
[ ... ]
> +/*
> + * Format:
> + * - header/unknown (2 bytes)
> + * - per-thermistor entries (3 bytes): thermistor_id, param1, param2
> + */
> +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 },
> +	{ "sys_therm2", 0x04 },
> +	{ "sys_therm3", 0x05 },
> +	{ "sys_therm4", 0x06 },
> +	{ "sys_therm5", 0x07 },
> +	{ "sys_therm6", 0x08 },
> +};

[Severity: Medium]
Does this thermistor ID mapping mismatch the profile?

The dell_xps13_9345_thermistor_profile array configures 7 thermistor IDs from
0x01 through 0x07 (with comments sys_therm0 through sys_therm6). However, the
dell_xps13_9345_therms array maps sys_therm0 through sys_therm6 to command
bytes 0x02 through 0x08.

This misalignment means that the thermal polling loop sends temperature updates
to ID 0x08 (which is completely unconfigured in the EC profile) and fails to
send updates to ID 0x01 (which is explicitly configured in the profile). Will
this configuration mismatch result in incorrect or missing thermal data being
fed to the EC?

[ ... ]
> +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: High]
Will this dummy IRQ handler cause an interrupt storm?

The driver requests an EC interrupt which is defined as IRQ_TYPE_LEVEL_LOW in
the device tree (commit 8c751badf45b). The threaded IRQ handler simply logs a
debug message and unconditionally returns IRQ_HANDLED without issuing any I2C
commands to acknowledge or clear the event on the EC hardware.

Because the interrupt is requested with IRQF_ONESHOT in dell_xps_ec_probe(),
the interrupt is unmasked after the threaded handler returns. Since the
level-triggered interrupt is never cleared on the EC, it will remain asserted
and immediately fire again, potentially causing an infinite interrupt storm
that can consume 100% of a CPU core.

[ ... ]
> +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;

[Severity: Medium]
Does this early return permanently disable thermal polling if the EC is slow
to wake?

During system resume, if dell_xps_ec_suspend_cmd() fails (e.g., due to a
transient bus error or the EC taking longer to wake up), the function returns
early without calling schedule_delayed_work().

Because the PM core largely ignores individual device resume errors and
continues bringing the system up, the device will remain in the 'resumed'
state, but the periodic temperature polling workqueue will be permanently
stopped. Will this starve the EC of the thermal updates required to spin the
fans, potentially leading to SoC overheating?

> +
> +	schedule_delayed_work(&ec->temp_work, DELL_XPS_EC_TEMP_POLL_JIFFIES);
> +
> +	return 0;
> +}

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