Re: [PATCH v3 03/10] usb: find internal hub tier mismatch via acpi

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux