Re: [PATCH 0/5] firmware: Add support for loading compressed files

[Takashi Iwai]

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
>