question about firmware caching and relying on it (CONFIG_FW_CACHE)

question about firmware caching and relying on it (CONFIG_FW_CACHE)

[Dave Airlie]

Hi,

I'm asking to see if there is any chance of consensus on having a
driver rely on (select) FW_CACHE. Nobody currently does.

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.

Do I
a) select FW_CACHE in nouveau Kconfig
b) fix nouveau to avoid loading the fw at suspend time (effectively
caching it itself?)

Dave.

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@gmail.com> 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

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

[Greg Kroah-Hartman]

On Sun, Feb 02, 2025 at 12:54:07PM -0800, Linus Torvalds wrote:
> On Sun, 2 Feb 2025 at 12:15, Dave Airlie <airlied@gmail.com> 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.

The "Link:" there is to the original patch submission, so yes, it's
useless in that point, but it can find the original patch which is why
we add them in this case, because many times the original patch did have
discussions on it before it was accepted.

> 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?

There was reports from Android devices that the uevent was causing the
system to wake up from the netlink messages that were sent when going to
sleep and so it would get caught in a loop and never actually go to
sleep.  I can't remember any more than that, maybe Mark can recall the
specifics and dig up the Android bug reports for it?

thanks,

greg k-h

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

[Luis Chamberlain]

On Mon, Feb 03, 2025 at 09:19:59AM +0100, Greg Kroah-Hartman wrote:
> On Sun, Feb 02, 2025 at 12:54:07PM -0800, Linus Torvalds wrote:
> > Greg, Luis, can you explain that odd uevent message / netlink issue?
> 
> There was reports from Android devices that the uevent was causing the
> system to wake up from the netlink messages that were sent when going to
> sleep and so it would get caught in a loop and never actually go to
> sleep.  I can't remember any more than that, maybe Mark can recall the
> specifics and dig up the Android bug reports for it?

Nope, the only thing that my spidy senses tells me is that it would be
great to know if the uevent flood was caused by the uevent flood which
caused the same udev duplicate messages to load modules per-cpu. Linus
had a nice fix to make these idempotent via commit 9b9879fc03275ffe
("modules: catch concurrent module loads, treat them as idempotent")
on the module load path, so I'm wondering if uvents could likely could
trigger floods to delays suspend.

Provided Mark Salyzyn hasn't moved on (from commit 030cc787c30e
("firmware_class: make firmware caching configurable") it would
be great if he could enable the FW_CACHE and re-test. At the very least
hopefully kernel-team-android can redirect this to the appropriate folks
to verify.

One of the reasons to *not* want the fw cache is for firmware images
which are *huge*, like those which may be used on remote procs. But we
we already have an internal FW_OPT_NOCACHE and remote-proc stuff likely
already uses request_firmware_into_buf() which uses FW_OPT_NOCACHE.

  Luis