linux-kernel - Re: adm9240 error handling (was Re: [PATCHv1] hwmon: adm9240: handle temperature readings below 0)

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: <26be7a39-780e-2659-9afd-ec82f2511845@roeck-us.net>
Date:   Wed, 5 Oct 2016 06:23:37 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Chris Packham <Chris.Packham@...iedtelesis.co.nz>
Cc:     "jdelvare@...e.com" <jdelvare@...e.com>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Michael Wang <Michael.Wang@...iedtelesis.co.nz>
Subject: Re: adm9240 error handling (was Re: [PATCHv1] hwmon: adm9240: handle
 temperature readings below 0)

On 10/04/2016 10:00 PM, Chris Packham wrote:
> Hi Guenter,
>
> On 10/05/2016 11:10 AM, Guenter Roeck wrote:
>> On Tue, Oct 04, 2016 at 09:09:10PM +0000, Chris Packham wrote:
>>>
>>>>
>>>> Of course, all that doesn't solve the real problem in this driver, which is
>>>> that it ignores error codes from the smbus functions, but that is a different
>>>> problem.
>>>
>>> Yeah I'd noted that too. This patch is actually a port of changes we've
>>> made to a custom LM81 driver based on the original adm9240 driver which
>>> included some error handling. I'll look at bringing more of that code in
>>> for a future patch.
>>>
>> Sounds good...
>>
>
> So on this. The as-yet unpublished changes we have basically consist of
> changing adm9240_update_device() to do something like this
>
>
>                          || !data->valid) {
>
>                  for (i = 0; i < 6; i++) { /* read voltages */
> -                       data->in[i] = i2c_smbus_read_byte_data(client,
> +                       ret = i2c_smbus_read_byte_data(client,
>                                          ADM9240_REG_IN(i));
> +                       if (ret >= 0)
> +                               data->in[i] = ret;
>
> The user is still none the wiser that there was an error but at least we
> haven't just attempted to write a error code to the data. Is this the
> direction you want to head? Are there other hwmon drivers that do
> something saner?
>
> Another attempt we've made is to try to read something innocuous like
> the CONFIG register to check that the i2c bus is in a good state before
> reading the rest of the values.
>
> As you can guess these "fixes" have piled up over time and because
> they're hard to test we haven't been brave enough to remove them.
>
> Any thoughts?
>

The usual approach would be to return an error from adm9240_update_device(),
and report it to the user. Hiding the error and returning stale data doesn't
sound like a good idea.

Another option would be to drop value caching from the driver entirely,
and convert it to use regmap (so only the actually changing registers
would be read again).

Thanks,
Guenter