The original code did this without the lock held; the patch does it while holding the lock. Why the difference?

I removed the mtx_lock()/unlock() at the beginning of the function, which synchronizes with the msleep() in firmware_get(). A plain lock/unlock looks kind of strange, and this reduces the number of lock acquisitions in the common case (though that doesn't really matter). Both versions are correct.

Yeah, the back-to-back lock/unlock was... odd.

But, why was that necessary to synchronize with the msleep()? Wouldn't the actual meaningful lock and unlock have done that?

In any case, I'm still not clear on why the mutex needs to be held around the wakeup_one() now, when it didn't before.

Indeed, it's not strictly necessary to hold the lock across the wakeup_one() call. Acquiring and releasing the firmware mutex once in each path through loadimage() is enough: once loadimage() acquires the mutex, we know that firmware_get() is sleeping, so there is no possibility of lost wakeups. I did it this way to make it a bit more clear that the mutex is used to synchronize the wakeup as well as protect access to the firmware table.