From: Takashi Iwai <tiwai@suse.de>
To: shuah <shuah@kernel.org>
Cc: Kees Cook <keescook@chromium.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Luis Chamberlain <mcgrof@kernel.org>,
"Rafael J . Wysocki" <rafael@kernel.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/5] firmware: Add support for loading compressed files
Date: Mon, 20 May 2019 20:07:25 +0200 [thread overview]
Message-ID: <s5h36l980f6.wl-tiwai@suse.de> (raw)
In-Reply-To: <5a3a0649-ece0-c3e3-3ebb-9d8d19d9499f@kernel.org>
On Mon, 20 May 2019 19:26:59 +0200,
shuah wrote:
>
> On 5/20/19 10:22 AM, Takashi Iwai wrote:
> > On Mon, 20 May 2019 17:18:48 +0200,
> > Takashi Iwai wrote:
> >>
> >> On Mon, 20 May 2019 16:39:37 +0200,
> >> Takashi Iwai wrote:
> >>>
> >>> On Mon, 20 May 2019 11:39:29 +0200,
> >>> Greg Kroah-Hartman wrote:
> >>>>
> >>>> On Mon, May 20, 2019 at 11:26:42AM +0200, Takashi Iwai wrote:
> >>>>> Hi,
> >>>>>
> >>>>> this is a patch set to add the support for loading compressed firmware
> >>>>> files.
> >>>>>
> >>>>> The primary motivation is to reduce the storage size; e.g. currently
> >>>>> the amount of /lib/firmware on my machine counts up to 419MB, and this
> >>>>> can be reduced to 130MB file compression. No bad deal.
> >>>>>
> >>>>> The feature adds only fallback to the compressed file, so it should
> >>>>> work as it was as long as the normal firmware file is present. The
> >>>>> f/w loader decompresses the content, so that there is no change needed
> >>>>> in the caller side.
> >>>>>
> >>>>> Currently only XZ format is supported. A caveat is that the kernel XZ
> >>>>> helper code supports only CRC32 (or none) integrity check type, so
> >>>>> you'll have to compress the files via xz -C crc32 option.
> >>>>>
> >>>>> The patch set begins with a few other improvements and refactoring,
> >>>>> followed by the compression support.
> >>>>>
> >>>>> In addition to this, dracut needs a small fix to deal with the *.xz
> >>>>> files.
> >>>>>
> >>>>> Also, the latest patchset is found in topic/fw-decompress branch of my
> >>>>> sound.git tree:
> >>>>> git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
> >>>>
> >>>> After a quick review, these all look good to me, nice job.
> >>>>
> >>>> One recommendation, can we add support for testing this to the
> >>>> tools/testing/selftests/firmware/ tests? And you did run those
> >>>> regression tests to verify that you didn't get any of the config options
> >>>> messed up, right? :)
> >>>
> >>> Now I've been testing the firmware selftest, and this turned out to be
> >>> surprisingly difficult on my system. By some reason, the test always
> >>> fails at the point triggering the request (line 58 of
> >>> fw_filesystem.sh):
> >>>
> >>> if ! echo -n "$NAME" >"$DIR"/trigger_request ; then
> >>> ....
> >>>
> >>> Judging from the strace output, this echo writes only the first byte
> >>> of $NAME. Then kernfs write op is invoked and it deals this one byte
> >>> input as if a whole argument were passed, leading to an error.
> >>>
> >>> My temporary workaround was to replace the all "echo" call with
> >>> "/usr/bin/echo".
> >>>
> >>> Then it hits a similar write error at the places like:
> >>>
> >>> echo 1 > $DIR/config_sync_direct
> >>>
> >>> This could be worked around by adding -n option to echo.
> >>>
> >>> Finally, I noticed that the user-fallback doesn't work on my system
> >>> any longer and the test stopped. This is expected, so it implies that
> >>> all direct loading tests passed.
> >>>
> >>> FWIW, my system is openSUSE Leap 15.1. Does anyone experience a
> >>> similar problem?
> >>
> >> This seems to be a regression on 5.2-rc1.
> >> The tests on 4.20 worked fine. 5.1 worked, but gave the error at
> >> fallback test instead of skipping. This is likely another regression,
> >> but irrelevant with the major issue as above.
> >>
> >> Now bisecting...
> >
> > Still in bisection, but it's timeout, I'll have to leave now...
> >
> > FWIW, the regression seems to have been introduced around the latest
> > kselftest merge.
> > The commit 4c7b63a32d54 is bad, and the commit 9cbda1bddb4c good.
> >
> > Shuah, could you check it?
> >
> >
>
> Kees,
>
> Could this be related to your selftest Makefile test run output
> refactoring work?
>
> I did a quick test on 5.2 after my first kselftest update without
> the refactor work - firmware test worked.
>
> I am thinking this is related to
>
> eff3ee303d0d
> selftests: Extract single-test shell logic from lib.mk
OK, I could identify the culprit commit. It's 5c069b6dedef
selftests: Move test output to diagnostic lines
It seems that stdbuf usage caused a regression, at least on my
system.
Takashi
>
> bash vs. sh difference in "echo" command behavior is the cause
> is my best guess. Will you be able to take a look at this?
>
> sudo make -C tools/testing/selftests/firmware/ run_tests
>
> will show the difference.
>
> thanks,
> -- Shuah
>
next prev parent reply other threads:[~2019-05-20 18:07 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-20 9:26 [PATCH 0/5] firmware: Add support for loading compressed files Takashi Iwai
2019-05-20 9:26 ` [PATCH 1/5] firmware: Free temporary page table after vmapping Takashi Iwai
2019-05-20 9:26 ` [PATCH 2/5] firmware: Unify the paged buffer release helper Takashi Iwai
2019-05-20 9:26 ` [PATCH 3/5] firmware: Use kvmalloc for page tables Takashi Iwai
2019-05-20 9:26 ` [PATCH 4/5] firmware: Factor out the paged buffer handling code Takashi Iwai
2019-05-20 9:26 ` [PATCH 5/5] firmware: Add support for loading compressed files Takashi Iwai
2019-05-20 9:39 ` [PATCH 0/5] " Greg Kroah-Hartman
2019-05-20 9:56 ` Takashi Iwai
2019-05-21 5:30 ` Takashi Iwai
2019-05-20 14:39 ` Takashi Iwai
2019-05-20 15:18 ` Takashi Iwai
2019-05-20 16:22 ` Takashi Iwai
2019-05-20 17:26 ` shuah
2019-05-20 18:07 ` Takashi Iwai [this message]
2019-05-20 18:59 ` Takashi Iwai
2019-05-20 21:50 ` Kees Cook
2019-05-28 5:25 ` Takashi Iwai
2019-06-10 17:21 ` Greg Kroah-Hartman
2019-06-10 17:30 ` Takashi Iwai
2019-06-10 17:48 ` Greg Kroah-Hartman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=s5h36l980f6.wl-tiwai@suse.de \
--to=tiwai@suse.de \
--cc=gregkh@linuxfoundation.org \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mcgrof@kernel.org \
--cc=rafael@kernel.org \
--cc=shuah@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).