linux-kernel - Re: [PATCH v2] 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] [day] [month] [year] [list]

Message-ID: <0ba1d271-78cd-c30a-e662-97ab8ba6451a@roeck-us.net>
Date:   Tue, 6 Apr 2021 11:42:07 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Konstantin Aladyshev <aladyshev22@...il.com>
Cc:     kunyi@...gle.com, Jean Delvare <jdelvare@...e.com>,
        linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] hwmon: (sbtsi) Don't read sensor more than once if it
 doesn't respond

On 4/6/21 11:16 AM, Konstantin Aladyshev wrote:
> 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 operation.
> 
> Signed-off-by: Konstantin Aladyshev <aladyshev22@...il.com>
> ---
> Changes in v2:
>   - Fix typo in a commit message
>   - Don't swap temp_int/temp_dec checks at the end of the 'sbtsi_read' function
> 

This doesn't explain the reason for the extra read operation for
limits. Preventing a second read in error cases is not an argument
for adding an extra read for non-error cases.

Guenter

>  drivers/hwmon/sbtsi_temp.c | 55 +++++++++++++++++++-------------------
>  1 file changed, 27 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/hwmon/sbtsi_temp.c b/drivers/hwmon/sbtsi_temp.c
> index e35357c48b8e..4ae48635bb31 100644
> --- a/drivers/hwmon/sbtsi_temp.c
> +++ b/drivers/hwmon/sbtsi_temp.c
> @@ -74,48 +74,47 @@ 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;
> +
> +	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;
>