Re: [PATCH 2/2] hwmon: (sht4x): Add support for Sensirion STS4x temperature sensors

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

New issues:
- [High] Uninitialized `heating_complete` timestamp causes sensor reads to block for up to 5 minutes on boot.

Pre-existing issues:
- [High] Lack of driver-level locking between hwmon core attributes and custom heater attributes leads to I2C race conditions and data corruption.
- [High] `heater_enable_store` incorrectly returns 0 on success, breaking standard userspace write operations.
- [High] Unit mismatch when evaluating timeout with `data->update_interval` leads to HZ-dependent I2C errors.
--

--- Patch [2]: [PATCH 2/2] hwmon: (sht4x): Add support for Sensirion STS4x temperature sensors ---
commit ab4b49384ea571fb866537cfb0dea5a4f4296ef8
Author: Alessandro Zini <[email protected]>

hwmon: (sht4x): Add support for Sensirion STS4x temperature sensors

The Sensirion STS4x series is the temperature-only variant of the
SHT4x family. Add support for the STS4x series by dynamically adjusting
the read response length, suppressing humidity channel attributes when probed
as STS4x, and omitting heater sysfs attributes.

Link: https://sensirion.com/resource/datasheet/sts4x

> diff --git a/drivers/hwmon/sht4x.c b/drivers/hwmon/sht4x.c
> index 9cace0e8acdab..d2b0d79bb7fe0 100644
> --- a/drivers/hwmon/sht4x.c
> +++ b/drivers/hwmon/sht4x.c
[ ... ]
> @@ -400,11 +420,15 @@ static int sht4x_probe(struct i2c_client *client)
>  	if (!data)
>  		return -ENOMEM;
>  
> +	data->chip_id = (uintptr_t)i2c_get_match_data(client);
>  	data->update_interval = SHT4X_MIN_POLL_INTERVAL;
>  	data->client = client;
> -	data->heater_power = 200;
> -	data->heater_time = 1000;
> -	data->heating_complete = jiffies;
> +	if (data->chip_id != sts4x) {
> +		data->heater_power = 200;
> +		data->heater_time = 1000;
> +		data->heating_complete = jiffies;
> +		groups = sht4x_groups;
> +	}

[Severity: High]
Since data->heating_complete is bypassed for the sts4x chip, it defaults to 0.

In sht4x_read_values(), the code unconditionally checks this:

	curr_jiffies = jiffies;
	if (time_before(curr_jiffies, data->heating_complete))
		msleep(jiffies_to_msecs(data->heating_complete - curr_jiffies));

Since jiffies is typically initialized to a large negative number at boot, it
evaluates as logically "before" 0.

Will this cause any userspace read of the temperature within the first 5
minutes of system uptime to hang while msleep waits for 0 - curr_jiffies?

[ ... ]

While reviewing this patch, I noticed a few pre-existing issues in the driver:

[Severity: High]
This isn't a bug introduced by this patch, but it looks like the driver lacks
a local mutex to protect I2C transfers.

The hwmon subsystem core only serializes sysfs operations for attributes
registered via the info parameter.

Custom attributes registered via extra_groups (like heater_enable) are not
protected by the hwmon core lock.

If a user writes to heater_enable, it triggers i2c_master_send():

drivers/hwmon/sht4x.c:heater_enable_store() {
	...
	ret = i2c_master_send(data->client, &cmd, SHT4X_CMD_LEN);
	if (ret < 0)
		return ret;

	data->heating_complete = jiffies + msecs_to_jiffies(heating_time_bound);
	data->data_pending = true;
	...
}

Can this run concurrently with sht4x_read_values() doing its multi-step
I2C transfers?

drivers/hwmon/sht4x.c:sht4x_read_values() {
	...
		ret = i2c_master_send(client, cmd, SHT4X_CMD_LEN);
		if (ret < 0)
			return ret;

		usleep_range(SHT4X_MEAS_DELAY_HPM, SHT4X_MEAS_DELAY_HPM + SHT4X_DELAY_EXTRA);
	}

	ret = i2c_master_recv(client, raw_data, response_length);
	...
}

If so, could this interleave and corrupt the I2C state machine or cause
measurement aborts?

[Severity: High]
This isn't a bug introduced by this patch, but heater_enable_store() returns
0 on success instead of the number of bytes processed (count):

drivers/hwmon/sht4x.c:heater_enable_store() {
	...
	data->heating_complete = jiffies + msecs_to_jiffies(heating_time_bound);
	data->data_pending = true;
	return 0;
}

Since standard userspace tools interpret a 0 return from write() as no bytes
consumed, will they loop to retry the write, which will immediately fail with
-EBUSY because data->heating_complete is now in the future?

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a unit
mismatch when calculating timeouts.

In sht4x_read_values(), data->update_interval (which stores milliseconds)
is added directly to data->heating_complete (which is in jiffies):

drivers/hwmon/sht4x.c:sht4x_read_values() {
	...
	if (data->data_pending &&
	    time_before(jiffies, data->heating_complete + data->update_interval)) {
		data->data_pending = false;
	...
}

On systems with HZ=100, could adding 2000 (the default update_interval)
evaluate to 20 seconds instead of 2 seconds, causing the driver to skip
sending the measurement command and jump straight to i2c_master_recv(),
resulting in I2C NACKs?

Should this be converted using msecs_to_jiffies()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2