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
