[RFC PATCH 0/3] watchdog servicing during decompression

[RFC PATCH 0/3] watchdog servicing during decompression

[Rasmus Villemoes]

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

[RFC PATCH 0/3] watchdog servicing during decompression

[Rasmus Villemoes]

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

[RFC PATCH 1/3] decompress/keepalive.h: prepare for watchdog keepalive during kernel decompression

[Rasmus Villemoes]

Some boards have a hardware watchdog that (a) cannot be disabled
and (b) has a timeout short enough that there's no chance for the
kernel to get through decompression, let alone reach the
initialization of the appropriate watchdog device driver.

In order to allow booting such boards, the decompression routine needs
to service the watchdog in its main loop. This adds a header making it
easy to wire up each decompressor - just include this header and add a
decompress_keepalive() in the main loop.

Outside of the pre-boot stage, this is always a no-op.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/decompress/keepalive.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 include/linux/decompress/keepalive.h

diff --git a/include/linux/decompress/keepalive.h b/include/linux/decompress/keepalive.h
new file mode 100644
index 000000000000..39caa7693624
--- /dev/null
+++ b/include/linux/decompress/keepalive.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef DECOMPRESS_KEEPALIVE_H
+#define DECOMPRESS_KEEPALIVE_H
+
+#ifdef PREBOOT
+
+#endif
+
+#ifndef decompress_keepalive
+#define decompress_keepalive() do { } while (0)
+#endif
+
+#endif /* DECOMPRESS_KEEPALIVE_H */
-- 
2.20.1

[RFC PATCH 1/3] decompress/keepalive.h: prepare for watchdog keepalive during kernel decompression

[Rasmus Villemoes]

Some boards have a hardware watchdog that (a) cannot be disabled
and (b) has a timeout short enough that there's no chance for the
kernel to get through decompression, let alone reach the
initialization of the appropriate watchdog device driver.

In order to allow booting such boards, the decompression routine needs
to service the watchdog in its main loop. This adds a header making it
easy to wire up each decompressor - just include this header and add a
decompress_keepalive() in the main loop.

Outside of the pre-boot stage, this is always a no-op.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/decompress/keepalive.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 include/linux/decompress/keepalive.h

diff --git a/include/linux/decompress/keepalive.h b/include/linux/decompress/keepalive.h
new file mode 100644
index 000000000000..39caa7693624
--- /dev/null
+++ b/include/linux/decompress/keepalive.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef DECOMPRESS_KEEPALIVE_H
+#define DECOMPRESS_KEEPALIVE_H
+
+#ifdef PREBOOT
+
+#endif
+
+#ifndef decompress_keepalive
+#define decompress_keepalive() do { } while (0)
+#endif
+
+#endif /* 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

[RFC PATCH 2/3] lib: lz4: wire up watchdog keepalive during decompression

[Rasmus Villemoes]

Some boards have a hardware watchdog that (a) cannot be disabled
and (b) has a timeout short enough that there's no chance for the
kernel to get through decompression, let alone reach the
initialization of the appropriate watchdog device driver.

In order to allow booting such boards, the decompression routine needs
to service the watchdog in its main loop. This wires up lz4 to do that
via the decompress_keepalive() macro defined in the new
linux/decompress/keepalive.h.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/lz4/lz4_decompress.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/lz4/lz4_decompress.c b/lib/lz4/lz4_decompress.c
index 0c9d3ad17e0f..54ba41d073a6 100644
--- a/lib/lz4/lz4_decompress.c
+++ b/lib/lz4/lz4_decompress.c
@@ -39,6 +39,7 @@
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <asm/unaligned.h>
+#include <linux/decompress/keepalive.h>
 
 /*-*****************************
  *	Decompression functions
@@ -129,6 +130,7 @@ static FORCE_INLINE int LZ4_decompress_generic(
 
 		/* ip < iend before the increment */
 		assert(!endOnInput || ip <= iend);
