Re: [PATCH v1 1/3] drivers/platform: lenovo-t14s-ec: Add hwmon support for temperatures and fan speed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Direct calls to `t14s_ec_read` bypass the `regmap` mutex, violating the mandatory 10ms hardware recovery delay between EC commands.
- [Medium] Reading the fan RPM requires two separate I2C transfers for the LSB and MSB, allowing the hardware value to update in between and cause torn reads.
- [Low] Passing `int *` arguments to a function expecting `unsigned int *` for I2C register reads.

Pre-existing issues:
- [High] The `t14s_ec_read` function passes buffers allocated on the kernel stack to `__i2c_transfer()`, which violates DMA mapping rules and can cause panics.
--

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

drivers/platform: lenovo-t14s-ec: Add hwmon support for temperatures and fan speed

Expose the Lenovo ThinkPad T14s EC environmental sensors through
the hwmon subsystem.

> diff --git a/drivers/platform/arm64/lenovo-thinkpad-t14s.c b/drivers/platform/arm64/lenovo-thinkpad-t14s.c
> index 5590302a5694f..142464623f0e8 100644
> --- a/drivers/platform/arm64/lenovo-thinkpad-t14s.c
> +++ b/drivers/platform/arm64/lenovo-thinkpad-t14s.c

[ ... ]

> @@ -555,6 +576,128 @@ static irqreturn_t t14s_ec_irq_handler(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static umode_t t14s_ec_hwmon_is_visible(const void *drvdata,

[ ... ]

> +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 mutex?

Because the embedded controller requires a 10ms hardware recovery delay between
commands, regmap serializes accesses and includes the sleep while holding the
lock. Calling t14s_ec_read() directly appears to bypass this locking, potentially
allowing concurrent sysfs reads to interleave with other subsystem I2C
transfers and violate the hardware constraint.

[Severity: High]
This isn't a bug introduced by this patch, but because this exposes new sysfs
attributes that userspace can trigger, does t14s_ec_read() pass stack-allocated
buffers to __i2c_transfer()?

Looking at t14s_ec_read():

static int t14s_ec_read(void *context, unsigned int reg,
                        unsigned int *val)
{
        ...
        u8 buf[4] = {T14S_EC_CMD_ECRD, reg, 0x00, 0x01};
        struct i2c_msg request, response;
        u8 result;
        ...
        request.buf = buf;
        ...
        response.buf = &result;
        ...
        ret = __i2c_transfer(client->adapter, &request, 1);

On ARM64 architectures with CONFIG_VMAP_STACK, passing stack memory to I2C
drivers using DMA (like Qualcomm GENI) can trigger a BUG() panic or corrupt
adjacent physical memory. Is it possible to use heap-allocated buffers here
instead?

> +			if (ret)
> +				return ret;
> +			*val = value * 1000;
> +
> +			return 0;
> +		}
> +		break;
> +
> +	case hwmon_fan:
> +		if (attr == hwmon_fan_input) {
> +			int lsb, msb;
> +			ret = t14s_ec_read(ec, T14S_EC_FAN_RPM_LSB, &lsb);

[Severity: Low]
Does this pass pointers of type int * to t14s_ec_read() which expects
unsigned int *?

Newer compilers like GCC 14 treat -Wincompatible-pointer-types as an
error by default, which might break the build here.

[Severity: Medium]
Is there a risk of torn reads when reading the lsb and msb sequentially?

Because t14s_ec_read() enforces a 10ms sleep after each I2C transfer, there is
a large race window between reading T14S_EC_FAN_RPM_LSB and 
T14S_EC_FAN_RPM_MSB. If the fan RPM rolls over (e.g., from 0x01ff to 0x0200)
during that window, could we combine an old lsb with a new msb and report
a wildly incorrect fan speed?

> +			if (ret)
> +				return ret;
> +
> +			ret = t14s_ec_read(ec, T14S_EC_FAN_RPM_MSB, &msb);
> +			if (ret)
> +				return ret;
> +
> +			*val = 0;
> +			*val = lsb + (msb << 8);
> +
> +			return 0;
> +		}
> +		break;

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