linux-kernel - Re: [PATCH] hwmon: (sbtsi) Don't read sensor more than once if it doesn't respond

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ed438518-66ad-6e08-2a1b-597ac3f2ae8d@roeck-us.net>
Date:   Mon, 5 Apr 2021 14:41:17 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Konstantin Aladyshev <aladyshev22@...il.com>
Cc:     kunyi@...gle.com, Konstantin Aladyshev <aladyshev@...evt.ru>,
        Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hwmon: (sbtsi) Don't read sensor more than once if it
 doesn't respond

On 4/1/21 2:45 PM, Konstantin Aladyshev wrote:
> From: Konstantin Aladyshev <aladyshev@...evt.ru>
> 
> SBTSI sensors don't work when the CPU is off.
> In this case every 'i2c_smbus_read_byte_data' function would fail
> by a timeout.
> Currently temp1_max/temp1_min file reads can cause two such timeouts
> for every read.
> Restructure code so there will be no more than one timeout for every
> read opeartion.
> 

operation

> Signed-off-by: Konstantin Aladyshev <aladyshev@...evt.ru>
> ---
>  drivers/hwmon/sbtsi_temp.c | 59 +++++++++++++++++++-------------------
>  1 file changed, 29 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/hwmon/sbtsi_temp.c b/drivers/hwmon/sbtsi_temp.c
> index e35357c48b8e..e09a8cf6de45 100644
> --- a/drivers/hwmon/sbtsi_temp.c
> +++ b/drivers/hwmon/sbtsi_temp.c
> @@ -74,53 +74,52 @@ static int sbtsi_read(struct device *dev, enum hwmon_sensor_types type,
>  		      u32 attr, int channel, long *val)
>  {
>  	struct sbtsi_data *data = dev_get_drvdata(dev);
> +	u8 temp_int_reg, temp_dec_reg;
>  	s32 temp_int, temp_dec;
>  	int err;
>  
>  	switch (attr) {
>  	case hwmon_temp_input:
> -		/*
> -		 * ReadOrder bit specifies the reading order of integer and
> -		 * decimal part of CPU temp for atomic reads. If bit == 0,
> -		 * reading integer part triggers latching of the decimal part,
> -		 * so integer part should be read first. If bit == 1, read
> -		 * order should be reversed.
> -		 */
> -		err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
> -		if (err < 0)
> -			return err;
> -
> -		mutex_lock(&data->lock);
> -		if (err & BIT(SBTSI_CONFIG_READ_ORDER_SHIFT)) {
> -			temp_dec = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_DEC);
> -			temp_int = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_INT);
> -		} else {
> -			temp_int = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_INT);
> -			temp_dec = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_DEC);
> -		}
> -		mutex_unlock(&data->lock);
> +		temp_int_reg = SBTSI_REG_TEMP_INT;
> +		temp_dec_reg = SBTSI_REG_TEMP_DEC;
>  		break;
>  	case hwmon_temp_max:
> -		mutex_lock(&data->lock);
> -		temp_int = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_HIGH_INT);
> -		temp_dec = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_HIGH_DEC);
> -		mutex_unlock(&data->lock);
> +		temp_int_reg = SBTSI_REG_TEMP_HIGH_INT;
> +		temp_dec_reg = SBTSI_REG_TEMP_HIGH_DEC;
>  		break;
>  	case hwmon_temp_min:
> -		mutex_lock(&data->lock);
> -		temp_int = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_LOW_INT);
> -		temp_dec = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_LOW_DEC);
> -		mutex_unlock(&data->lock);
> +		temp_int_reg = SBTSI_REG_TEMP_LOW_INT;
> +		temp_dec_reg = SBTSI_REG_TEMP_LOW_DEC;
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * ReadOrder bit specifies the reading order of integer and
> +	 * decimal part of CPU temp for atomic reads. If bit == 0,
> +	 * reading integer part triggers latching of the decimal part,
> +	 * so integer part should be read first. If bit == 1, read
> +	 * order should be reversed.
> +	 */
> +	err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
> +	if (err < 0)
> +		return err;
> +

It seems the "fix" is to always execute above code, which presumably
fails if the CPU is off. The downside of this approach is that it forces
an unnecessary extra and unnecessary i2c operation when reading
the limits.

The real fix would be to check for error after each i2c operation,
not only after both operations are complete.

> +	mutex_lock(&data->lock);
> +	if (err & BIT(SBTSI_CONFIG_READ_ORDER_SHIFT)) {
> +		temp_dec = i2c_smbus_read_byte_data(data->client, temp_dec_reg);
> +		temp_int = i2c_smbus_read_byte_data(data->client, temp_int_reg);
> +	} else {
> +		temp_int = i2c_smbus_read_byte_data(data->client, temp_int_reg);
> +		temp_dec = i2c_smbus_read_byte_data(data->client, temp_dec_reg);
> +	}
> +	mutex_unlock(&data->lock);
>  
> -	if (temp_int < 0)
> -		return temp_int;
>  	if (temp_dec < 0)
>  		return temp_dec;
> +	if (temp_int < 0)
> +		return temp_int;

I don't see a value in swapping the checks.

Guenter

>  
>  	*val = sbtsi_reg_to_mc(temp_int, temp_dec);
>  
>