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 > >
