Re: Is request_firmware() really safe to call in resume callback when /usr/lib/firmware is on btrfs?

[Lukas Middendorf <kernel@tuxforce.de>]

Great to hear that you now succeeded in reproducing the problem.

On 03/04/2021 22:25, Luis Chamberlain wrote:
> On Sat, Apr 03, 2021 at 12:24:07PM +0200, Lukas Middendorf wrote:
>> One further thing I noticed which might be problematic in rare cases:
>> According to the kernel debug messages, the firmware-loader does not attempt
>> to cache the firmware during suspend,
> 
> Correct, the goal with this test was to purposely *not* call for the
> firmware prior to suspend, and instead *race* (do the wrong thing) at
> resume. It shouldn't really stall... fail yes, but stall, that seems
> fishy.

I understood that.
>> if the previous call to
>> request_firmware() has failed (file not present; call made during previous
>> resume). In my opinion it should attempt to cache the firmware on suspend
>> even in this case
> 
> Yes, that is the *proper* way to do use the firmware API, but we want to
> reproduce the stall, and so we have to recreate the issue you reported
> by doing something bad.

Should firmware_request_cache() always be called, or only instead of 
request_firmware() ? In case request_firmware is called on its own, and 
the firmware file is not present, it might still race on the next resume.
request_firmware seems to be meant to include the cache request for the 
next suspend, but it apparently *does not* if firmware loading fails due 
to a missing file. I think this is something that should either be 
changed or properly documented in the API documentation.


>> Did you also try to create a random test-firmware.bin (I used 1M from
>> /dev/urandom) instead of an empty /lib/firmware ?
> 
> No, right now I want to to just focus on fixing the stall you saw.

This is a second nuance of the stall I saw: The firmware file (in this 
case test-firmware.bin) is present but not in cache.

If you run

ls -lR /lib/firmware > /dev/null

before

systemctl suspend

your reproduction steps will very likely not stall.
If you in addition run

dd if=/dev/urandom of=/lib/firmware/test-firmware.bin bs=1K count=64

during the preparation of /lib/firmware (in addition to or instead of 
your for loop), then it will always stall (as long as the file content 
is not read before suspend).

One stall is happening while the directory content is being listed (to 
find that the file is not present), the second stall is happening while 
actually trying to read the file. Those two stalls likely have the same 
root cause, but it might be different. I just want to make sure you have 
covered all cases.


>>> You might be better off just reposting your
>>> patches with the respective Reviewed-by tags and pestering your
>>> maintainer.
>>
>> I will try to be a little bit more insistent this time. Is "just repost" the
>> usual way to handle if patches are ignored?
> 
> You can repost, v2 just add my reviewed-by. Those patches are indeed
> correct as they are calling for the firmware prior to resume. Maybe
> clarify that.

OK, will do.

> But indeed there is also another issue which you reported which needs to
> be fixed, and for that thanks so much for you patience! I'll be looking
> into this!

Also thank you for your effort.

Lukas