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: <CANarb_ci_vgBMU4XejMwOwxKjGVccfMuweUQA7JwbNzWZqotuw@mail.gmail.com>
Date: Thu, 27 Feb 2025 10:39:56 +0100
From: Régis DARGENT <regis.dargent@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
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

Le mar. 25 févr. 2025 à 22:51, Guenter Roeck <linux@...ck-us.net> a écrit :
>
> 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.

Ok, I prefer parenthesis for (my) readability, but I will remove them.

wdt_timeout_map is not linear, so timeout in seconds is i when
wdt_timeout_map[i] == reg, some values of i not corresponding to any
reg value.
reg values of 0x0c and more will end with a timeout of 16s
(i == WDT_MAX_TIMEOUT) which will lead to unknown behaviour only
because these values are marked "reserved" in the HW timer documentation.

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

sorry about that, my comprehension of the documentation on
min/max_hw_heartbeat_ms suggested that they define the time window during
which the HW timer must receive a heartbeat or it will reset.
0-'timeout' defines the time window for the userspace point of view,
meaning that if 'timeout' > max_hw_heartbeat_ms, extra heartbeats must be
sent to HW timer so that the reset can occur only after 'timeout' seconds.

Just to be clear, setting max_hw_heartbeat_ms to the maximum HW possible
value (here 16000ms) means that one cannot use sub-second HW timer values,
while all the watchdog API actually handles them well enough (my tests on
the 500ms HW timer worked good).

For this sunxi driver case, this implies that instead of just reading the
current timeout value from the HW timer (if it was enabled before the
kernel start), I also have to reconfigure the HW timer (to 1sec) if its
timeout is less than a second (the 0 register value).

> Guenter
>