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: <CADvTj4pnYn478mzM6LQFJ7x1SBANY_Q4JBygTUqx2PnJgJkQxg@mail.gmail.com>
Date: Thu, 17 Oct 2024 22:29:41 -0600
From: James Hilliard <james.hilliard1@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: linux-watchdog@...r.kernel.org, Wim Van Sebroeck <wim@...ux-watchdog.org>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] watchdog: it87_wdt: add quirks for some Qotom IT8786 boards

On Thu, Oct 17, 2024 at 9:59 PM Guenter Roeck <linux@...ck-us.net> wrote:
>
> On 10/17/24 20:09, 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.
> >
> > For the Qotom QGLK02 board the vendor indicates that the IT8786
> > watchdog hardware is not functional due to a conflict with the BIOS
> > power-on function, with PWRGD set the watchdog will trigger but the
> > board will poweroff rather than restart as expected. Disable the
> > it87 driver on this broken hardware.
> >
>
> This shouldn't be done in drivers, and it doesn't scale. The driver needs
> to be disabled with the mechanism supported by the distribution, for example
> in /etc/modprobe.d/blacklist-watchdog.conf, or by whatever other mechanism
> the distribution supports for that purpose.

There isn't really a good way that I've found with my setup since I use common
images for both of these boards. I'm also worried that it's much easier to mess
something critical like this up if user space is involved in hardware detection.

Many other watchdog drivers do this sort of thing so I'm a bit confused why we
would want to not do that here as well, for example:
https://github.com/torvalds/linux/blob/v6.11/drivers/watchdog/renesas_wdt.c#L176-L207
https://github.com/torvalds/linux/blob/v6.11/drivers/watchdog/ebc-c384_wdt.c#L125
https://github.com/torvalds/linux/blob/v6.11/drivers/watchdog/lenovo_se10_wdt.c#L242-L285
https://github.com/torvalds/linux/blob/v6.11/drivers/watchdog/sbc_fitpc2_wdt.c#L203-L206

>
> Guenter
>
> > Signed-off-by: James Hilliard <james.hilliard1@...il.com>
> > ---
> >   drivers/watchdog/it87_wdt.c | 55 +++++++++++++++++++++++++++++++++++++
> >   1 file changed, 55 insertions(+)
> >
> > diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
> > index 3e8c15138edd..dec501c21fd3 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
> > +
> >   /* GPIO Configuration Registers LDN=0x07 */
> >   #define WDTCTRL             0x71
> >   #define WDTCFG              0x72
> > @@ -240,6 +248,28 @@ static int wdt_set_timeout(struct watchdog_device *wdd, unsigned int t)
> >       return ret;
> >   }
> >
> > +enum {
> > +     IT87_WDT_BROKEN                 = BIT(0),
> > +     IT87_WDT_OUTPUT_THROUGH_PWRGD   = BIT(1),
> > +};
> > +
> > +static const struct dmi_system_id it8786_quirks[] = {
> > +     {
> > +             /* Qotom Q730P/Q8XXG2-P */
> > +             .matches = {
> > +                     DMI_EXACT_MATCH(DMI_BOARD_NAME, "QGLK02"),
> > +             },
> > +             .driver_data = (void *)IT87_WDT_BROKEN,
> > +     },
> > +     {
> > +             /* 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 +291,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 +305,20 @@ 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;
> > +
> > +     if (quirks & IT87_WDT_BROKEN)
> > +             return -ENODEV;
> > +
> >       switch (chip_type) {
> >       case IT8702_ID:
> >               max_units = 255;
> > @@ -333,6 +379,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) {
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