| Message ID | 20191017114906.30302-1-linux@rasmusvillemoes.dk (mailing list archive) |
|---|---|
| Headers | show |
| Series | watchdog servicing during decompression | expand |
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 >
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
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...
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
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