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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] platform: arm64: dell-xps-ec: level-low IRQ storm in dell_xps_ec_irq_handler()
- [Medium] platform: arm64: dell-xps-ec: temp_work not rescheduled on resume error
--

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

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

This commit introduces an I2C embedded controller driver for the Dell XPS
13 9345 to configure thermistor profiles, periodically feed IIO thermistor
temperature readings to the EC, and notify the EC across system sleep
transitions.

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

[ ... ]

> +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;
> +}
> +
> +static int dell_xps_ec_probe(struct i2c_client *client)
> +{

[ ... ]

> +	/* 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");
> +
> +	return 0;
> +}

[Severity: High]
Will requesting client->irq here cause an unmasked interrupt storm when the
EC asserts its interrupt line (for example, on lid close)?

In Documentation/devicetree/bindings/embedded-controller/dell,xps13-9345-ec.yaml,
the interrupt is specified as level-triggered active-low:

    interrupts-extended = <&tlmm 66 IRQ_TYPE_LEVEL_LOW>;

When the EC asserts the line, dell_xps_ec_irq_handler() logs a debug message
and returns IRQ_HANDLED without performing an I2C transfer to clear or
acknowledge the event on the EC. Once irq_finalize_oneshot() unmasks the line,
the level-low interrupt will immediately fire again, and returning IRQ_HANDLED
prevents note_interrupt() in kernel/irq/spurious.c from detecting and
disabling the stuck interrupt.

Should requesting the IRQ be deferred until the EC event readout is
implemented, or should the interrupt be left disabled for now?

> +
> +/*
> + * Notify EC of suspend
> + *
> + * This will:
> + * - Cut power to display/trackpad/keyboard/touchrow, wake-up source still works.
> + */
> +static int dell_xps_ec_suspend(struct device *dev)
> +{
> +	struct dell_xps_ec *ec = dev_get_drvdata(dev);
> +
> +	int ret;
> +
> +	cancel_delayed_work_sync(&ec->temp_work);
> +
> +	ret = dell_xps_ec_suspend_cmd(ec, true);
> +	if (ret) {
> +		schedule_delayed_work(&ec->temp_work,
> +				      DELL_XPS_EC_TEMP_POLL_JIFFIES);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * 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: Medium]
If dell_xps_ec_suspend_cmd(ec, false) fails in dell_xps_ec_resume(), does
returning early leave ec->temp_work permanently stopped after the system
resumes or thaws?

During suspend or hibernation freeze, dell_xps_ec_suspend() cancels
ec->temp_work via cancel_delayed_work_sync(&ec->temp_work). On resume, thaw,
or restore, the PM core logs errors from dell_xps_ec_resume() but continues
resuming the system without re-running the callback. Would it make sense to
still reschedule ec->temp_work even when dell_xps_ec_suspend_cmd() returns an
error?

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