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. 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. > > 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. > 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. Alan Stern -- 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
