[RFC,0/3] watchdog servicing during decompression
mbox series

Message ID 20191017114906.30302-1-linux@rasmusvillemoes.dk
Headers show
Series
  • watchdog servicing during decompression
Related show

Message

Rasmus Villemoes Oct. 17, 2019, 11:49 a.m. UTC
Many custom boards have an always-running external watchdog
circuit. When the timeout of that watchdog is small, one cannot boot a
compressed kernel since the board gets reset before it even starts
booting the kernel proper.

One way around that is to do the decompression in a bootloader which
knows how to service the watchdog. However, one reason to prefer using
the kernel's own decompressor is to be able to take advantage of
future compression enhancements (say, a faster implementation of the
current method, or switching over when a new method such a zstd is
invented) - often, the bootloader cannot be updated without physical
access or is locked down for other reasons, so the decompressor has to
be bundled with the kernel image for that to be possible.

This POC adds a linux/decompress/keepalive.h header which provides a
decompress_keepalive() macro. Wiring up any given decompressor just
amounts to including that header and adding decompress_keepalive() in
the main loop - for simplicity, this series just does it for lz4.

The actual decompress_keepalive() implementation is of course very
board-specific. The third patch adds a kconfig knob that handles a
common case (and in fact suffices for all the various boards I've come
across): An external watchdog serviced by toggling a gpio, with the
value of that gpio being settable in a memory-mapped register.

Rasmus Villemoes (3):
  decompress/keepalive.h: prepare for watchdog keepalive during kernel
    decompression
  lib: lz4: wire up watchdog keepalive during decompression
  decompress/keepalive.h: add config option for toggling a set of bits

 include/linux/decompress/keepalive.h | 22 +++++++++++++++++++
 init/Kconfig                         | 33 ++++++++++++++++++++++++++++
 lib/lz4/lz4_decompress.c             |  2 ++
 3 files changed, 57 insertions(+)
 create mode 100644 include/linux/decompress/keepalive.h

Comments

Russell King - ARM Linux admin Oct. 17, 2019, 12:03 p.m. UTC | #1
We used to have this on ARM - it was called from the decompressor
code via an arch_decomp_wdog() hook.

That code got removed because it is entirely unsuitable for a multi-
platform kernel.  This looks like it takes an address for the watchdog
from the Kconfig, and builds that into the decompressor, making the
decompressor specific to that board or platform.

I'm not sure distros are going to like that given where we are with
multiplatform kernels.

On Thu, Oct 17, 2019 at 01:49:03PM +0200, Rasmus Villemoes wrote:
> Many custom boards have an always-running external watchdog
> circuit. When the timeout of that watchdog is small, one cannot boot a
> compressed kernel since the board gets reset before it even starts
> booting the kernel proper.
> 
> One way around that is to do the decompression in a bootloader which
> knows how to service the watchdog. However, one reason to prefer using
> the kernel's own decompressor is to be able to take advantage of
> future compression enhancements (say, a faster implementation of the
> current method, or switching over when a new method such a zstd is
> invented) - often, the bootloader cannot be updated without physical
> access or is locked down for other reasons, so the decompressor has to
> be bundled with the kernel image for that to be possible.
> 
> This POC adds a linux/decompress/keepalive.h header which provides a
> decompress_keepalive() macro. Wiring up any given decompressor just
> amounts to including that header and adding decompress_keepalive() in
> the main loop - for simplicity, this series just does it for lz4.
> 
> The actual decompress_keepalive() implementation is of course very
> board-specific. The third patch adds a kconfig knob that handles a
> common case (and in fact suffices for all the various boards I've come
> across): An external watchdog serviced by toggling a gpio, with the
> value of that gpio being settable in a memory-mapped register.
> 
> Rasmus Villemoes (3):
>   decompress/keepalive.h: prepare for watchdog keepalive during kernel
>     decompression
>   lib: lz4: wire up watchdog keepalive during decompression
>   decompress/keepalive.h: add config option for toggling a set of bits
> 
>  include/linux/decompress/keepalive.h | 22 +++++++++++++++++++
>  init/Kconfig                         | 33 ++++++++++++++++++++++++++++
>  lib/lz4/lz4_decompress.c             |  2 ++
>  3 files changed, 57 insertions(+)
>  create mode 100644 include/linux/decompress/keepalive.h
> 
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Rasmus Villemoes Oct. 17, 2019, 12:34 p.m. UTC | #2
On 17/10/2019 14.03, Russell King - ARM Linux admin wrote:
> We used to have this on ARM - it was called from the decompressor
> code via an arch_decomp_wdog() hook.
> 
> That code got removed because it is entirely unsuitable for a multi-
> platform kernel.  This looks like it takes an address for the watchdog
> from the Kconfig, and builds that into the decompressor, making the
> decompressor specific to that board or platform.
> 
> I'm not sure distros are going to like that given where we are with
> multiplatform kernels.

