Re: [PATCH] firmware: wake all waiters

[Linus Torvalds]

On Fri, Jun 23, 2017 at 4:37 PM, Jakub Kicinski
<jakub.kicinski@xxxxxxxxxxxxx> wrote:
> -               swake_up(&fw_st->wq);
> +               swake_up_all(&fw_st->wq);

Why is that code using the braindamaed "swait" model in the first place?

That's the real problem here - swait() is a very specialized
interface, and it does not make sense to use it here.

Among all the simplifications it has is exactly the fact that it wakes
up only one thing, because it is *so* specialized.

But the *only* reason for swait is extreme memory issues and some very
special realtime issues, where it saves a couple of bytes in
structures that need close packing, and doesn't even use normal
spinlocks, so it saves a couple of cycles at wakeup/sleep because it
doesn't do a good job in general.

The "avoid normal spinlocks" is because it is meant for code that is
*so* special that it needs the magical low-level raw spinlocks.

I really have *no* idea why the firmware code uses that idiotic
special wait-queue. It has no reason to do so, except this comment
from the commit that added it:

  "We use also swait instead of wait because don't need all the additional
    features wait provides."

which is bogus, since it clearly just got the waiting wrong exactly
*because* swait is pretty damn bad and specialized.

I think the two valid users are RCU (which needed it for RT), and kvm
(which also needed it for similar issues - it needs to be
non-preemptible).

I don't see any similar reason for the firmware loading, and all it
did was use an odd interface that resulted in this bug.

Why is the firmware code being so damn odd on purpose?

           Linus