On Sun, Feb 02, 2025 at 12:54:07PM -0800, Linus Torvalds wrote:
> 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.
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
