linux-kernel - Re: [PATCH v2 6/6] clk: renesas: r9a06g032: Disable the watchdog reset sources when halting

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]

Date:   Mon, 14 Feb 2022 11:45:36 +0100
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Jean-Jacques Hiblot <jjhiblot@...phandler.com>
Cc:     linux-renesas-soc@...r.kernel.org,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Guenter Roeck <linux@...ck-us.net>,
        linux-watchdog@...r.kernel.org
Subject: Re: [PATCH v2 6/6] clk: renesas: r9a06g032: Disable the watchdog
 reset sources when halting

Hi Jean-Jacques,

CC watchdog people, who only received some patches.

On Tue, Feb 8, 2022 at 7:35 PM Jean-Jacques Hiblot
<jjhiblot@...phandler.com> wrote:
> The watchdog reset sources must be disabled when the system is halted.
> Otherwise the watchdogs will trigger a reset if they have been armed.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@...phandler.com>

Thanks for your patch!

[inserting changelog]

| Changes v1 -> v2:
| * Modified the clock driver to not enable the watchdog reset sources.
|   On other renesas platforms, those bits are by the bootloader. The
|   watchdog reset sources are still disabled when the platform is halted
|   to prevent a watchdog reset.

I still have my doubts about this part. So on halt, you override the
policy configured by the boot loader, which means the watchdog is no
longer triggered on halt.

>From a system perspective, the system can be in five states:
  1. Running,
  2. Crashed/lock-ed up,
  3. Halt,
  4. Reboot,
  5. Poweroff.

Now, from a policy perspective, what is the difference between a
system that crashes or locks up, and a system that halts?
I.e. should the system reboot on halt, or not?

I think halting a system where the watchdog has been activated makes
no sense, and the user either wants to explicitly reboot the system, or
power it off, but never halt it.  So I think this patch is not needed.

Watchdog people: what is your opinion?
Thanks!

> --- a/drivers/clk/renesas/r9a06g032-clocks.c
> +++ b/drivers/clk/renesas/r9a06g032-clocks.c
> @@ -129,6 +129,11 @@ enum { K_GATE = 0, K_FFC, K_DIV, K_BITSEL, K_DUALGATE };
>
>  #define R9A06G032_CLOCK_COUNT          (R9A06G032_UART_GROUP_34567 + 1)
>
> +#define R9A06G032_SYSCTRL_REG_RSTEN            0x120
> +#define WDA7RST1       BIT(2)
> +#define WDA7RST0       BIT(1)
> +#define MRESET         BIT(0)
> +
>  static const struct r9a06g032_clkdesc r9a06g032_clocks[] = {
>         D_ROOT(CLKOUT, "clkout", 25, 1),
>         D_ROOT(CLK_PLL_USB, "clk_pll_usb", 12, 10),
> @@ -893,6 +898,19 @@ static void r9a06g032_clocks_del_clk_provider(void *data)
>         of_clk_del_provider(data);
>  }
>
> +static void r9a06g032_reset_sources(struct r9a06g032_priv *clocks,
> +                       uint32_t mask, uint32_t value)
> +{
> +       uint32_t rsten;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&clocks->lock, flags);
> +       rsten = readl(clocks->reg);
> +       rsten = (rsten & ~mask) | (value & mask);
> +       writel(rsten, clocks->reg + R9A06G032_SYSCTRL_REG_RSTEN);
> +       spin_unlock_irqrestore(&clocks->lock, flags);
> +}
> +
>  static int __init r9a06g032_clocks_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
> @@ -910,6 +928,8 @@ static int __init r9a06g032_clocks_probe(struct platform_device *pdev)
>         if (!clocks || !clks)
>                 return -ENOMEM;
>
> +       platform_set_drvdata(pdev, clocks);
> +
>         spin_lock_init(&clocks->lock);
>
>         clocks->data.clks = clks;
> @@ -963,9 +983,18 @@ static int __init r9a06g032_clocks_probe(struct platform_device *pdev)
>         if (error)
>                 return error;
>
> +
>         return r9a06g032_add_clk_domain(dev);
>  }
>
> +static void r9a06g032_clocks_shutdown(struct platform_device *pdev)
> +{
> +       struct r9a06g032_priv *clocks = platform_get_drvdata(pdev);
> +
> +       /* Disable the watchdog reset sources */
> +       r9a06g032_reset_sources(clocks, WDA7RST0 | WDA7RST1, 0);
> +}
> +
>  static const struct of_device_id r9a06g032_match[] = {
>         { .compatible = "renesas,r9a06g032-sysctrl" },
>         { }
> @@ -976,6 +1005,7 @@ static struct platform_driver r9a06g032_clock_driver = {
>                 .name   = "renesas,r9a06g032-sysctrl",
>                 .of_match_table = r9a06g032_match,
>         },
> +       .shutdown = r9a06g032_clocks_shutdown,
>  };
>
>  static int __init r9a06g032_clocks_init(void)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds