| 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
| ||
|
Message-ID: <8bdf9a4d-5d25-4b53-947b-0644c00cc999@roeck-us.net>
Date: Tue, 11 Mar 2025 09:06:09 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Diogo Ivo <diogo.ivo@...mens.com>, "Rafael J. Wysocki"
<rafael@...nel.org>, Len Brown <lenb@...nel.org>,
Wim Van Sebroeck <wim@...ux-watchdog.org>
Cc: linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
linux-watchdog@...r.kernel.org, jan.kiszka@...mens.com,
benedikt.niedermayr@...mens.com
Subject: Re: [PATCH] watchdog: Add driver for Intel OC WDT
On 3/11/25 07:59, Diogo Ivo wrote:
> Hi Guenter, thanks for the quick review!
>
> On 3/11/25 2:10 PM, Guenter Roeck wrote:
>> On 3/11/25 06:18, Diogo Ivo wrote:
>>> Add a driver for the Intel Over-Clocking Watchdog found in Intel
>>> Platform Controller (PCH) chipsets. This watchdog is controlled
>>> via a simple single-register interface and would otherwise be
>>> standard except for the presence of a LOCK bit that can only be
>>> set once per power cycle, needing extra handling around it.
>>>
>>> Signed-off-by: Diogo Ivo <diogo.ivo@...mens.com>
>>> ---
>>> drivers/acpi/acpi_pnp.c | 2 +
>>> drivers/watchdog/Kconfig | 11 ++
>>> drivers/watchdog/Makefile | 1 +
>>> drivers/watchdog/intel_oc_wdt.c | 235 ++++++++++++++++++++++++++++++ ++++++++++
>>> 4 files changed, 249 insertions(+)
>>>
>>> diff --git a/drivers/acpi/acpi_pnp.c b/drivers/acpi/acpi_pnp.c
>>> index 01abf26764b00c86f938dea2ed138424f041f880..3f5a1840f573303c71f5d579e32963a5b29d2587 100644
>>> --- a/drivers/acpi/acpi_pnp.c
>>> +++ b/drivers/acpi/acpi_pnp.c
>>> @@ -355,8 +355,10 @@ static bool acpi_pnp_match(const char *idstr, const struct acpi_device_id **matc
>>> * device represented by it.
>>> */
>>> static const struct acpi_device_id acpi_nonpnp_device_ids[] = {
>>> + {"INT3F0D"},
>>> {"INTC1080"},
>>> {"INTC1081"},
>>> + {"INTC1099"},
>>> {""},
>>> };
>>
>> This needs to be a separate patch.
>
> I will split it for v2.
>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>> index f81705f8539aa0b12d156a86aae521aa40b4527d..16e6df441bb344c0f91b40bd76b6322ad3016e72 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -1350,6 +1350,17 @@ config INTEL_MID_WATCHDOG
>>> To compile this driver as a module, choose M here.
>>> +config INTEL_OC_WATCHDOG
>>> + tristate "Intel OC Watchdog"
>>> + depends on X86 && ACPI
>>> + select WATCHDOG_CORE
>>> + help
>>> + Hardware driver for Intel Over-Clocking watchdog present in
>>> + Platform Controller Hub (PCH) chipsets.
>>> +
>>> + To compile this driver as a module, choose M here: the
>>> + module will be called intel_oc_wdt.
>>> +
>>> config ITCO_WDT
>>> tristate "Intel TCO Timer/Watchdog"
>>> depends on X86 && PCI
>>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>>> index 8411626fa162268e8ccd06349f7193b15a9d281a..3a13f3e80a0f460b99b4f1592fcf17cc6428876b 100644
>>> --- a/drivers/watchdog/Makefile
>>> +++ b/drivers/watchdog/Makefile
>>> @@ -149,6 +149,7 @@ obj-$(CONFIG_W83977F_WDT) += w83977f_wdt.o
>>> obj-$(CONFIG_MACHZ_WDT) += machzwd.o
>>> obj-$(CONFIG_SBC_EPX_C3_WATCHDOG) += sbc_epx_c3.o
>>> obj-$(CONFIG_INTEL_MID_WATCHDOG) += intel-mid_wdt.o
>>> +obj-$(CONFIG_INTEL_OC_WATCHDOG) += intel_oc_wdt.o
>>> obj-$(CONFIG_INTEL_MEI_WDT) += mei_wdt.o
>>> obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o
>>> obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o
>>> diff --git a/drivers/watchdog/intel_oc_wdt.c b/drivers/watchdog/ intel_oc_wdt.c
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..0a2df3440024090f7e342fe7da895a7930ac09b2
>>> --- /dev/null
>>> +++ b/drivers/watchdog/intel_oc_wdt.c
>>> @@ -0,0 +1,235 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Intel OC Watchdog driver
>>> + *
>>> + * Copyright (C) 2025, Siemens SA
>>> + * Author: Diogo Ivo <diogo.ivo@...mens.com>
>>> + */
>>> +
>>> +#define DRV_NAME "intel_oc_wdt"
>>> +
>>> +#include <linux/acpi.h>
>>> +#include <linux/bits.h>
>>> +#include <linux/io.h>
>>> +#include <linux/module.h>
>>> +#include <linux/moduleparam.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/watchdog.h>
>>> +
>>> +#define INTEL_OC_WDT_TOV GENMASK(9, 0)
>>> +#define INTEL_OC_WDT_MIN_TOV 1
>>> +#define INTEL_OC_WDT_MAX_TOV 1024
>>> +
>>> +/*
>>> + * One-time writable lock bit. If set forbids
>>> + * modification of itself, _TOV and _EN until
>>> + * next reboot.
>>> + */
>>> +#define INTEL_OC_WDT_CTL_LCK BIT(12)
>>> +
>>> +#define INTEL_OC_WDT_EN BIT(14)
>>> +#define INTEL_OC_WDT_NO_ICCSURV_STS BIT(24)
>>> +#define INTEL_OC_WDT_ICCSURV_STS BIT(25)
>>> +#define INTEL_OC_WDT_RLD BIT(31)
>>> +
>>> +#define INTEL_OC_WDT_STS_BITS (INTEL_OC_WDT_NO_ICCSURV_STS | \
>>> + INTEL_OC_WDT_ICCSURV_STS)
>>> +
>>> +#define INTEL_OC_WDT_CTRL_REG(wdt) ((wdt)->ctrl_res->start)
>>> +
>>> +struct intel_oc_wdt {
>>> + struct watchdog_device wdd;
>>> + struct resource *ctrl_res;
>>> + bool locked;
>>> +};
>>> +
>>> +#define WDT_HEARTBEAT 60
>>> +static int heartbeat = WDT_HEARTBEAT;
>>
>> Normally this is set to 0 and the default timeout is initialized together
>> with min_timeout and max_timeout. This lets the watchdog core override it
>> with devicetree data (if that is available).
>
> Ok, thank you for the insight. I will address this for v2.
> It is unlikely that this driver will have devicetree data but it's
> better to follow best practice.
>
I just submitted a patch to have the watchdog core also look into firmware data,
which would include data provided by ACPI.
>>> +module_param(heartbeat, uint, 0);
>>> +MODULE_PARM_DESC(heartbeat, "Watchdog heartbeats in seconds. (default="
>>> + __MODULE_STRING(WDT_HEARTBEAT) ")");
>>> +
>>> +static bool nowayout = WATCHDOG_NOWAYOUT;
>>> +module_param(nowayout, bool, 0);
>>> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>>> + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>>> +
>>> +static int intel_oc_wdt_start(struct watchdog_device *wdd)
>>> +{
>>> + struct intel_oc_wdt *oc_wdt = watchdog_get_drvdata(wdd);
>>> +
>>> + if (oc_wdt->locked)
>>> + return 0;
>>> +
>>> + outl(inl(INTEL_OC_WDT_CTRL_REG(oc_wdt)) | INTEL_OC_WDT_EN,
>>> + INTEL_OC_WDT_CTRL_REG(oc_wdt));
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int intel_oc_wdt_stop(struct watchdog_device *wdd)
>>> +{
>>> + struct intel_oc_wdt *oc_wdt = watchdog_get_drvdata(wdd);
>>> +
>>> + outl(inl(INTEL_OC_WDT_CTRL_REG(oc_wdt)) & ~INTEL_OC_WDT_EN,
>>> + INTEL_OC_WDT_CTRL_REG(oc_wdt));
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int intel_oc_wdt_ping(struct watchdog_device *wdd)
>>> +{
>>> + struct intel_oc_wdt *oc_wdt = watchdog_get_drvdata(wdd);
>>> +
>>> + outl(inl(INTEL_OC_WDT_CTRL_REG(oc_wdt)) | INTEL_OC_WDT_RLD,
>>> + INTEL_OC_WDT_CTRL_REG(oc_wdt));
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int intel_oc_wdt_set_timeout(struct watchdog_device *wdd,
>>> + unsigned int t)
>>> +{
>>> + struct intel_oc_wdt *oc_wdt = watchdog_get_drvdata(wdd);
>>> +
>>> + outl((inl(INTEL_OC_WDT_CTRL_REG(oc_wdt)) & ~INTEL_OC_WDT_TOV) | (t - 1),
>>> + INTEL_OC_WDT_CTRL_REG(oc_wdt));
>>> +
>>> + wdd->timeout = t;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct watchdog_info intel_oc_wdt_info = {
>>> + .options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
>>> + .identity = DRV_NAME,
>>> +};
>>> +
>>> +static const struct watchdog_ops intel_oc_wdt_ops = {
>>> + .owner = THIS_MODULE,
>>> + .start = intel_oc_wdt_start,
>>> + .stop = intel_oc_wdt_stop,
>>> + .ping = intel_oc_wdt_ping,
>>> + .set_timeout = intel_oc_wdt_set_timeout,
>>> +};
>>> +
>>> +static int intel_oc_wdt_setup(struct intel_oc_wdt *oc_wdt)
>>> +{
>>> + struct watchdog_info *info;
>>> + unsigned long val;
>>> +
>>> + val = inl(INTEL_OC_WDT_CTRL_REG(oc_wdt));
>>> +
>>> + if (val & INTEL_OC_WDT_STS_BITS)
>>> + oc_wdt->wdd.bootstatus |= WDIOF_CARDRESET;
>>> +
>>> + oc_wdt->locked = !!(val & INTEL_OC_WDT_CTL_LCK);
>>> +
>>> + if (val & INTEL_OC_WDT_EN) {
>>> + /*
>>> + * No need to issue a ping here to "commit" the new timeout
>>> + * value to hardware as the watchdog core schedules one
>>> + * immediately when registering the watchdog.
>>> + */
>>> + set_bit(WDOG_HW_RUNNING, &oc_wdt->wdd.status);
>>> +
>>> + if (oc_wdt->locked) {
>>> + info = (struct watchdog_info *)&intel_oc_wdt_info;
>>> + /*
>>> + * Set nowayout unconditionally as we cannot stop
>>> + * the watchdog and read the current timeout.
>>> + */
>>
>> But the timeout is read below ? Do you mean "change the current timeout" ?
>
> In this case where the BIOS both enabled the watchdog and set the LOCK
> bit we cannot change the timeout anymore, meaning that we have to read
> the value currently in the register and pass it to the watchdog core,
> which is what is done below.
>
Yes, but the comment says " we cannot stop the watchdog and _read_ the
current timeout" (emphasis added), suggesting that the current timeout
can not be _read_ if locked is true.
>>> + nowayout = true;
>>> +
>>> + oc_wdt->wdd.timeout = (val & INTEL_OC_WDT_TOV) + 1;
However, this code does read the timeout even if locked is set. That
suggests that it is not possible to _change_ the timeout if locked is set,
but that it is possible to read the current timeout.
Guenter
Powered by blists - more mailing lists