This is definitely not for multiplatform kernels or general distros,
it's for kernels that are built as part of a BSP for a specific board -
hence the "Say N unless you know you need this.".

I didn't know it used to exist. But I do know that something like this
is carried out-of-tree for lots of boards, so I thought I'd try to get
upstream support.

The first two patches, or something like them, would be nice on their
own, as that would minimize the conflicts when forward-porting the
board-specific patch. But such a half-implemented feature that requires
out-of-tree patches to actually do anything useful of course won't fly.

I'm not really a big fan of the third patch, even though it does work
for all the cases I've encountered so far - I'm sure there would be
boards where a much more complicated solution would be needed. Another
method I thought of was to just supply a __weak no-op
decompress_keepalive(), and then have a config option to point at an
extra object file to link into the compressed/vmlinux (similar to the
initramfs_source option that also points to some external resource).

But if the mainline kernel doesn't want anything like this
re-introduced, that's also fine, that just means a bit of job security.

Rasmus
Russell King - ARM Linux admin Oct. 17, 2019, 12:48 p.m. UTC | #3
On Thu, Oct 17, 2019 at 02:34:52PM +0200, Rasmus Villemoes wrote:
> On 17/10/2019 14.03, Russell King - ARM Linux admin wrote:
> > We used to have this on ARM - it was called from the decompressor
> > code via an arch_decomp_wdog() hook.
> > 
> > That code got removed because it is entirely unsuitable for a multi-
> > platform kernel.  This looks like it takes an address for the watchdog
> > from the Kconfig, and builds that into the decompressor, making the
> > decompressor specific to that board or platform.
> > 
> > I'm not sure distros are going to like that given where we are with
> > multiplatform kernels.
> 
> This is definitely not for multiplatform kernels or general distros,
> it's for kernels that are built as part of a BSP for a specific board -
> hence the "Say N unless you know you need this.".
> 
> I didn't know it used to exist. But I do know that something like this
> is carried out-of-tree for lots of boards, so I thought I'd try to get
> upstream support.

Sorry, it does still exist, just been moved around a bit.

See lib/inflate.c:

STATIC int INIT inflate(void)
{
...
#ifdef ARCH_HAS_DECOMP_WDOG
    arch_decomp_wdog();
#endif

Given that it still exists, maybe this hook name should be used for
this same issue in the LZ4 code?

> The first two patches, or something like them, would be nice on their
> own, as that would minimize the conflicts when forward-porting the
> board-specific patch. But such a half-implemented feature that requires
> out-of-tree patches to actually do anything useful of course won't fly.
> 
> I'm not really a big fan of the third patch, even though it does work
> for all the cases I've encountered so far - I'm sure there would be
> boards where a much more complicated solution would be needed. Another
> method I thought of was to just supply a __weak no-op
> decompress_keepalive(), and then have a config option to point at an
> extra object file to link into the compressed/vmlinux (similar to the
> initramfs_source option that also points to some external resource).
> 
> But if the mainline kernel doesn't want anything like this
> re-introduced, that's also fine, that just means a bit of job security.

Well, we'll see whether the arm-soc people have anything to add...
Linus Walleij Oct. 24, 2019, 12:38 p.m. UTC | #4
On Thu, Oct 17, 2019 at 2:34 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On 17/10/2019 14.03, Russell King - ARM Linux admin wrote:
> > We used to have this on ARM - it was called from the decompressor
> > code via an arch_decomp_wdog() hook.
> >
> > That code got removed because it is entirely unsuitable for a multi-
> > platform kernel.  This looks like it takes an address for the watchdog
> > from the Kconfig, and builds that into the decompressor, making the
> > decompressor specific to that board or platform.
> >
> > I'm not sure distros are going to like that given where we are with
> > multiplatform kernels.

That's a very good point.

What we have for debug UART etc is explicitly just for
debugging on one specific platform and not for production
code.

But as pointed out there is code like this already.

> This is definitely not for multiplatform kernels or general distros,
> it's for kernels that are built as part of a BSP for a specific board -
> hence the "Say N unless you know you need this.".

Not much to do about that, we need to support it already and
adding another usecase just makes it more reasonable to
support I think.

What we need to think about is whether we can imagine some
solution that would work with multiplatform.

At one point we discussed putting some easily accessible
values in the device tree for the "decompressing...." message,
so easy to get at that the decompressor could access them
easily, or even providing a small binary code snippet in the
DTB file to write to the UART. None of this worked out
IIUC.

I think nothing really materialized from this and the problem
is swept under the carpet: no decompress messages for
multiplatform. I tried to think about something and just feel
I would be reinventing mach-types.

Do we have an idea of whether it is possible to dig into
a DTB in early boot and find the node for the UART and
watchdog and use the physical address from there?
Is it really hard or is it just that no-one tried?
(Sorry if this is a naive question...)

Yours,
Linus Walleij