linux-kernel - Re: [PATCH v2 0/3] add "delay" clock support to gpio

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, 8 Mar 2021 21:51:51 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>,
        Arnd Bergmann <arnd@...db.de>, Stephen Boyd <sboyd@...nel.org>
Cc:     Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org,
        linux-watchdog@...r.kernel.org
Subject: Re: [PATCH v2 0/3] add "delay" clock support to gpio_wdt

On 3/4/21 2:12 PM, Rasmus Villemoes wrote:
> As Arnd and Guenther suggested, this adds support to the gpio_wdt
> driver for being a consumer of the clock driving the ripple
> counter. However, I don't think it should be merged as-is, see below.
> 
> The first patch makes sense on its own, quick grepping suggests plenty
> of places that could benefit from this, and I thought it would be odd
> to have to re-introduce a .remove callback in the gpio_wdt driver.
> 

This has zero chance to be accepted. As suggested in the patch,
just use devm_add_action(), like many other watchdog drivers.

> Unfortunately, this turns out to be a bit of an "operation succeeded,
> patient (almost) died": We use CONFIG_GPIO_WATCHDOG_ARCH_INITCALL
> because the watchdog has a rather short timeout (1.0-2.25s, 1.6s
> typical according to data sheet). At first, I put the new code right
> after the devm_gpiod_get(), but the problem is that this early, we get
> -EPROBE_DEFER since the clock provider (the RTC which sits off i2c)
> isn't probed yet. But then the board would reset because it takes way
> too long for the rest of the machine to initialize. [The bootloader
> makes sure to turn on the RTC's clock output so the watchdog is
> actually functional, the task here is to figure out the proper way to
> prevent clk_disable_unused() from disabling it.]
> 

Is there a property indicating always-on for clocks, similar to
regulator-always-on ? The idea seems to exist, but it looks like
it is always explict (ie mentioned somewhere in the code that a clock
is always on, or "safe"). It would help if the clock in question
can be marked as always-on without explicit consumer.

Thanks,
Guenter

> Moving the logic to after the first "is it always-running and if so
> give it an initial ping" made the board survive, but unfortunately the
> second, and succesful, probe happens a little more than a second
> later, which happens to work on this particular board, but is
> obviously not suitable for production given that it's already above
> what the spec says, and other random changes in the future might make
> the gap even wider.
> 
> So I don't know. The hardware is obviously misdesigned, and I don't
> know how far the mainline kernel should stretch to support this; OTOH
> the kernel does contain lots of workarounds for quirks and hardware
> bugs. 
> 
> 
> 
> 
> Rasmus Villemoes (3):
>   clk: add devm_clk_prepare_enable() helper
>   dt-bindings: watchdog: add optional "delay" clock to gpio-wdt binding
>   watchdog: gpio_wdt: implement support for optional "delay" clock
> 
>  .../devicetree/bindings/watchdog/gpio-wdt.txt |  6 ++++
>  .../driver-api/driver-model/devres.rst        |  1 +
>  drivers/clk/clk-devres.c                      | 29 +++++++++++++++++++
>  drivers/watchdog/gpio_wdt.c                   |  9 ++++++
>  include/linux/clk.h                           | 13 +++++++++
>  5 files changed, 58 insertions(+)
>