Re: question about firmware caching and relying on it (CONFIG_FW_CACHE)

[Linus Torvalds]

On Sun, 2 Feb 2025 at 12:15, Dave Airlie <airlied@xxxxxxxxx> wrote:
>
> Currently FW_CACHE is an optional feature (that distros may or may not
> configure off), where we will cache loaded firmwares to avoid problems
> over suspend/resume (and speed up resume).
>
> I've just discovered a problem in nouveau's suspend code when the
> FW_CACHE is turned off, it tries to load a few in it's suspend path
> for certain scenarios. Enabling FW_CACHE fixes this, but I'm unsure if
> that is considered properly fixing it or should FW_CACHE just be
> considered an optimisation.

Honestly, if the alternative is "driver hacks up its own caching",
then I think it definitely should just say "select FW_CACHE".

You need to make it conditional on PM_SLEEP to make Kconfig happy, but
arguably that acts as documentation (ie it kind of ends up being a
"this is *why* we select FW_CACHE").

So a simple

        select FW_CACHE if PM_SLEEP

sounds like the right thing to do for nouveau.

And in fact, that was how things used to be globally (with no driver
selection noise needed). There was no FW_CACHE option, we would just
enable it unconditionally for PM_SLEEP, exactly so that drivers would
*not* try to load firmware during resume when the filesystem may be
off-limits.

HOWEVER.

We apparently have some completely cray-cray "uevent messages" thing
going on, which caused commit 030cc787c30e ("firmware_class: make
firmware caching configurable")

Honestly, I'm not sure what broken thing that is all about. What
uevent messages? Firmwas caching should cause *less* uevent noise, not
more, because now we don't need to talk to user space as much.

Is that some Android-only thing, or is it some inherent stupidity in
the FW_CACHE code that I just don't see off-hand?

I do *not* see any real explanation for that commit, only that
statement about netlink that appears very odd.

That commit really makes me angry. It has that pattern that I
absolutely hate: no actual background, and a "Link:" that makes me
follow it ("Yay! Explanation!") that then only points back to the
patch submission ("#^&% this thing").

Useless. Annoying. I absolutely *detest* those links that give no
actual useful backstory to what the thing is about and only point back
to the patch that I'm wondering about. The disappointment it causes is
intense and real.

Anyway, I would say that particularly for a driver that wants caching,
adding that select is very much the right thing to do.

I'd rather get rid of that stupid config option entirely, but since I
don't know what the background is for it having been added, I guess we
can't do that.

Greg, Luis, can you explain that odd uevent message / netlink issue?

            Linus