+		decompress_keepalive();
 
 		/*
 		 * A two-stage shortcut for the most common case:
-- 
2.20.1

[RFC PATCH 2/3] lib: lz4: wire up watchdog keepalive during decompression

[Rasmus Villemoes]

Some boards have a hardware watchdog that (a) cannot be disabled
and (b) has a timeout short enough that there's no chance for the
kernel to get through decompression, let alone reach the
initialization of the appropriate watchdog device driver.

In order to allow booting such boards, the decompression routine needs
to service the watchdog in its main loop. This wires up lz4 to do that
via the decompress_keepalive() macro defined in the new
linux/decompress/keepalive.h.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/lz4/lz4_decompress.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/lz4/lz4_decompress.c b/lib/lz4/lz4_decompress.c
index 0c9d3ad17e0f..54ba41d073a6 100644
--- a/lib/lz4/lz4_decompress.c
+++ b/lib/lz4/lz4_decompress.c
@@ -39,6 +39,7 @@
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <asm/unaligned.h>
+#include <linux/decompress/keepalive.h>
 
 /*-*****************************
  *	Decompression functions
@@ -129,6 +130,7 @@ static FORCE_INLINE int LZ4_decompress_generic(
 
 		/* ip < iend before the increment */
 		assert(!endOnInput || ip <= iend);
