linux-kernel - Re: [PATCH] hwmon: sparx5: Fix initial reading of temperature

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: <57a29906-7831-dbfa-8344-f464308fcbc8@roeck-us.net>
Date:   Fri, 4 Sep 2020 07:08:43 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Lars Povlsen <lars.povlsen@...rochip.com>
Cc:     Jean Delvare <jdelvare@...e.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        linux-hwmon@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, Zhang Rui <rui.zhang@...el.com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Amit Kucheria <amitk@...nel.org>
Subject: Re: [PATCH] hwmon: sparx5: Fix initial reading of temperature

On 9/4/20 1:33 AM, Lars Povlsen wrote:
> 
> Guenter Roeck writes:
> 
>> On 9/3/20 6:47 AM, Lars Povlsen wrote:
>>> If the temperature is read before the internal calibration is
>>> completed, the driver returns -EIO. Instead it should return -EAGAIN
>>> to encourage repeating the operation.
>>>
>>> Signed-off-by: Lars Povlsen <lars.povlsen@...rochip.com>
>>> ---
>>>  drivers/hwmon/sparx5-temp.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hwmon/sparx5-temp.c b/drivers/hwmon/sparx5-temp.c
>>> index 1a2b1026b026..98be48e3a22a 100644
>>> --- a/drivers/hwmon/sparx5-temp.c
>>> +++ b/drivers/hwmon/sparx5-temp.c
>>> @@ -56,7 +56,7 @@ static int s5_read(struct device *dev, enum hwmon_sensor_types type,
>>>       case hwmon_temp_input:
>>>               stat = readl_relaxed(hwmon->base + TEMP_STAT);
>>>               if (!(stat & TEMP_STAT_VALID))
>>> -                     return -EIO;
>>> +                     return -EAGAIN;
>>
>> The problem is that this may result in a hard loop and effectively hang the system,
>> or at least the calling process. It may be better to return -ENODATA in this situation.
>>
> 
> Guenther,
> 
> Thank you for your response.
> 
> The motivation for the patch was as I tried to use thermal support to
> control a fan, and I noticed an initial quip from
> thermal_core.c:update_temperature() - it will throw a dev_warn() for
> anything else than an "EAGAIN". The EAGAIN also seems to be used by
> several other hwmon drivers in similar situations.
> 
> The bottom line is that I would like to get rid of the warning
> message. Changing the error code to ENODATA does not fix what I am
> trying to accomplish.
> 

Sigh. I know that -EAGAIN is used, but it has its own problems - userspace
is inclined to retry directly, causing hard loops.

Ok, lets go with -EAGAIN then. Can't be helped. I'll apply your patch.

Guenter

> Do you have any suggestions on how to achieve the objective?
> 
> Sincerely,
> 
> ---Lars
> 
>> Guenter
>>
>>>               value = stat & TEMP_STAT_TEMP;
>>>               /*
>>>                * From register documentation:
>>> --
>>> 2.27.0
>>>
>