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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] If the I2C suspend command fails during suspend or resume, the temperature polling delayed_work is permanently halted, leaving the EC without thermal updates.
- [High] The threaded IRQ handler returns IRQ_HANDLED without issuing commands to clear the interrupt source on the EC, leading to an interrupt storm.
- [High] Mismatch between thermistor IDs initialized in the profile and command bytes used for thermal updates.
- [Medium] Hardware state is modified before all software resources are acquired, leading to inconsistent EC state if probe defers.
- [Low] The i2c_device_id array does not use named initializers, violating I2C subsystem guidelines.
--

--- Patch [2]: [PATCH v4 2/3] platform: arm64: dell-xps-ec: new driver ---
commit 4cf5509cb5dd3a2ed8d4226af2e175536980f3e7
Author: Aleksandrs Vinarskis <alex@vinarskis.com>

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

This commit introduces a new Embedded Controller driver for the Dell XPS 13
9345. It handles thermal management by periodically polling thermistors via
IIO channels and sending the readings to the EC using I2C commands to
control fan speeds 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
> --- /dev/null
> +++ b/drivers/platform/arm64/dell-xps-ec.c
> @@ -0,0 +1,299 @@

[ ... ]

> +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: High]
Is there a mismatch between the thermistor IDs initialized in the profile and
the command bytes used for thermal updates?

The profile array initializes thermistors 0x01 through 0x07, but the command
mapping array maps sys_therm0 to 0x02 and goes up to 0x08. Could this cause
thermal updates for ID 0x01 to be completely missed, while sending updates
for the uninitialized ID 0x08?

[ ... ]

> +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_info_ratelimited(ec->dev, "IRQ triggered! (irq=%d)\n", irq);
> +
> +	return IRQ_HANDLED;
> +}

[Severity: High]
Does this handler properly clear the hardware interrupt source?

Returning IRQ_HANDLED without issuing I2C commands to acknowledge the
interrupt on the EC side could result in an interrupt storm if the IRQ is
level-triggered. The core will unmask the line on return, and the hardware
might immediately fire the interrupt again.

[ ... ]

> +static int dell_xps_ec_probe(struct i2c_client *client)
> +{
> +	const struct dell_xps_ec_data *data;
> +	struct device *dev = &client->dev;
> +	struct dell_xps_ec *ec;
> +	unsigned int i;
> +	int ret;

[ ... ]

> +	/* Set default thermistor profile */
> +	ret = i2c_master_send(client, data->profile, data->profile_len);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to set thermistor profile\n");
> +
> +	/* Get IIO channels for thermistors */
> +	for (i = 0; i < ec->num_therms; i++) {
> +		ec->therm_channels[i] =
> +			devm_iio_channel_get(dev, data->therms[i].name);
> +		if (IS_ERR(ec->therm_channels[i])) {
> +			ret = PTR_ERR(ec->therm_channels[i]);
> +			ec->therm_channels[i] = NULL;
> +			if (ret == -EPROBE_DEFER)
> +				return ret;

[Severity: Medium]
Is it safe to modify the EC hardware state before fully acquiring all IIO
channels?

If devm_iio_channel_get() returns -EPROBE_DEFER, the probe will abort and be
retried later. This leaves the EC initialized with a new profile but waiting
for thermal updates that will not arrive until the driver successfully probes.

[ ... ]

> +static int dell_xps_ec_suspend(struct device *dev)
> +{
> +	struct dell_xps_ec *ec = dev_get_drvdata(dev);
> +
> +	cancel_delayed_work_sync(&ec->temp_work);
> +
> +	return dell_xps_ec_suspend_cmd(ec, true);
> +}

[Severity: High]
If dell_xps_ec_suspend_cmd() fails and returns an error, the suspend will be
aborted by the PM core, but the delayed work is already cancelled and never
restarted.

Could this permanently halt the thermal polling loop if the I2C transfer
encounters a temporary error?

> +
> +/*
> + * Notify EC of resume
> + *
> + * This will undo the suspend actions
> + * Without the resume signal, device would wake up but be forced back into
> + * suspend by EC within seconds
> + */
> +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]
Similarly, during resume, if dell_xps_ec_suspend_cmd() fails, it returns early
without scheduling the delayed work.

Does this completely stop thermal updates to the EC after a failed resume I2C
transaction?

[ ... ]

> +static const struct i2c_device_id dell_xps_ec_i2c_id[] = {
> +	{ "dell-xps-ec", (kernel_ulong_t)&dell_xps13_9345_data },
> +	{}
> +};

[Severity: Low]
Should this i2c_device_id array use named initializers?

The I2C subsystem guidelines state that initialized arrays of type
struct i2c_device_id must be declared const and use named initializers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260809-dell-xps-9345-ec-v4-0-88306dbbc7aa@vinarskis.com?part=2