Re: [PATCH 2/2] xhci: Improve the XHCI resume time

[Basavaraj Natikar <bnatikar@amd.com>]

On 4/24/2023 8:35 PM, Mathias Nyman wrote:
> On 21.4.2023 7.58, Basavaraj Natikar wrote:
>>
>> On 4/20/2023 10:33 PM, Mark Hasemeyer wrote:
>>>> It may be necessary to wait only for auto-resume cases.
>>> I find this comment misleading as the patch assumes that it's only
>>> necessary to
>>> wait for auto-resume cases. Are there any cases where the driver
>>> should wait
>>> during system-resume?
>>
>> Only in case of auto-resume (runtime resume).
>>
>> Rewording the commit message as follows.
>
> Thanks for fixing this extra system resume delay
>
> Maybe some kind of big picture explanation could be added to the
> commit message,
> such as:
>
> Avoid extra 120ms delay during system resume.
>
> xHC controller may signal wake up to 120ms before it shows which USB
> device
> caused the wake on the xHC port registers.
>
> The xhci driver therefore checks for port activity up to 120ms during
> resume,
> making sure that the hub driver can see the port change, and won't
> immediately
> runtime suspend back due to no port activity.
>
> This is however only needed for runtime resume as system resume will
> resume
> all child hubs and other child usb devices anyway.

Thanks for the explanation. I will change the commit message as stated above.

>
>>
>> Each XHCI controller while xhci_resumes by default takes 120 ms more if
>> there is no activity on the ports or no ports connected. Therefore, if
>> there are more USB controllers on the system, 120 ms more per controller
>> will add delay to system resume from suspended states like s2idle, S3 or
>> S4 states.
>>
>> Once the XHCI controller is in runtime suspended state (D3 state), on
>> USB
>> device hotplug controller will runtime resume (D0 state) and check for
>> pending port events if no events, wait for 120 ms to re-check for port
>> activity to handle missed wake signal.
>>
>> A delay of 120 ms more to re-check for port activity is needed only in
>> auto-resume (runtime resume) cases. Hence, add a check only for runtime
>> resume from runtime suspend (D3->D0) to avoid the 120ms more delay for
>> other PM events (system resume from suspend states like s2idle, S3 or S4
>> states) so that the system resume time can be improved.
>>
>> Please let me know if any inputs.
>
> I can only think of one minor side-effect that would be runtime
> suspending back
> too early after system resume. This could happen when connecting the
> first
> usb device to a roothub on a (system) suspended setup?
>
> steps:
> 1. in system suspend, no usb devices connected, xhci in D3, can signal
> wake with PME#
> 2. connect first usb device, xHC signals PME# wake
> 3. system resumes, xhci resumes to D0, but no actity visible on xHC
> port registers
> 4. rootubs resumes, no other children on this bus.
> 5. roothubs sees no activity (due to 120ms max latency before visible
> on port registers)
> 6. roothubs runtime suspend
> 7. xhci runtime suspends
> 8. same device now causes xHC to PME# wake again,
> 9. runtime reusume xhci, do wait 120ms for port activity
> 10. see port activity, resume hub, enumerate device.
>
> It might be that this really isn't an issue due to the the graceperiod
> fix:
>
> 33e321586e37 xhci: Add grace period after xHC start to prevent
> premature runtime suspend.

Yes correct above changes helps to prevent premature runtime suspend.

Thanks,
--
Basavaraj 

>
> Thanks
> -Mathias
>
>