| 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: <ba0a378e-ccc3-45be-9c82-43e08b6ac3af@roeck-us.net>
Date: Fri, 31 Oct 2025 08:42:24 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
hrishabh.rajput@....qualcomm.com
Cc: Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>,
Wim Van Sebroeck <wim@...ux-watchdog.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, linux-arm-msm@...r.kernel.org,
linux-watchdog@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, Pavan Kondeti
<pavan.kondeti@....qualcomm.com>, Neil Armstrong <neil.armstrong@...aro.org>
Subject: Re: [PATCH v4 2/2] watchdog: Add driver for Gunyah Watchdog
On 10/31/25 07:36, Dmitry Baryshkov wrote:
> On Fri, Oct 31, 2025 at 10:18:14AM +0000, Hrishabh Rajput via B4 Relay wrote:
>> From: Hrishabh Rajput <hrishabh.rajput@....qualcomm.com>
>>
>> On Qualcomm SoCs running under the Gunyah hypervisor, access to watchdog
>> through MMIO is not available on all platforms. Depending on the
>> hypervisor configuration, the watchdog is either fully emulated or
>> exposed via ARM's SMC Calling Conventions (SMCCC) through the Vendor
>> Specific Hypervisor Service Calls space.
>>
>> Add driver to support the SMC-based watchdog provided by the Gunyah
>> Hypervisor. Device registration is done in the SMEM driver after checks
>> to restrict the watchdog initialization to Qualcomm devices.
>> module_exit() is intentionally not implemented as this driver is
>> intended to be a persistent module.
>>
>> Signed-off-by: Hrishabh Rajput <hrishabh.rajput@....qualcomm.com>
>> ---
>> MAINTAINERS | 1 +
>> drivers/watchdog/Kconfig | 14 +++
>> drivers/watchdog/Makefile | 1 +
>> drivers/watchdog/gunyah_wdt.c | 249 ++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 265 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index c0b444e5fd5a..56dbd0d3e31b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -3083,6 +3083,7 @@ F: arch/arm64/boot/dts/qcom/
>> F: drivers/bus/qcom*
>> F: drivers/firmware/qcom/
>> F: drivers/soc/qcom/
>> +F: drivers/watchdog/gunyah_wdt.c
>> F: include/dt-bindings/arm/qcom,ids.h
>> F: include/dt-bindings/firmware/qcom,scm.h
>> F: include/dt-bindings/soc/qcom*
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 0c25b2ed44eb..f0dee04b3650 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -2343,4 +2343,18 @@ config KEEMBAY_WATCHDOG
>> To compile this driver as a module, choose M here: the
>> module will be called keembay_wdt.
>>
>> +config GUNYAH_WATCHDOG
>> + tristate "Qualcomm Gunyah Watchdog"
>> + depends on ARCH_QCOM || COMPILE_TEST
>> + depends on HAVE_ARM_SMCCC
>> + depends on OF
>> + select WATCHDOG_CORE
>> + help
>> + Say Y here to include support for watchdog timer provided by the
>> + Gunyah hypervisor. The driver uses ARM SMC Calling Convention (SMCCC)
>> + to interact with Gunyah Watchdog.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called gunyah_wdt.
>> +
>> endif # WATCHDOG
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index bbd4d62d2cc3..308379782bc3 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -102,6 +102,7 @@ obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o
>> obj-$(CONFIG_APPLE_WATCHDOG) += apple_wdt.o
>> obj-$(CONFIG_SUNPLUS_WATCHDOG) += sunplus_wdt.o
>> obj-$(CONFIG_MARVELL_GTI_WDT) += marvell_gti_wdt.o
>> +obj-$(CONFIG_GUNYAH_WATCHDOG) += gunyah_wdt.o
>>
>> # X86 (i386 + ia64 + x86_64) Architecture
>> obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
>> diff --git a/drivers/watchdog/gunyah_wdt.c b/drivers/watchdog/gunyah_wdt.c
>> new file mode 100644
>> index 000000000000..bfe8b656d674
>> --- /dev/null
>> +++ b/drivers/watchdog/gunyah_wdt.c
>> @@ -0,0 +1,249 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>> + */
>> +
>> +#include <linux/arm-smccc.h>
>> +#include <linux/delay.h>
>> +#include <linux/errno.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>
> Is this header used here?
>
>> +#include <linux/platform_device.h>
>> +#include <linux/watchdog.h>
>> +
>> +#define GUNYAH_WDT_SMCCC_CALL_VAL(func_id) \
>> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_32,\
>> + ARM_SMCCC_OWNER_VENDOR_HYP, func_id)
>> +
>> +/* SMCCC function IDs for watchdog operations */
>> +#define GUNYAH_WDT_CONTROL GUNYAH_WDT_SMCCC_CALL_VAL(0x0005)
>> +#define GUNYAH_WDT_STATUS GUNYAH_WDT_SMCCC_CALL_VAL(0x0006)
>> +#define GUNYAH_WDT_PING GUNYAH_WDT_SMCCC_CALL_VAL(0x0007)
>> +#define GUNYAH_WDT_SET_TIME GUNYAH_WDT_SMCCC_CALL_VAL(0x0008)
>
> What about calls 0-4?
>
>> +
>> +/*
>> + * Control values for GUNYAH_WDT_CONTROL.
>> + * Bit 0 is used to enable or disable the watchdog. If this bit is set,
>> + * then the watchdog is enabled and vice versa.
>> + * Bit 1 should always be set to 1 as this bit is reserved in Gunyah and
>> + * it's expected to be 1.
>> + */
>> +#define WDT_CTRL_ENABLE (BIT(1) | BIT(0))
>> +#define WDT_CTRL_DISABLE BIT(1)
>> +
>> +enum gunyah_error {
>> + GUNYAH_ERROR_OK = 0,
>> + GUNYAH_ERROR_UNIMPLEMENTED = -1,
>> + GUNYAH_ERROR_ARG_INVAL = 1,
>> +};
>> +
>> +/**
>> + * gunyah_error_remap() - Remap Gunyah hypervisor errors into a Linux error code
>> + * @gunyah_error: Gunyah hypercall return value
>> + */
>> +static inline int gunyah_error_remap(enum gunyah_error gunyah_error)
>> +{
>> + switch (gunyah_error) {
>> + case GUNYAH_ERROR_OK:
>> + return 0;
>> + case GUNYAH_ERROR_UNIMPLEMENTED:
>> + return -EOPNOTSUPP;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static int gunyah_wdt_call(unsigned long func_id, unsigned long arg1,
>> + unsigned long arg2, struct arm_smccc_res *res)
>> +{
>
> struct arm_smccc_res res;
>
> There is no need to pass it through arguments.
>
>> + arm_smccc_1_1_smc(func_id, arg1, arg2, res);
>> + return gunyah_error_remap(res->a0);
>> +}
>> +
>> +static int gunyah_wdt_start(struct watchdog_device *wdd)
>> +{
>> + struct arm_smccc_res res;
>> + unsigned int timeout_ms;
>> + struct device *dev = wdd->parent;
>> + int ret;
>> +
>> + ret = gunyah_wdt_call(GUNYAH_WDT_CONTROL, WDT_CTRL_DISABLE, 0, &res);
>> + if (ret && watchdog_active(wdd)) {
>> + dev_err(dev, "%s: Failed to stop gunyah wdt %d\n", __func__, ret);
>> + return ret;
>> + }
>> +
>> + timeout_ms = wdd->timeout * 1000;
>> + ret = gunyah_wdt_call(GUNYAH_WDT_SET_TIME,
>> + timeout_ms, timeout_ms, &res);
>> + if (ret) {
>> + dev_err(dev, "%s: Failed to set timeout for gunyah wdt %d\n",
>> + __func__, ret);
>> + return ret;
>> + }
>> +
>> + ret = gunyah_wdt_call(GUNYAH_WDT_CONTROL, WDT_CTRL_ENABLE, 0, &res);
>> + if (ret)
>> + dev_err(dev, "%s: Failed to start gunyah wdt %d\n", __func__, ret);
>> +
>> + return ret;
>> +}
>> +
>> +static int gunyah_wdt_stop(struct watchdog_device *wdd)
>> +{
>> + struct arm_smccc_res res;
>> +
>> + return gunyah_wdt_call(GUNYAH_WDT_CONTROL, WDT_CTRL_DISABLE, 0, &res);
>> +}
>> +
>> +static int gunyah_wdt_ping(struct watchdog_device *wdd)
>> +{
>> + struct arm_smccc_res res;
>> +
>> + return gunyah_wdt_call(GUNYAH_WDT_PING, 0, 0, &res);
>> +}
>> +
>> +static int gunyah_wdt_set_timeout(struct watchdog_device *wdd,
>> + unsigned int timeout_sec)
>> +{
>> + wdd->timeout = timeout_sec;
>> +
>> + if (watchdog_active(wdd))
>> + return gunyah_wdt_start(wdd);
>> +
>> + return 0;
>> +}
>> +
>> +static unsigned int gunyah_wdt_get_timeleft(struct watchdog_device *wdd)
>> +{
>> + struct arm_smccc_res res;
>> + unsigned int seconds_since_last_ping;
>> + int ret;
>> +
>> + ret = gunyah_wdt_call(GUNYAH_WDT_STATUS, 0, 0, &res);
>> + if (ret)
>> + return 0;
>
> This is the only place which passes something back in res. Please wrap
> it separately and return int value.
>
>> +
>> + seconds_since_last_ping = res.a2 / 1000;
>> + if (seconds_since_last_ping > wdd->timeout)
>> + return 0;
>> +
>> + return wdd->timeout - seconds_since_last_ping;
>> +}
>> +
>> +static int gunyah_wdt_restart(struct watchdog_device *wdd,
>> + unsigned long action, void *data)
>> +{
>> + struct arm_smccc_res res;
>> +
>> + /* Set timeout to 1ms and send a ping */
>> + gunyah_wdt_call(GUNYAH_WDT_CONTROL, WDT_CTRL_ENABLE, 0, &res);
>> + gunyah_wdt_call(GUNYAH_WDT_SET_TIME, 1, 1, &res);
>> + gunyah_wdt_call(GUNYAH_WDT_PING, 0, 0, &res);
>> +
>> + /* Wait to make sure reset occurs */
>> + mdelay(100);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct watchdog_info gunyah_wdt_info = {
>> + .identity = "Gunyah Watchdog",
>> + .firmware_version = 0,
>
> =0 is default and can be omited
>
>> + .options = WDIOF_SETTIMEOUT
>> + | WDIOF_KEEPALIVEPING
>> + | WDIOF_MAGICCLOSE,
>> +};
>> +
>> +static const struct watchdog_ops gunyah_wdt_ops = {
>> + .owner = THIS_MODULE,
>> + .start = gunyah_wdt_start,
>> + .stop = gunyah_wdt_stop,
>> + .ping = gunyah_wdt_ping,
>> + .set_timeout = gunyah_wdt_set_timeout,
>> + .get_timeleft = gunyah_wdt_get_timeleft,
>> + .restart = gunyah_wdt_restart
>> +};
>> +
>> +static int gunyah_wdt_probe(struct platform_device *pdev)
>> +{
>> + struct arm_smccc_res res;
>> + struct watchdog_device *wdd;
>> + struct device *dev = &pdev->dev;
>> + int ret;
>> +
>> + ret = gunyah_wdt_call(GUNYAH_WDT_STATUS, 0, 0, &res);
>> + if (ret) {
>> + dev_dbg(dev, "Watchdog interface status check failed with %d\n", ret);
>
> dev_err
>
Then -ENODEV is inappropriate and the actual error should be returned.
Guenter
>> + return -ENODEV;
Powered by blists - more mailing lists