Re: [PATCH RFC 2/3] Decompressors: Add boot-time XZ support
On 24/11/10 20:54, Lasse Collin wrote:
@@ -176,10 +179,20 @@ config KERNEL_LZMA
bool LZMA
depends on HAVE_KERNEL_LZMA
help
+ This is the predecessor of XZ.
+
You seem to have moved the help text from LZMA into the entry for XZ,
leaving LZMA merely with the observation it's the predecessor of XZ. I
think LZMA should keep it's help text describing what it is.
+config KERNEL_XZ
+ bool XZ
+ depends on HAVE_KERNEL_XZ
+ help
The most recent compression algorithm.
This sounds odd, The most recent compression algorithm to what? The
most recent compression algorithm in the world, the most recent open
source compression algorithm, or the most recently added compression
algorithm to the Linux kernel?
These statements can become quickly out of date, especially if they're
vague as to what they mean - if someone added another compression
algorithm to the Linux kernel in the future, should they change this
statement?
BTW I appreciate that you've merely kept the original poorly worded
description from the LZMA help text.
diff -uprN linux-2.6.37-rc3.orig/lib/decompress_unxz.c
linux-2.6.37-rc3/lib/decompress_unxz.c
--- linux-2.6.37-rc3.orig/lib/decompress_unxz.c 1970-01-01 02:00:00.0
+0200
+++ linux-2.6.37-rc3/lib/decompress_unxz.c 2010-11-24 18:18:37.0
+0200
@@ -0,0 +1,390 @@
+/*
+ * XZ decoder as a single file for uncompressing the kernel and initramfs
Does Single file XZ decoder for ... sound better?
+#ifndef memeq
+static bool memeq(const void *a, const void *b, size_t size)
+{
+ const uint8_t *x = a;
+ const uint8_t *y = b;
+ size_t i;
+
+ for (i = 0; i size; ++i)
The kernel uses either i size or isize ...
lots of these ...
+ if (x[i] != y[i])
+ return false;
+
+ return true;
+}
+#endif
+
+#ifndef memzero
+static void memzero(void *buf, size_t size)
+{
+ uint8_t *b = buf;
+ uint8_t *e = b + size;
New line here
+ while (b != e)
+ *b++ = '\0';
+}
+#endif
+
+/*
+ * This function implements the API defined inlinux/decompress/generic.h.
+ *
Not completely, see below. Your wrapper behaves correctly (bar one
respect) for the restricted use cases of initramfs/initrd, but for
other inputs it will behave differently to the other decompressors, and
differently to that described in generic.h, and it is broken for some
inputs.
Is this a problem? Depends on your viewpoint. One viewpoint is all
the decompressors/wrappers should behave the same (as realistically
possible) given the same inputs, so code can switch between compressors
and get the same behaviour. The other viewpoint is to just say that
the decompressors give the same behaviour for the restricted
inittramfs/initrd use cases and state all other usage is unpredictable.
+ if (in != NULL out != NULL)
+ s = xz_dec_init(XZ_SINGLE, 0);
+ else
+ s = xz_dec_init(XZ_DYNALLOC, (uint32_t)-1);
+
+ if (s == NULL)
+ goto error_alloc_state;
+
+ b.in = in;
+ b.in_pos = 0;
+ b.in_size = in_size;
+ b.out_pos = 0;
+
+ if (in_used != NULL)
+ *in_used = 0;
+
+ if (fill == NULL flush == NULL) {
Space before
+ b.out = out;
+ b.out_size = (size_t)-1;
+ ret = xz_dec_run(s,b);
+ } else {
+ b.out_size = XZ_IOBUF_SIZE;
+ b.out = malloc(XZ_IOBUF_SIZE);
You're assuming here that flush != NULL. The API as described in
generic.h allows for the situation where fill != NULL flush == NULL,
in which case out is assumed to be != NULL and large enough to hold the
entire output data without flushing.
From generic.h: If flush = NULL, outbuf must be large enough to buffer
all the expected output
+ if (b.out == NULL)
+ goto error_alloc_out;
+
+ if (fill != NULL) {
+ in = malloc(XZ_IOBUF_SIZE);
From generic.h: inbuf can be NULL, in which case the decompressor will
allocate the input buffer. If inbuf != NULL it must be at least
XXX_IOBUF_SIZE bytes. fill will be called (repeatedly...) to read data
If in != NULL, you'll discard the passed in buffer.
+ if (in == NULL)
+ goto error_alloc_in;
+
+ b.in = in;
+ }
+
+ do {
+ if (b.in_pos == b.in_size fill != NULL) {
Space before
If fill != NULL you're relying on the caller to have passed
in_size == 0, so first time around the loop the fill function is
called to fill your empty malloced buffer. If in_size is passed in
!= 0, you won't call fill and therefore you will pass an empty buffer
to the decompressor.
+ if (in_used != NULL)
+ *in_used += b.in_pos;
+
+
+
[OT] Display for an embedded device
Hi, I don't know if this is the proper place to ask, so if anyone knows a better place just tell me... Which kind of display devices are supported by the kernel? Which kind of tiny display devices do you recommend to use in a rackable PC? (it would be ideal to fit the display in the box of one rack unit) Thanks in advance, and sorry if the message if so off topic. Best regards, Natanael. -- To unsubscribe from this list: send the line unsubscribe linux-embedded in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/16 v4] pramfs: file operations
2010/11/24 Paul Mundt let...@linux-sh.org: On Wed, Nov 24, 2010 at 09:11:13AM +0100, Marco Stornelli wrote: 2010/11/24 Paul Mundt let...@linux-sh.org: most of this from ext2, I'm curious why you opted to hardcode this instead of maintaining the flexibility that ext2 XIP has over this. First of all because it was simpler :) In addition there was some design problem to use it in combination with the memory protection. Do you have more details on this? You can easily check for unsupportable configurations with mount options and bail out accordingly. The difference with ext2 is that we aren't talking about a general purpose fs used (mainly) on normal desktop/server systems, but a specific fs for embedded world, so I think some little constraints are ok. I'm not sure what your point is. It's not a general purpose file system, but that's not an excuse for taking shortcuts. Out of the boards on my desk, I have at least 3 that could make use of this file system where I could use both XIP and non-XIP for different stores out of the box. I wouldn't exactly call it a corner case. Also, as Tony's patch set demonstrates, these sorts of non-volatile data stores are common enough in the server space to make pramfs an option there, too. Please lose this mentality that because something was originally tasked for embedded it's perfectly acceptable to ship a crippled interface. I'm not responsible if someone uses something outside its scope, you can use FAT on a flash but you can't claim reliability. However since I'm a collaborative person I'll try to fix it, implementing where possible a mount option. As it is, this is something that will have to be rewritten one way or the other, but whether that happens in or out of staging/ is not such a big concern. Good. Marco -- To unsubscribe from this list: send the line unsubscribe linux-embedded in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/3] Decompressors: Add XZ decompressor module
On 2010-11-25 Andrew Morton wrote: On Wed, 24 Nov 2010 22:51:52 +0200 Lasse Collin lasse.col...@tukaani.org wrote: This patch: Add the main decompression code (xz_dec), testing module (xz_dec_test), wrapper script (xz_wrap.sh) for the xz command line tool, and documentation. The xz_dec module is enough to have a usable XZ decompressor e.g. for Squashfs. I'm not seeing any documentation which tells me how to create, install and execute xs-compressed kernels. There are new makefile targets? The last paragraph under XZ related components in the kernel in Documentation/xz.txt mentions xzkern and xzmisc commands available for makefiles. They are defined in scripts/Makefile.lib and have some comments as documentation there too. The Kconfig options to enable XZ-compressed kernel are added in the second patch. There is already an option to select the kernel compression method so I only added XZ to that list. I assume that people will find this option just like they have been able to find the other compression methods. +#define bcj_x86_test_msbyte(b) ((b) == 0x00 || (b) == 0xFF) This should be written in C. It looks nicer, and so bcj_x86_test_msbyte(*p++) won't explode. Thanks. It's fixed in XZ Embedded git repository now: git clone http://git.tukaani.org/xz-embedded.git (clone only, no WWW for now) I will post updated patches when needed. +static noinline_for_stack size_t bcj_x86( hm, but it uses little stack space. I wrote that code 19 months ago and now it looks odd to me too. I remember that my idea was to minimize stack usage in this call stack: xz_dec_run() - xz_dec_bcj_run() - xz_dec_lzma2_run() - ... I wanted to prevent the BCJ filters from being inlined into xz_dec_bcj_run() because inlining would have increased the maximum stack usage by 100-150 bytes. The BCJ filters are called via bcj_apply() which is called in two places from xz_dec_bcj_run(). Since there are two calls to bcj_apply(), GCC won't inline it. It will only inline the BCJ filters into bcj_apply() and that's OK. So noinline_for_stack does nothing good here. Maybe my early version had only one call to bcj_apply() or something like that. Anyway, I removed noinline_for_stacks. I tested it also when only one BCJ filter is enabled and with a few different GCC versions. +static noinline_for_stack size_t bcj_x86( + struct xz_dec_bcj *s, uint8_t *buf, size_t size) The preferred style is static noinline_for_stack size_t bcj_x86(struct xz_dec_bcj *s, uint8_t *buf, size_t size) or static noinline_for_stack size_t bcj_x86(struct xz_dec_bcj *s, uint8_t *buf, size_t size) (lots of dittoes) Fixed. -- Lasse Collin | IRC: Larhzu @ IRCnet Freenode -- To unsubscribe from this list: send the line unsubscribe linux-embedded in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 2/3] Decompressors: Add boot-time XZ support
On 2010-11-25 Phillip Lougher wrote:
On 24/11/10 20:54, Lasse Collin wrote:
@@ -176,10 +179,20 @@ config KERNEL_LZMA
bool LZMA
depends on HAVE_KERNEL_LZMA
help
+ This is the predecessor of XZ.
+
You seem to have moved the help text from LZMA into the entry for XZ,
leaving LZMA merely with the observation it's the predecessor of XZ.
I think LZMA should keep it's help text describing what it is.
OK. It needs some updating though with a separate patch. E.g. it
currently says decompression speed is between the other two while
there are already four supported kernel compression methods (LZO is
the fourth).
+config KERNEL_XZ
+ bool XZ
+ depends on HAVE_KERNEL_XZ
+ help
The most recent compression algorithm.
This sounds odd, The most recent compression algorithm to what?
Yes, it needs to be improved.
diff -uprN linux-2.6.37-rc3.orig/lib/decompress_unxz.c
linux-2.6.37-rc3/lib/decompress_unxz.c
--- linux-2.6.37-rc3.orig/lib/decompress_unxz.c 1970-01-01
02:00:00.0 +0200
+++ linux-2.6.37-rc3/lib/decompress_unxz.c2010-11-24 18:18:37.0
+0200
@@ -0,0 +1,390 @@
+/*
+ * XZ decoder as a single file for uncompressing the kernel and initramfs
Does Single file XZ decoder for ... sound better?
Thanks. I have fixed it in the git repository of XZ Embedded.
Nowadays decompress_unxz.c uses xz_dec module for initramfs and
initrd decompression, so it's not a single-file decompressor anymore.
Wrapper for decompressing XZ-compressed kernel, initramfs, and
initrd sounds more correct.
+#ifndef memzero
+static void memzero(void *buf, size_t size)
+{
+ uint8_t *b = buf;
+ uint8_t *e = b + size;
New line here
Fixed.
+/*
+ * This function implements the API defined inlinux/decompress/generic.h.
+ *
Not completely, see below. Your wrapper behaves correctly (bar one
respect) for the restricted use cases of initramfs/initrd, but for
other inputs it will behave differently to the other decompressors,
and differently to that described in generic.h, and it is broken for
some inputs.
There is a problem but I think it's a little more complex than it
seems at first. I asked Alain Knaff for clarification about the API
in 2009-05-26 (that discussion was CC'ed to H. Peter Anvin). I got
an excellent answer. I wrote decompress_unxz.c based on that, and
unxz() hasn't changed much since then.
I also wrote improved documentation for linux/decompress/generic.h
on the same day. I got some feedback about it from H. Peter Anvin and
I think the final version was good. Unfortunately the patch got
forgotten and didn't end up in Linux. I understood that Alain Knaff
was busy back then and I didn't pay attention to the patch later either.
Your documentation improvement to generic.h got into Linux in
2009-08-07.
Is this a problem? Depends on your viewpoint. One viewpoint is all
the decompressors/wrappers should behave the same (as realistically
possible) given the same inputs, so code can switch between
compressors and get the same behaviour. The other viewpoint is to
just say that the decompressors give the same behaviour for the
restricted inittramfs/initrd use cases and state all other usage is
unpredictable.
I think all decompressors should behave the same as long as the
specified API is followed, the rest being undefined. So my code is
broken with the current API specification from generic.h.
+ b.out = out;
+ b.out_size = (size_t)-1;
+ ret = xz_dec_run(s,b);
+ } else {
+ b.out_size = XZ_IOBUF_SIZE;
+ b.out = malloc(XZ_IOBUF_SIZE);
You're assuming here that flush != NULL. The API as described in
generic.h allows for the situation where fill != NULL flush ==
NULL, in which case out is assumed to be != NULL and large enough to
hold the entire output data without flushing.
From generic.h: If flush = NULL, outbuf must be large enough to
buffer all the expected output
The docs in generic.h allow it but I would like to get a
clarification if this is really what is wanted the API to be. Alain
Knaff said in 2009 that there are only three use cases:
- pre-boot (buffer to buffer)
- initramfs (buffer to callback)
- initrd (callback to callback)
In these cases it's impossible to have fill != NULL flush == NULL.
+ if (b.out == NULL)
+ goto error_alloc_out;
+
+ if (fill != NULL) {
+ in = malloc(XZ_IOBUF_SIZE);
From generic.h: inbuf can be NULL, in which case the decompressor
will allocate the input buffer. If inbuf != NULL it must be at
least XXX_IOBUF_SIZE bytes. fill will be called (repeatedly...) to
read data
If in != NULL, you'll discard the passed in buffer.
You are right again. On the other hand, Alain Knaff made it very
clear to me in 2009 that the situation you describe should not occur,
and indeed it does not occur
