linux-kernel - Re: [PATCH v2] watchdog: it87_wdt: add PWRGD enable quirk for Qotom QCML04

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: <0702196c-5447-4a4b-ac71-336dbf06a6c7@roeck-us.net>
Date: Sat, 19 Oct 2024 19:48:40 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: James Hilliard <james.hilliard1@...il.com>
Cc: linux-watchdog@...r.kernel.org, Wim Van Sebroeck
 <wim@...ux-watchdog.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] watchdog: it87_wdt: add PWRGD enable quirk for Qotom
 QCML04

On 10/19/24 12:55, James Hilliard wrote:
> On Sat, Oct 19, 2024 at 8:33 AM Guenter Roeck <linux@...ck-us.net> wrote:
>>
>> On 10/18/24 08:48, James Hilliard wrote:
>>> For the watchdog timer to work properly on the QCML04 board we need to
>>> set PWRGD enable in the Environment Controller Configuration Registers
>>> Special Configuration Register 1 when it is not already set, this may
>>> be the case when the watchdog is not enabled from within the BIOS.
>>>
>>> Signed-off-by: James Hilliard <james.hilliard1@...il.com>
>>> ---
>>> Changes v1 -> v2:
>>>     - remove QGLK02/IT87_WDT_BROKEN
>>> ---
>>>    drivers/watchdog/it87_wdt.c | 44 +++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 44 insertions(+)
>>>
>>> diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
>>> index 3e8c15138edd..b8be9af065b2 100644
>>> --- a/drivers/watchdog/it87_wdt.c
>>> +++ b/drivers/watchdog/it87_wdt.c
>>> @@ -20,6 +20,7 @@
>>>
>>>    #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>
>>> +#include <linux/dmi.h>
>>>    #include <linux/init.h>
>>>    #include <linux/io.h>
>>>    #include <linux/kernel.h>
>>> @@ -40,6 +41,7 @@
>>>    #define VAL         0x2f
>>>
>>>    /* Logical device Numbers LDN */
>>> +#define EC           0x04
>>>    #define GPIO                0x07
>>>
>>>    /* Configuration Registers and Functions */
>>> @@ -73,6 +75,12 @@
>>>    #define IT8784_ID   0x8784
>>>    #define IT8786_ID   0x8786
>>>
>>> +/* Environment Controller Configuration Registers LDN=0x04 */
>>> +#define SCR1         0xfa
>>> +
>>> +/* Environment Controller Bits SCR1 */
>>> +#define WDT_PWRGD    0x20
>>
>> The IT8786 documentation I have states that this bit is reserved.
>> Do you have information suggesting otherwise ?
> 
> Yes, if you clone this repo you'll see the docs in the .rar archive:
> https://gitcode.com/open-source-toolkit/c602e.git
> 
>>
>>> +
>>>    /* GPIO Configuration Registers LDN=0x07 */
>>>    #define WDTCTRL             0x71
>>>    #define WDTCFG              0x72
>>> @@ -240,6 +248,20 @@ static int wdt_set_timeout(struct watchdog_device *wdd, unsigned int t)
>>>        return ret;
>>>    }
>>>
>>> +enum {
>>> +     IT87_WDT_OUTPUT_THROUGH_PWRGD   = BIT(0),
>>
>> I don't mind starting to use BIT(), but then <linux/bits.h> needs to be
>> included as well.
> 
> Ok, will add in v3.
> 
>>
>>> +};
>>> +
>>> +static const struct dmi_system_id it8786_quirks[] = {
>>
>> I see that bit 5 of EC register 0xfa _is_ documented for this purpose on
>> at least one other chip supported by this driver, so the flag should be made
>> generic, and not be IT8786 specific. Please name the quirks it87_quirks
>> or similar and check it for all chips.
> 
> So, the enum is generic but I wanted to separate out the quirks by chip
> since from my understanding each board only uses one chip model and
> thus doing the DMI check for chips with no known boards that need
> quirks applied would be unnecessary. This also helps to document in the
> code which chip a specific board uses which I think is potentially useful
> information to have. If quirks for additional chips end up being needed
> one can simply add another quirks table for that chip.
> 
> I'm also trying to be as specific as possible for the DMI match since these
> Qotom boards only set board names and have no other unique DMI attributes
> we can match against like vendor names.
> 
> Maybe I'm being a bit overly paranoid here but I figured it would be best
> to minimize the probability of a bad match as much as possible.
> 

I really think you are, and I really do not want to have to maintain
multiple quirks tables, and I do not want to see a case match per chip
type. I also really do not care if dmi_first_match() is called for each
chip. It is called exactly once.

Please let's stick with one table and a single call to dmi_first_match().
That is much simpler and much less error prone. If that ever turns out
to be insufficient, we can talk about it then.

Thanks,
Guenter