Sorry, on the list to give this a closer look today- it looked nice on the surface and cleans up one annoyance I've had in the past with how this stuff works, but I want to be sure that we don't accidentally regress the serial bits.

In D56970#1310204, @kevans wrote:

Sorry, on the list to give this a closer look today- it looked nice on the surface and cleans up one annoyance I've had in the past with how this stuff works, but I want to be sure that we don't accidentally regress the serial bits.

Thanks. I haven't test on umb (maybe @khorben can help), ucom, and ugold device a I have none of them. I can wait further to find more testers.

In D56970#1310860, @adrian wrote:

So I'm starting to understand what you're trying to do here, however just a few things:

• this code gets used or was used in some bootloader builds, right? So we need to make sure it runs in its standalone mode versus in the kernel

Oh. I totally forget this. I will check if it works with our bootloader.

• I'm a fan of pushing serialising items into a taskqueue to ensure they get run in order, but you definitely need to make sure you pay attention to locking and draining needs
• I suggest thinking about what locks are and are not supposed to be held across schedule, drain, etc and put explicit lock assertions there

You are right. I will start to add more asserts tomorrow.

• the moment I see UNLOCK; xxx ; LOCK patterns I am immediately worried. We already have a whole class of problems with USB consumers because of this (there's a bug about USB ethernet NICs getting link flaps and it's due to unlock;X;relock in USB allowing other work to make forward progress, messing things up!) so you really REALLY need to avoid that.

UNLCOK; xxx; LOCK; only happens when we need to mwait something. Actually, the original mwait implementation do this implicitly as we are waiting something that also requires this lock.

• the point of bus lock ; mwait / signal ; unlock in the original code is rooted in making sure it's not racy with other things. You've changed it to do the signaling /outside/ of a lock, which can introduce timing races where something happens between your unlock and your signaling call.

I don't unlock the mtx when we only use signal. As you said, it breaks the synchronozation. Maybe I falsely unlock something when we only need to signal. I will carefully check again tomorrow.

So I'm going to request a whole big rethink about this. I like that you're trying to do work ordering using a taskqueue for work, but I'm very hesitant to see this happen given the common "racy" anti-patterns being introduced to work around locking issues (unlock; work; relock as a big example.)

My strategy for these modifications is that we initially held the lock implicitly, but now we need to acquire it explicitly.

For callbacks:

If the prologue already performs an unlock operation, I remove the unlock and do not add a new lock.
If the prologue does not unlock, I explicitly add the lock back.
If the epilogue performs a lock operation, I remove the lock; otherwise, I add an unlock.

For callers (e.g. msignal, mwait):

If only msignal is used, we do nothing — no unlock and no lock — which preserves the original ordering.
If msignal is followed by mwait, we unlock only if required. Since mwait immediately follows msignal, this should not break existing behavior.

The original implementation also had a limitation: it only accepted one (or two) types of events. As a result, a hotplug event had to enumerate the entire bus and fetch the port status for the bus and all child devices, which held the enumeration lock for a long time.

With this change, we can set up per-hub events and enumerate only the hub that triggered the hotplug interrupt.

ugh yeah you're right, our mwait() implementation does the same thing and has the same issues, doesn't it? I've come to increasingly not like that; code really needs to treat exiting the mwait() as "all state may have changed", and it's not really suitable for serialising stuff like usb state / transfers.

Want to take a look at the USB stack and bootloader stuff in particular? I'll go look at the unlock;XX;relock bit some more and see if there's something else which can be done or whether it needs to just be clearly commented in the function and whatever it is calling.