Re: [PATCH v3 1/2] platform: arm64: lenovo-thinkpad-t14s-ec: Add hwmon support for temperatures

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] t14s_ec_hwmon_read() bypasses the regmap lock, allowing concurrent I2C commands to the EC without the required 10ms hardware delay.
- [Low] The driver unconditionally uses hwmon functions but its Kconfig entry does not depend on HWMON, leading to linker errors.
--

commit 53ea0adbdbb89073fdf3f9621385cdebffccdb5b
Author: Daniel Lezcano <daniel.lezcano@oss.qualcomm.com>

platform: arm64: lenovo-thinkpad-t14s-ec: Add hwmon support for temperatures

This commit exposes the Lenovo ThinkPad T14s EC environmental sensors through
the hwmon subsystem. It registers a hwmon device to provide access to six EC
temperature sensors and exports their labels.

> diff --git a/drivers/platform/arm64/lenovo-thinkpad-t14s.c b/drivers/platform/arm64/lenovo-thinkpad-t14s.c
> index 5590302a5694f..c9917a1d2bd7a 100644
> --- a/drivers/platform/arm64/lenovo-thinkpad-t14s.c
> +++ b/drivers/platform/arm64/lenovo-thinkpad-t14s.c
> @@ -11,6 +11,7 @@
>  #include <linux/delay.h>
>  #include <linux/dev_printk.h>
>  #include <linux/err.h>
> +#include <linux/hwmon.h>

[Severity: Low]
The driver now unconditionally uses hwmon functions like
devm_hwmon_device_register_with_info(), but the Kconfig definition for
EC_LENOVO_THINKPAD_T14S doesn't appear to depend on HWMON.

Will this cause linker errors if EC_LENOVO_THINKPAD_T14S is built-in while
HWMON is built as a module or disabled?

[ ... ]

> @@ -555,6 +574,113 @@ static irqreturn_t t14s_ec_irq_handler(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
[ ... ]
> +static int t14s_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +			      u32 attr, int channel, long *val)
> +{
> +	struct t14s_ec *ec = dev_get_drvdata(dev);
> +	unsigned int value;
> +	int ret;
> +
> +	switch (type) {
> +	case hwmon_temp:
> +		if (attr == hwmon_temp_input) {
> +			ret = t14s_ec_read(ec, ec->ec_hwmon.sys_thermx[channel].reg, &value);

[Severity: High]
Does calling t14s_ec_read() directly here bypass the regmap serialization?

Multiple subsystems (LEDs, keyboard, etc.) use regmap_write/read which serialize
access and respect the 10ms delay between EC I2C commands.

Since t14s_ec_read() drops the I2C bus lock before sleeping for 10ms:

drivers/platform/arm64/lenovo-thinkpad-t14s.c:t14s_ec_read() {
    ...
out:
    i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
    fsleep(10000);
    return ret;
}

could a concurrent regmap access acquire the I2C bus lock immediately, issuing
a new command to the EC before the hardware-mandated 10ms delay has elapsed?

> +			if (ret)
> +				return ret;
> +			*val = value * MILLIDEGREE_PER_DEGREE;
> +
> +			return 0;
> +		}
> +		break;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701103714.22583-1-daniel.lezcano@oss.qualcomm.com?part=1