| 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: <CAGb2v67tmzGfcFPchjzfSima-sT_u7viYd1UDGB9r6ZeJEgdyg@mail.gmail.com>
Date: Thu, 6 Feb 2025 18:14:26 +0800
From: Chen-Yu Tsai <wens@...e.org>
To: Regis Dargent <regis.dargent@...il.com>
Cc: linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
linux-kernel@...r.kernel.org, Wim Van Sebroeck <wim@...ux-watchdog.org>,
Guenter Roeck <linux@...ck-us.net>, linux-watchdog@...r.kernel.org,
Jernej Skrabec <jernej.skrabec@...il.com>, Samuel Holland <samuel@...lland.org>
Subject: Re: [PATCH]: watchdog: Allow watchdog to remain enabled after probe
On Thu, Feb 6, 2025 at 5:40 PM Regis Dargent <regis.dargent@...il.com> wrote:
Please format your commit subject correctly. See previous commits for
examples.
Please provide a commit message including justification for the change.
The message below already makes sense, though it could be made more
concise.
Please also include your Signed-off-by.
> ---
> The sunxi_wdt watchdog unconditionally stops the watchdog during probe (on my Allwinner H616).
>
> What I want to achieve with this patch is to start the watchdog in the bootloader (either manually or automatically), then boot Linux.
> The watchdog is about 16sec timeout maximum, while the full boot to userland lasts about 90sec, and I want the board to reset if, eg.
> the rootfs cannot be mounted. So I need the watchdog to be handled by the kernel during boot (which it can do pretty well).
>
> The thing is, the current driver stops the watchdog during probe, so it does not run during boot, and it also does not manages the "status"
> field, so the kernel would know that it must handle the HW watchdog.
> This avoids automatic reboot in case a problem occurs during boot (and for example handling in the bootloader).
>
> The driver should detect if the HW watchdog is already running during probe and set its appropriate status bit to allow the kernel to handle the watchdog pings itself.
> The call to sunxi_wdt_start/stop allows for proper driver and device configuration.
> By default, the kernel will then ping the HW watchdog at apropriate frequency, but the user can have it stop after a time with open_timeout parameter.
^ appropriate
Please wrap your messages under 80 characters per line.
> drivers/watchdog/sunxi_wdt.c | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
> index b85354a99582..20fe7da445ea 100644
> --- a/drivers/watchdog/sunxi_wdt.c
> +++ b/drivers/watchdog/sunxi_wdt.c
> @@ -192,6 +192,16 @@ static int sunxi_wdt_start(struct watchdog_device *wdt_dev)
> return 0;
> }
>
> +static int sunxi_wdt_enabled(struct sunxi_wdt_dev *wdt)
Make it return bool.
> +{
> + u32 reg;
> + void __iomem *wdt_base = wdt->wdt_base;
> + const struct sunxi_wdt_reg *regs = wdt->wdt_regs;
> +
> + reg = readl(wdt_base + regs->wdt_mode);
> + return (reg & WDT_MODE_EN);
return !!(reg & WDT_MODE_EN);
> +}
> +
> static const struct watchdog_info sunxi_wdt_info = {
> .identity = DRV_NAME,
> .options = WDIOF_SETTIMEOUT |
> @@ -268,6 +278,11 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
> sunxi_wdt->wdt_dev.max_timeout = WDT_MAX_TIMEOUT;
> sunxi_wdt->wdt_dev.min_timeout = WDT_MIN_TIMEOUT;
> sunxi_wdt->wdt_dev.parent = dev;
> + if (sunxi_wdt_enabled(sunxi_wdt)) {
> + set_bit(WDOG_HW_RUNNING, &sunxi_wdt->wdt_dev.status);
Maybe also read back the current timeout? This should be done after
watchdog_init_timeout(), so ...
> + } else {
> + clear_bit(WDOG_HW_RUNNING, &sunxi_wdt->wdt_dev.status);
> + }
... you should just combine the if-else block with the one below.
>
> watchdog_init_timeout(&sunxi_wdt->wdt_dev, timeout, dev);
> watchdog_set_nowayout(&sunxi_wdt->wdt_dev, nowayout);
> @@ -275,7 +290,10 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
>
> watchdog_set_drvdata(&sunxi_wdt->wdt_dev, sunxi_wdt);
>
> - sunxi_wdt_stop(&sunxi_wdt->wdt_dev);
> + if (watchdog_hw_running(&sunxi_wdt->wdt_dev))
> + sunxi_wdt_start(&sunxi_wdt->wdt_dev);
> + else
> + sunxi_wdt_stop(&sunxi_wdt->wdt_dev);
FWIW this is what the dw_wdt driver does as well.
Thanks
ChenYu
> watchdog_stop_on_reboot(&sunxi_wdt->wdt_dev);
> err = devm_watchdog_register_device(dev, &sunxi_wdt->wdt_dev);
> --
> 2.25.1
>
On Thu, Feb 6, 2025 at 5:40 PM Regis Dargent <regis.dargent@...il.com> wrote:
>
> ---
> The sunxi_wdt watchdog unconditionally stops the watchdog during probe (on my Allwinner H616).
>
> What I want to achieve with this patch is to start the watchdog in the bootloader (either manually or automatically), then boot Linux.
> The watchdog is about 16sec timeout maximum, while the full boot to userland lasts about 90sec, and I want the board to reset if, eg.
> the rootfs cannot be mounted. So I need the watchdog to be handled by the kernel during boot (which it can do pretty well).
>
> The thing is, the current driver stops the watchdog during probe, so it does not run during boot, and it also does not manages the "status"
> field, so the kernel would know that it must handle the HW watchdog.
> This avoids automatic reboot in case a problem occurs during boot (and for example handling in the bootloader).
>
> The driver should detect if the HW watchdog is already running during probe and set its appropriate status bit to allow the kernel to handle the watchdog pings itself.
> The call to sunxi_wdt_start/stop allows for proper driver and device configuration.
> By default, the kernel will then ping the HW watchdog at apropriate frequency, but the user can have it stop after a time with open_timeout parameter.
>
> drivers/watchdog/sunxi_wdt.c | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
> index b85354a99582..20fe7da445ea 100644
> --- a/drivers/watchdog/sunxi_wdt.c
> +++ b/drivers/watchdog/sunxi_wdt.c
> @@ -192,6 +192,16 @@ static int sunxi_wdt_start(struct watchdog_device *wdt_dev)
> return 0;
> }
>
> +static int sunxi_wdt_enabled(struct sunxi_wdt_dev *wdt)
> +{
> + u32 reg;
> + void __iomem *wdt_base = wdt->wdt_base;
> + const struct sunxi_wdt_reg *regs = wdt->wdt_regs;
> +
> + reg = readl(wdt_base + regs->wdt_mode);
> + return (reg & WDT_MODE_EN);
> +}
> +
> static const struct watchdog_info sunxi_wdt_info = {
> .identity = DRV_NAME,
> .options = WDIOF_SETTIMEOUT |
> @@ -268,6 +278,11 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
> sunxi_wdt->wdt_dev.max_timeout = WDT_MAX_TIMEOUT;
> sunxi_wdt->wdt_dev.min_timeout = WDT_MIN_TIMEOUT;
> sunxi_wdt->wdt_dev.parent = dev;
> + if (sunxi_wdt_enabled(sunxi_wdt)) {
> + set_bit(WDOG_HW_RUNNING, &sunxi_wdt->wdt_dev.status);
> + } else {
> + clear_bit(WDOG_HW_RUNNING, &sunxi_wdt->wdt_dev.status);
> + }
>
> watchdog_init_timeout(&sunxi_wdt->wdt_dev, timeout, dev);
> watchdog_set_nowayout(&sunxi_wdt->wdt_dev, nowayout);
> @@ -275,7 +290,10 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
>
> watchdog_set_drvdata(&sunxi_wdt->wdt_dev, sunxi_wdt);
>
> - sunxi_wdt_stop(&sunxi_wdt->wdt_dev);
> + if (watchdog_hw_running(&sunxi_wdt->wdt_dev))
> + sunxi_wdt_start(&sunxi_wdt->wdt_dev);
> + else
> + sunxi_wdt_stop(&sunxi_wdt->wdt_dev);
>
> watchdog_stop_on_reboot(&sunxi_wdt->wdt_dev);
> err = devm_watchdog_register_device(dev, &sunxi_wdt->wdt_dev);
> --
> 2.25.1
>
Powered by blists - more mailing lists