linux-kernel - Re: [PATCH v4 4/4] watchdog: Add support for VIA/WonderMedia SoC watchdog functionality

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: <CABjd4YxE4eDYt7MEZCrqHK7G9SuziBYEtWSK35DEUxsMmW60tA@mail.gmail.com>
Date: Wed, 21 May 2025 18:15:44 +0400
From: Alexey Charkov <alchark@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Krzysztof Kozlowski <krzk@...nel.org>, Daniel Lezcano <daniel.lezcano@...aro.org>, 
	Thomas Gleixner <tglx@...utronix.de>, Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Wim Van Sebroeck <wim@...ux-watchdog.org>, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-watchdog@...r.kernel.org
Subject: Re: [PATCH v4 4/4] watchdog: Add support for VIA/WonderMedia SoC
 watchdog functionality

On Wed, May 21, 2025 at 5:33 PM Guenter Roeck <linux@...ck-us.net> wrote:
>
> On 5/20/25 23:20, Alexey Charkov wrote:
> > On Wed, May 21, 2025 at 3:15 AM Guenter Roeck <linux@...ck-us.net> wrote:
> >>
> >> On 5/20/25 13:01, Alexey Charkov wrote:
> >>> VIA/WonderMedia SoCs can use their system timer's first channel as a
> >>> watchdog device which will reset the system if the clocksource counter
> >>> matches the value given in its match register 0 and if the watchdog
> >>> function is enabled.
> >>>
> >>> Since the watchdog function is tightly coupled to the timer itself, it
> >>> is implemented as an auxiliary device of the timer device
> >>>
> >>> Signed-off-by: Alexey Charkov <alchark@...il.com>
> >>> ---
> >>>    MAINTAINERS                   |  1 +
> >>>    drivers/watchdog/Kconfig      | 15 ++++++++
> >>>    drivers/watchdog/Makefile     |  1 +
> >>>    drivers/watchdog/vt8500-wdt.c | 80 +++++++++++++++++++++++++++++++++++++++++++
> >>>    4 files changed, 97 insertions(+)
> >>>
> >>> diff --git a/MAINTAINERS b/MAINTAINERS
> >>> index 5362095240627f613638197fda275db6edc16cf7..97d1842625dbdf7fdca3556260662dab469ed091 100644
> >>> --- a/MAINTAINERS
> >>> +++ b/MAINTAINERS
> >>> @@ -3447,6 +3447,7 @@ F:      drivers/tty/serial/vt8500_serial.c
> >>>    F:  drivers/video/fbdev/vt8500lcdfb.*
> >>>    F:  drivers/video/fbdev/wm8505fb*
> >>>    F:  drivers/video/fbdev/wmt_ge_rops.*
> >>> +F:   drivers/watchdog/vt8500-wdt.c
> >>>    F:  include/linux/vt8500-timer.h
> >>>
> >>>    ARM/ZYNQ ARCHITECTURE
> >>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> >>> index 0d8d37f712e8cfb4bf8156853baa13c23a57d6d9..8049ae630301a98123f97f6e3ee868bd3bf1534a 100644
> >>> --- a/drivers/watchdog/Kconfig
> >>> +++ b/drivers/watchdog/Kconfig
> >>> @@ -2003,6 +2003,21 @@ config PIC32_DMT
> >>>          To compile this driver as a loadable module, choose M here.
> >>>          The module will be called pic32-dmt.
> >>>
> >>> +config VT8500_WATCHDOG
> >>> +     tristate "VIA/WonderMedia VT8500 watchdog support"
> >>> +     depends on ARCH_VT8500 || COMPILE_TEST
> >>> +     select WATCHDOG_CORE
> >>> +     select AUXILIARY_BUS
> >>> +     help
> >>> +       VIA/WonderMedia SoCs can use their system timer as a hardware
> >>> +       watchdog, as long as the first timer channel is free from other
> >>> +       uses and respective function is enabled in its registers. To
> >>> +       make use of it, say Y here and ensure that the device tree
> >>> +       lists at least two interrupts for the VT8500 timer device.
> >>> +
> >>> +       To compile this driver as a module, choose M here.
> >>> +       The module will be called vt8500-wdt.
> >>> +
> >>>    # PARISC Architecture
> >>>
> >>>    # POWERPC Architecture
> >>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> >>> index c9482904bf870a085c7fce2a439ac5089b6e6fee..3072786bf226c357102be3734fe6e701f753d45b 100644
> >>> --- a/drivers/watchdog/Makefile
> >>> +++ b/drivers/watchdog/Makefile
> >>> @@ -101,6 +101,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_VT8500_WATCHDOG) += vt8500-wdt.o
> >>>
> >>>    # X86 (i386 + ia64 + x86_64) Architecture
> >>>    obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
> >>> diff --git a/drivers/watchdog/vt8500-wdt.c b/drivers/watchdog/vt8500-wdt.c
> >>> new file mode 100644
> >>> index 0000000000000000000000000000000000000000..54fe5ad7695de36f6b3c1d28e955f00bef00e9db
> >>> --- /dev/null
> >>> +++ b/drivers/watchdog/vt8500-wdt.c
> >>> @@ -0,0 +1,80 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/* Copyright (C) 2025 Alexey Charkov <alchark@...il.com */
> >>> +
> >>> +#include <linux/auxiliary_bus.h>
> >>> +#include <linux/container_of.h>
> >>> +#include <linux/io.h>
> >>> +#include <linux/limits.h>
> >>> +#include <linux/module.h>
> >>> +#include <linux/types.h>
> >>> +#include <linux/watchdog.h>
> >>> +#include <linux/vt8500-timer.h>
> >>> +
> >>> +static int vt8500_watchdog_start(struct watchdog_device *wdd)
> >>> +{
> >>> +     struct vt8500_wdt_info *info = watchdog_get_drvdata(wdd);
> >>> +     u64 deadline = info->timer_next(wdd->timeout * VT8500_TIMER_HZ);
> >>> +
> >>
> >> wdd->timeout is the soft timeout, which may be larger
> >> than the maximum hardware timeout. That means you'll need
> >> to use something like "min(wdd->timeout, U32_MAX / VT8500_TIMER_HZ)
> >> * VT8500_TIMER_HZ" as parameter to the timer_next call.
> >
> > Indeed. For some reason I thought the core splits up large user
> > requested timeout values into at most max_hw_heartbeat_ms long chunks
> > and feeds those to the driver via the timeout field, but now I see it
> > doesn't.
> >
> > Do I get it right that the core worker will try and do its last ping
> > of the hardware at exactly max_hw_heartbeat_ms before the user
> > specified deadline?
> >
>
> Where do you see that ? In the watchdog core:
>
>          hw_heartbeat_ms = min_not_zero(timeout_ms, wdd->max_hw_heartbeat_ms);
>          keepalive_interval = ms_to_ktime(hw_heartbeat_ms / 2);

This comment [1] which follows the lines you've pasted: "To ensure
that the watchdog times out wdd->timeout seconds after the most recent
ping from userspace, the last worker ping has to come in
hw_heartbeat_ms before this timeout."

I'm not exactly sure I'm following the code path there 100%, so wanted
to double check. Meanwhile, I've just sent a new version [2] adding
the min function as you pointed out.

[1] https://elixir.bootlin.com/linux/v6.14.6/source/drivers/watchdog/watchdog_dev.c#L120
[2] https://lore.kernel.org/all/20250521-vt8500-timer-updates-v5-0-7e4bd11df72e@gmail.com/

Best regards,
Alexey