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] [thread-next>] [day] [month] [year] [list]

Message-ID: <74b01dd4-79bb-44bb-98a4-a478a99a5654@roeck-us.net>
Date: Sat, 19 Oct 2024 07:33:54 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: James Hilliard <james.hilliard1@...il.com>, linux-watchdog@...r.kernel.org
Cc: 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/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 ?

> +
>   /* 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.

> +};
> +
> +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.

Thanks,
Guenter

> +	{
> +		/* Qotom Q30900P */
> +		.matches = {
> +			DMI_EXACT_MATCH(DMI_BOARD_NAME, "QCML04"),
> +		},
> +		.driver_data = (void *)IT87_WDT_OUTPUT_THROUGH_PWRGD,
> +	}
> +};
> +
>   static const struct watchdog_info ident = {
>   	.options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
>   	.firmware_version = 1,
> @@ -261,8 +283,10 @@ static struct watchdog_device wdt_dev = {
>   
>   static int __init it87_wdt_init(void)
>   {
> +	const struct dmi_system_id *dmi_id;
>   	u8  chip_rev;
>   	u8 ctrl;
> +	int quirks = 0;
>   	int rc;
>   
>   	rc = superio_enter();
> @@ -273,6 +297,17 @@ static int __init it87_wdt_init(void)
>   	chip_rev  = superio_inb(CHIPREV) & 0x0f;
>   	superio_exit();
>   
> +	switch (chip_type) {
> +	case IT8786_ID:
> +		dmi_id = dmi_first_match(it8786_quirks);
> +		break;
> +	default:
> +		dmi_id = NULL;
> +	}
> +
> +	if (dmi_id)
> +		quirks = (long)dmi_id->driver_data;
> +
>   	switch (chip_type) {
>   	case IT8702_ID:
>   		max_units = 255;
> @@ -333,6 +368,15 @@ static int __init it87_wdt_init(void)
>   		superio_outb(0x00, WDTCTRL);
>   	}
>   
> +	if (quirks & IT87_WDT_OUTPUT_THROUGH_PWRGD) {
> +		superio_select(EC);
> +		ctrl = superio_inb(SCR1);
> +		if (!(ctrl & WDT_PWRGD)) {
> +			ctrl |= WDT_PWRGD;
> +			superio_outb(ctrl, SCR1);
> +		}
> +	}
> +
>   	superio_exit();
>   
>   	if (timeout < 1 || timeout > max_units * 60) {