linux-kernel - Re: [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*

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]

Date:   Wed, 19 May 2021 06:55:42 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Jan Kundrát <jan.kundrat@...net.cz>
Cc:     Václav Kubernát <kubernat@...net.cz>,
        Jean Delvare <jdelvare@...e.com>,
        Jonathan Corbet <corbet@....net>, linux-hwmon@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable

On 5/19/21 2:10 AM, Jan Kundrát wrote:
>> As it turns out, even the current code doesn't really work for fans 7..12.
>>         sr = get_tach_period(data->fan_dynamics[channel]);
>> However, the data->fan_dynamics array has only 6 entries, not 12, so
>> reading fan[7-12]_input will result in bad/random values.
> 
> Hi Guenter, I'm Vaclav's colleague. The chip can indeed reconfigure each PWMOUT pin either as a PWM output or as a TACH input, but that's not something that's correctly implemented in the current code, and we have no use for that either (and we cannot test that on our PCBs easily, we do not have the manufacturer's eval kit).
> 
> It looks to me that the original bug is that the current docs mention 12 fan inputs. Would you be OK with a patch series which fixes the docs so that the chip always exports 6 TACH inputs and 6 PWMOUT channels?
> 
That would not be appropriate. The chip does support 12 fan inputs,
so that is not a bug. Its support has a bug, and the datasheet is kind
of vague when it comes to details, but that doesn't mean we can just
remove its support.

I see two bugs in the current code:
- pwm values should be read from the duty cycle registers,
   not from the target duty cycle registers.
- fan[1-12]_input needs to use a correct divider value.
   Unfortunately the datasheet is a bit vague when it comes
   to deciding which divider value to use, so the best we can
   do is going to use the values from fan[1-6].

Fixing this will require two patches, which should come first.
Let me know if you want to do that; if not I'll write patches
in the next few days.

As for fan[7-12]_enable, I don't even know if those can be enabled
separately. I see two options: Drop those attributes entirely (
assuming that those fan inputs are always enabled if the associated
pins are configured as inputs), or align them with fan[1-6]_enable.

Guenter