Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: Guenter Roeck <groeck7@gmail.com>
To:     Riwen Lu <luriwen@kylinos.cn>, jdelvare@suse.com
Cc:     linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org,
        Xin Chen <chenxin@kylinos.cn>
References: <20210521124204.22263-1-luriwen@kylinos.cn>
From:   Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH v1] hwmon: (scpi-hwmon) shows the negative temperature
 properly
Message-ID: <3e6f68d1-c8f6-8521-833f-a4652ef8236a@roeck-us.net>
Date:   Fri, 21 May 2021 07:43:41 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <20210521124204.22263-1-luriwen@kylinos.cn>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 5/21/21 5:42 AM, Riwen Lu wrote:
> The scpi hwmon shows the sub-zero temperature in an unsigned integer,
> which would confuse the users when the machine works in low temperature
> environment. This shows the sub-zero temperature in an signed value and
> users can get it properly from sensors.
> 
> Signed-off-by: Riwen Lu <luriwen@kylinos.cn>
> Tested-by: Xin Chen <chenxin@kylinos.cn>

What did you test ? Did you really manage to run the system
in such an environment ?

> ---
>   drivers/hwmon/scpi-hwmon.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/scpi-hwmon.c b/drivers/hwmon/scpi-hwmon.c
> index 25aac40f2764..583a600bc82d 100644
> --- a/drivers/hwmon/scpi-hwmon.c
> +++ b/drivers/hwmon/scpi-hwmon.c
> @@ -99,7 +99,7 @@ scpi_show_sensor(struct device *dev, struct device_attribute *attr, char *buf)
>   
>   	scpi_scale_reading(&value, sensor);
>   
> -	return sprintf(buf, "%llu\n", value);
> +	return sprintf(buf, "%lld\n", value);

'value' is declared as u64, not as s64.
I can not evaluate what the firmware actually reports. The API
reports an u64. Do you have any evidence for your claim that
it returns a signed value under any circumstances ?

On top of that, your change affects not only temperature values,
but all attributes. It is highly unlikely that the firmware would
report negative power or energy values. It is, however, possible
that energy values have the upper bit of an u64 set after a
long runtime. Your change would result in a negative energy value
if that is ever the case.

Guenter