linux-kernel - Re: [PATCH v3] watchdog: sunxi_wdt: Allow watchdog to remain enabled after probe

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: <2547277e-d9c0-4138-abaa-7afbff1ba3ca@roeck-us.net>
Date: Tue, 25 Feb 2025 13:51:32 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Regis Dargent <regis.dargent@...il.com>
Cc: Wim Van Sebroeck <wim@...ux-watchdog.org>, Chen-Yu Tsai <wens@...e.org>,
 Jernej Skrabec <jernej.skrabec@...il.com>,
 Samuel Holland <samuel@...lland.org>, linux-watchdog@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] watchdog: sunxi_wdt: Allow watchdog to remain enabled
 after probe

On 2/25/25 06:36, Regis Dargent wrote:
> If the watchdog is already running during probe, let it run on, read its
> configured timeout, and set its status so that it is correctly handled by the
> kernel.
> 
> Signed-off-by: Regis Dargent <regis.dargent@...il.com>
> 
> --
> 
> Changelog v1..v2:
> - add sunxi_wdt_read_timeout function
> - add signed-off-by tag
> 
> Changelog v2..v3:
> - WDIOF_SETTIMEOUT was set twice, and other code cleanup
> ---
>   drivers/watchdog/sunxi_wdt.c | 45 ++++++++++++++++++++++++++++++++++--
>   1 file changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
> index b85354a99582..d509dbcb77ce 100644
> --- a/drivers/watchdog/sunxi_wdt.c
> +++ b/drivers/watchdog/sunxi_wdt.c
> @@ -140,6 +140,7 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev,
>   		timeout++;
>   
>   	sunxi_wdt->wdt_dev.timeout = timeout;
> +	sunxi_wdt->wdt_dev.max_hw_heartbeat_ms = 0;
>   
>   	reg = readl(wdt_base + regs->wdt_mode);
>   	reg &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
> @@ -152,6 +153,32 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev,
>   	return 0;
>   }
>   
> +static int sunxi_wdt_read_timeout(struct watchdog_device *wdt_dev)
> +{
> +	struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
> +	void __iomem *wdt_base = sunxi_wdt->wdt_base;
> +	const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
> +	int i;
> +	u32 reg;
> +
> +	reg = readl(wdt_base + regs->wdt_mode);
> +	reg >>= regs->wdt_timeout_shift;
> +	reg &= WDT_TIMEOUT_MASK;
> +
> +	/* Start at 0 which actually means 0.5s */
> +	for (i = 0; (i < WDT_MAX_TIMEOUT) && (wdt_timeout_map[i] != reg); i++)

Unnecessary (). On top of that, its complexity is unnecessary.
The timeout in seconds, except for reg == 0, is wdt_timeout_map[reg],
with values of 0x0c..0x0f undefined. Worse, the above code can access
beyond the size of wdt_timeout_map[] if reg >= 0x0c.

> +		;
> +	if (i == 0) {
> +		wdt_dev->timeout = 1;
> +		wdt_dev->max_hw_heartbeat_ms = 500;

This is an unacceptable API abuse. max_hw_heartbeat_ms, if set,
should be 16000, not 500. You could set the timeout to 1 second instead.

Guenter