Re: hwmon: drivetemp: bogus values after wake up from suspend

[Guenter Roeck <linux@roeck-us.net>]

On 4/6/20 9:23 AM, Holger Hoffstätte wrote:
> 
> I've been giving the drivetemp hwmon driver a try and am very happy
> with it; works right away and - much to my surprise - doesn't wake up
> HDDs that have gone to sleep. Nice!
> 
> I did notice one tiny thing though: after waking up from suspend, my SSD
> (Samsung 850 Pro) reports a few initial bogus values - suspiciously -128°,
> which is definitely not the temperature in my office. While this is more
> a cosmetic problem, it cramps my monitoring setup and leads to wrong graphs.
> Can't have that!
> 
> So I looked into the source and found that the values are (understandably)
> passed on unfiltered/uncapped. Since it's unlikely any active device has
> operating temperature below-zero, I figured the laziest way is to cap the
> value to positive:
> 
> diff -rup a/drivers/hwmon/drivetemp.c b/drivers/hwmon/drivetemp.c
> --- a/drivers/hwmon/drivetemp.c    2020-04-02 08:02:32.000000000 +0200
> +++ b/drivers/hwmon/drivetemp.c    2020-04-06 18:13:04.892554087 +0200
> @@ -147,7 +147,7 @@ static LIST_HEAD(drivetemp_devlist);
>  #define INVALID_TEMP        0x80
>  
>  #define temp_is_valid(temp)    ((temp) != INVALID_TEMP)
> -#define temp_from_sct(temp)    (((s8)(temp)) * 1000)
> +#define temp_from_sct(temp)    (max(0, ((s8)(temp)) * 1000))
>  
>  static inline bool ata_id_smart_supported(u16 *id)
>  {
> 
> The assumption is of course *theoretically* wrong since some
> equipment might indeed operate in negative C°. One way might be
> to use the device's "low" operating point first, but then that
> might not be available and we'd be back to capping to 0.
> I'm open to other suggestions. :)
> 

Looking into the code, 0x80 or -128 indeed reflects an invalid temperature.
Any chance you can apply the following to see if it helps ?

diff --git a/drivers/hwmon/drivetemp.c b/drivers/hwmon/drivetemp.c
index 370d0c74eb01..c27239eb28cf 100644
--- a/drivers/hwmon/drivetemp.c
+++ b/drivers/hwmon/drivetemp.c
@@ -264,6 +264,8 @@ static int drivetemp_get_scttemp(struct drivetemp_data *st, u32 attr, long *val)
                return err;
        switch (attr) {
        case hwmon_temp_input:
+               if (!temp_is_valid(buf[SCT_STATUS_TEMP]))
+                       return -ENODATA;
                *val = temp_from_sct(buf[SCT_STATUS_TEMP]);
                break;
        case hwmon_temp_lowest:

I am not sure what the best error code would be - suggestions welcome.

Thanks,
Guenter