On Wed, Jan 22, 2014 at 6:48 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > On Tue, 21 Jan 2014, Dan Williams wrote: > >> >> I think we agree that khubd needs to not look at the portstatus while >> >> the port is down. pm runtime synchronization takes care of that and >> >> prevents khubd from starting while the port is inactive. With that in >> >> place we can now make requests in usb_port_runtime_resume() that are >> >> later serviced in khubd when the port is marked active (pm runtime >> >> enforces that ->runtime_resume() return 0 before the port is marked >> >> active). >> > >> > How do you know, when you make the request, that khubd won't start >> > running while the port is still down? If that happened then khubd >> > would know to avoid looking at the port, of course, but the request >> > would be used up and lost. >> >> Per above it requires khubd to barrier pm operations to finish >> bringing the port up. It does this while holding a reference to >> ensure the port does not go down while checking. I also expect a >> reference to be taken when the request is queued and dropped when >> khubd services it. > > That's not what I meant. > > Sending a request to khubd means doing something like this: > > set_bit(port1, hub->resume_child_bits); > kick_khubd(hub); > > khubd may start running even before kick_khubd returns. If it does, it > will find that the port has not yet returned to power-on status. It handles that with pm_runtime_barrier(). When the request bit is set the port is in the RPM_RESUMING state. We hold a reference and khubd forces the port all the way to RPM_ACTIVE before checking the state and dropping references. > Then > it will skip the port, leaving the bit set in resume_child_bits, and go > on to look at all the other ports in this hub. > > Thus, when the port does finish powering up, khubd won't be running any > more. And there will be nothing to start it up, since kick_khubd has > already done its job. So the child won't be resumed in a timely > manner. > Unless I am missing a hole in pm_runtime_barrier, I do not believe this scenario can happen. >> > With a lock, this problem doesn't arise. >> >> A lock for this scenario would effectively duplicate dev->power.lock, >> or be wider than necessary if I understand where you are considering >> holding it. > > I'm not sure why you say that. We could use the new status_lock. > >> That said, I do believe you are still skeptical of pm runtime >> synchronization. > > A little bit. I'd feel better if you solve the problem outlined above. > > Also, I'm not sure what advantages PM synchronization has over a plain, > simple mutex. Earlier you said it was more robust, but that's not > clear to me. > I'm looking for khubd to be gated by a ref count. pm synchronization fits that bill. >> We could certainly take the lock when >> setting/clearing ->power_is_on, and disable khubd from processing >> !power_is_on ports. However, that solution needs a way to flush a >> currently running khubd, or prevent a port from suspending while khubd >> is active (here comes pm runtime sync again) > > Easy: khubd will hold the status_lock almost the whole time it is > running. (Not while calling pm_runtime_resume, usb_disconnect, or > usb_new_device, though.) Since usb_port_runtime_resume will also > acquire the lock, there won't be any interference. > Right, that works, but the pm sync let's us minimize where we hold the lock in khubd and we don't even need to worry about taking the lock in usb_port_runtime_{suspend|resume}. Minimizing where we need to take the lock is why I contend it's more robust to (ab)use pm state. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html