+		decompress_keepalive();
 
 		/*
 		 * A two-stage shortcut for the most common case:
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH 3/3] decompress/keepalive.h: add config option for toggling a set of bits

[Rasmus Villemoes]

It's quite common to have an external watchdog which is serviced via
flipping a GPIO, with the value of that GPIO being settable directly
in a memory-mapped register. So add kconfig options defining the
physical address of such a register as well as a mask to have the
decompressor periodically xor into that register.

If and when other decompress_keepalive methods are added, this can be
made into a "choice DECOMPRESS_KEEPALIVE".

Since only LZ4 is wired up currently, this is "depends on KERNEL_LZ4"
for now. Also, prevent this option from being shown to the average
user.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/decompress/keepalive.h |  8 +++++++
 init/Kconfig                         | 33 ++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/include/linux/decompress/keepalive.h b/include/linux/decompress/keepalive.h
index 39caa7693624..c62e49bee7cf 100644
--- a/include/linux/decompress/keepalive.h
+++ b/include/linux/decompress/keepalive.h
@@ -5,6 +5,14 @@
 
 #ifdef PREBOOT
 
+#ifdef CONFIG_DECOMPRESS_KEEPALIVE_TOGGLE
+#define decompress_keepalive() do {					\
+		long addr = CONFIG_DECOMPRESS_KEEPALIVE_TOGGLE_REG;	\
+		volatile unsigned *reg = (volatile unsigned *)addr;	\
+		*reg ^= CONFIG_DECOMPRESS_KEEPALIVE_TOGGLE_MASK;	\
+} while (0)
+#endif
+
 #endif
 
 #ifndef decompress_keepalive
diff --git a/init/Kconfig b/init/Kconfig
index b4daad2bac23..8a894d9fdd77 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -289,6 +289,39 @@ config KERNEL_UNCOMPRESSED
 
 endchoice
 
+config DECOMPRESS_KEEPALIVE_TOGGLE
+	depends on KERNEL_LZ4
+	depends on EXPERT
+	bool "Toggle some bits while decompressing"
+	help
+	  Some embedded boards have an always-running hardware
+	  watchdog with a timeout short enough that the board is reset
+	  during decompression, thus preventing the board from ever
+	  booting.
+
+	  Enable this to toggle certain bits in a memory register
+	  while decompressing the kernel. This can for example be used
+	  in the common case of an external watchdog serviced via a
+	  memory-mapped GPIO.
+
+	  Say N unless you know you need this.
+
+if DECOMPRESS_KEEPALIVE_TOGGLE
+
+config DECOMPRESS_KEEPALIVE_TOGGLE_REG
+	hex "Address of register to modify while decompressing"
+	help
+	  Set this to a physical address of a 32-bit memory word to
+	  modify while decompressing.
+
+config DECOMPRESS_KEEPALIVE_TOGGLE_MASK
+	hex "Bit mask to toggle while decompressing"
+	help
+	  The register selected above will periodically be xor'ed with
+	  this value during decompression.
+
+endif
+
 config DEFAULT_HOSTNAME
 	string "Default hostname"
 	default "(none)"
-- 
2.20.1

[RFC PATCH 3/3] decompress/keepalive.h: add config option for toggling a set of bits

[Rasmus Villemoes]

It's quite common to have an external watchdog which is serviced via
flipping a GPIO, with the value of that GPIO being settable directly
in a memory-mapped register. So add kconfig options defining the
physical address of such a register as well as a mask to have the
decompressor periodically xor into that register.

If and when other decompress_keepalive methods are added, this can be
made into a "choice DECOMPRESS_KEEPALIVE".

Since only LZ4 is wired up currently, this is "depends on KERNEL_LZ4"
for now. Also, prevent this option from being shown to the average
user.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/decompress/keepalive.h |  8 +++++++
 init/Kconfig                         | 33 ++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/include/linux/decompress/keepalive.h b/include/linux/decompress/keepalive.h
index 39caa7693624..c62e49bee7cf 100644
--- a/include/linux/decompress/keepalive.h
+++ b/include/linux/decompress/keepalive.h
@@ -5,6 +5,14 @@
 
 #ifdef PREBOOT
 
+#ifdef CONFIG_DECOMPRESS_KEEPALIVE_TOGGLE
+#define decompress_keepalive() do {					\
+		long addr = CONFIG_DECOMPRESS_KEEPALIVE_TOGGLE_REG;	\
+		volatile unsigned *reg = (volatile unsigned *)addr;	\
+		*reg ^= CONFIG_DECOMPRESS_KEEPALIVE_TOGGLE_MASK;	\
+} while (0)
+#endif
+
 #endif
 
 #ifndef decompress_keepalive
diff --git a/init/Kconfig b/init/Kconfig
index b4daad2bac23..8a894d9fdd77 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -289,6 +289,39 @@ config KERNEL_UNCOMPRESSED
 
 endchoice
 
+config DECOMPRESS_KEEPALIVE_TOGGLE
+	depends on KERNEL_LZ4
+	depends on EXPERT
+	bool "Toggle some bits while decompressing"
+	help
+	  Some embedded boards have an always-running hardware
+	  watchdog with a timeout short enough that the board is reset
+	  during decompression, thus preventing the board from ever
+	  booting.
+
+	  Enable this to toggle certain bits in a memory register
+	  while decompressing the kernel. This can for example be used
+	  in the common case of an external watchdog serviced via a
+	  memory-mapped GPIO.
+
+	  Say N unless you know you need this.
+
+if DECOMPRESS_KEEPALIVE_TOGGLE
+
+config DECOMPRESS_KEEPALIVE_TOGGLE_REG
+	hex "Address of register to modify while decompressing"
+	help
+	  Set this to a physical address of a 32-bit memory word to
+	  modify while decompressing.
+
+config DECOMPRESS_KEEPALIVE_TOGGLE_MASK
+	hex "Bit mask to toggle while decompressing"
+	help
+	  The register selected above will periodically be xor'ed with
+	  this value during decompression.
+
+endif
+
 config DEFAULT_HOSTNAME
 	string "Default hostname"
 	default "(none)"
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/3] watchdog servicing during decompression

[Russell King - ARM Linux admin]

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
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC PATCH 0/3] watchdog servicing during decompression

[Russell King - ARM Linux admin]

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
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/3] watchdog servicing during decompression

[Rasmus Villemoes]

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

Re: [RFC PATCH 0/3] watchdog servicing during decompression

[Rasmus Villemoes]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/3] watchdog servicing during decompression

[Russell King - ARM Linux admin]

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...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC PATCH 0/3] watchdog servicing during decompression

[Russell King - ARM Linux admin]

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...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 3/3] decompress/keepalive.h: add config option for toggling a set of bits

[Linus Walleij]

On Thu, Oct 17, 2019 at 1:49 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:

> +config DECOMPRESS_KEEPALIVE_TOGGLE_REG
> +       hex "Address of register to modify while decompressing"
> +       help
> +         Set this to a physical address of a 32-bit memory word to
> +         modify while decompressing.
> +
> +config DECOMPRESS_KEEPALIVE_TOGGLE_MASK
> +       hex "Bit mask to toggle while decompressing"
> +       help
> +         The register selected above will periodically be xor'ed with
> +         this value during decompression.

I would not allow users to store these vital hex values in their
defconfig and other unsafe places. Instead follow the pattern from
arch/arm/Kconfig.debug for storing the DEBUG_UART_PHYS:

config DEBUG_UART_PHYS
        hex "Physical base address of debug UART"
        default 0x01c20000 if DEBUG_DAVINCI_DMx_UART0
        default 0x01c28000 if DEBUG_SUNXI_UART0
        default 0x01c28400 if DEBUG_SUNXI_UART1
....
i.e. make sure to provide the right default values. We probably
need at least one example for others to follow.

Maybe this is your plan, I don't know, wanted to point it out
anyways.

Yours,
Linus Walleij

Re: [RFC PATCH 3/3] decompress/keepalive.h: add config option for toggling a set of bits

[Linus Walleij]

On Thu, Oct 17, 2019 at 1:49 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:

> +config DECOMPRESS_KEEPALIVE_TOGGLE_REG
> +       hex "Address of register to modify while decompressing"
> +       help
> +         Set this to a physical address of a 32-bit memory word to
> +         modify while decompressing.
> +
> +config DECOMPRESS_KEEPALIVE_TOGGLE_MASK
> +       hex "Bit mask to toggle while decompressing"
> +       help
> +         The register selected above will periodically be xor'ed with
> +         this value during decompression.

I would not allow users to store these vital hex values in their
defconfig and other unsafe places. Instead follow the pattern from
arch/arm/Kconfig.debug for storing the DEBUG_UART_PHYS:

config DEBUG_UART_PHYS
        hex "Physical base address of debug UART"
        default 0x01c20000 if DEBUG_DAVINCI_DMx_UART0
        default 0x01c28000 if DEBUG_SUNXI_UART0
        default 0x01c28400 if DEBUG_SUNXI_UART1
....
i.e. make sure to provide the right default values. We probably
need at least one example for others to follow.

Maybe this is your plan, I don't know, wanted to point it out
anyways.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/3] watchdog servicing during decompression

[Linus Walleij]

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

Re: [RFC PATCH 0/3] watchdog servicing during decompression

[Linus Walleij]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 3/3] decompress/keepalive.h: add config option for toggling a set of bits

[Rasmus Villemoes]

On 24/10/2019 14.17, Linus Walleij wrote:
> On Thu, Oct 17, 2019 at 1:49 PM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
> 
>> +config DECOMPRESS_KEEPALIVE_TOGGLE_REG
>> +       hex "Address of register to modify while decompressing"
>> +       help
>> +         Set this to a physical address of a 32-bit memory word to
>> +         modify while decompressing.
>> +
>> +config DECOMPRESS_KEEPALIVE_TOGGLE_MASK
>> +       hex "Bit mask to toggle while decompressing"
>> +       help
>> +         The register selected above will periodically be xor'ed with
>> +         this value during decompression.
> 
> I would not allow users to store these vital hex values in their
> defconfig and other unsafe places. Instead follow the pattern from
> arch/arm/Kconfig.debug for storing the DEBUG_UART_PHYS:
> 
> config DEBUG_UART_PHYS
>         hex "Physical base address of debug UART"
>         default 0x01c20000 if DEBUG_DAVINCI_DMx_UART0
>         default 0x01c28000 if DEBUG_SUNXI_UART0
>         default 0x01c28400 if DEBUG_SUNXI_UART1
> ....
> i.e. make sure to provide the right default values. We probably
> need at least one example for others to follow.
>
> Maybe this is your plan, I don't know, wanted to point it out
> anyways.

The thing is, there is no proper default value for the use cases I have
in mind: Custom hardware based on some SOC, where the designer has wired
on an external gpio-triggered watchdog. That could be gpio 25 of gpio
bank 0, or gpio 2 of gpio bank 3, or ... so I don't see how there could
possibly be any sane default value - the kernel certainly shouldn't grow
a config option for every single custom board out there.

That's why this is different from the previously existing
arch_decomp_wdog - that was (AFAICT) about feeding the SOC's builtin
watchdog.

I realize this is rather specific, and the current implementation for
example won't work if the gpio value cannot be toggled in such a simple
way (perhaps there are separate "set" and "clear" registers or whatnot)
- but as I said, it is sufficient for the many different cases I've seen
so far (and something like my patches have been used for years on those
boards).

An alternative is to simply provide a complete implementation of
decompress_keepalive() (or arch_decomp_wdog if we want to keep that
name) in an external .o/.c/.S file, and do something like

OBJS += $(CONFIG_DECOMP_WDOG:"%"=%)

in arch/foo/boot/compressed/Makefile. Then the physical address etc. do
not get written in Kconfig, and it should work for all cases, including
the ones that need to write 0x55, 0xaa, 0x12 in order to some
SOC-specific register.

Rasmus

Re: [RFC PATCH 3/3] decompress/keepalive.h: add config option for toggling a set of bits

[Rasmus Villemoes]

On 24/10/2019 14.17, Linus Walleij wrote:
> On Thu, Oct 17, 2019 at 1:49 PM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
> 
>> +config DECOMPRESS_KEEPALIVE_TOGGLE_REG
>> +       hex "Address of register to modify while decompressing"
>> +       help
>> +         Set this to a physical address of a 32-bit memory word to
>> +         modify while decompressing.
>> +
>> +config DECOMPRESS_KEEPALIVE_TOGGLE_MASK
>> +       hex "Bit mask to toggle while decompressing"
>> +       help
>> +         The register selected above will periodically be xor'ed with
>> +         this value during decompression.
> 
> I would not allow users to store these vital hex values in their
> defconfig and other unsafe places. Instead follow the pattern from
> arch/arm/Kconfig.debug for storing the DEBUG_UART_PHYS:
> 
> config DEBUG_UART_PHYS
>         hex "Physical base address of debug UART"
>         default 0x01c20000 if DEBUG_DAVINCI_DMx_UART0
>         default 0x01c28000 if DEBUG_SUNXI_UART0
>         default 0x01c28400 if DEBUG_SUNXI_UART1
> ....
> i.e. make sure to provide the right default values. We probably
> need at least one example for others to follow.
>
> Maybe this is your plan, I don't know, wanted to point it out
> anyways.

The thing is, there is no proper default value for the use cases I have
in mind: Custom hardware based on some SOC, where the designer has wired
on an external gpio-triggered watchdog. That could be gpio 25 of gpio
bank 0, or gpio 2 of gpio bank 3, or ... so I don't see how there could
possibly be any sane default value - the kernel certainly shouldn't grow
a config option for every single custom board out there.

That's why this is different from the previously existing
arch_decomp_wdog - that was (AFAICT) about feeding the SOC's builtin
watchdog.

I realize this is rather specific, and the current implementation for
example won't work if the gpio value cannot be toggled in such a simple
way (perhaps there are separate "set" and "clear" registers or whatnot)
- but as I said, it is sufficient for the many different cases I've seen
so far (and something like my patches have been used for years on those
boards).

An alternative is to simply provide a complete implementation of
decompress_keepalive() (or arch_decomp_wdog if we want to keep that
name) in an external .o/.c/.S file, and do something like

OBJS += $(CONFIG_DECOMP_WDOG:"%"=%)

in arch/foo/boot/compressed/Makefile. Then the physical address etc. do
not get written in Kconfig, and it should work for all cases, including
the ones that need to write 0x55, 0xaa, 0x12 in order to some
SOC-specific register.

Rasmus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel