| Message ID | 20230418140817.3651909-3-Basavaraj.Natikar@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Handle PM events for pci resume | expand |
On Tue, Apr 18, 2023 at 07:38:17PM +0530, Basavaraj Natikar wrote: > xHC system resume time may increase due to a 120ms delay. I'm sorry, but I can not understand this sentence. Is the delay 120ms too long? Or too short? Or will it change to be always at least 120ms? Or something else? > A PME# signal > will trigger the xHC host to resume runtime, and the host must wait for a > missed U3 LFPS wake signal before re-checking for port activity. It may > be necessary to wait only for auto-resume cases. Add a check only for > runtime resume to avoid the delay for other PM events so that the resume > time can be improved. I also can not understand these last two sentences, can you try to reword it a bit differently? thanks, greg k-h
On 4/18/2023 8:55 PM, Greg KH wrote: > On Tue, Apr 18, 2023 at 07:38:17PM +0530, Basavaraj Natikar wrote: >> xHC system resume time may increase due to a 120ms delay. > I'm sorry, but I can not understand this sentence. Is the delay 120ms > too long? Or too short? Or will it change to be always at least 120ms? > Or something else? Each USB controller while xhci_resumes by default takes 120 ms if there is no activity on the ports or no ports connected. Therefore, if there are more USB controllers on the system, 120 ms per controller will add delay to system resume. This patch will prevent 120 ms for each XHCI controller during system resume. > >> A PME# signal >> will trigger the xHC host to resume runtime, and the host must wait for a >> missed U3 LFPS wake signal before re-checking for port activity. It may >> be necessary to wait only for auto-resume cases. Add a check only for >> runtime resume to avoid the delay for other PM events so that the resume >> time can be improved. Once XHCI controller in runtime suspended state (D3), on USB device hotplug controller will resume (D0) and check for pending port events if no events, wait for 120 ms to re-check for port activity i.e. the first wake signalling failed is handled in below changes. https://lore.kernel.org/all/20210311115353.2137560-3-mathias.nyman@linux.intel.com/ This check adds delay to system resume case as well, which may be only required for runtime resume (auto resume) case. So added an extra check to handle the runtime resume case only. Will retry to reword accordingly as above. If you have any input, please let me know. Thanks, -- Basavaraj > I also can not understand these last two sentences, can you try to > reword it a bit differently? > > thanks, > > greg k-h
> 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?
Also, the commit title could specifically mention "system resume".
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. 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. > > Also, the commit title could specifically mention "system resume". Sure will change title "Improve the XHCI system resume time" Thanks, -- Basavaraj
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. > > 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. Thanks -Mathias
Hi Mathias, On 4/24/2023 8:05 AM, 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. > >> >> 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 Thanks for bringing up this topic Basavaraj. Sorry for jumping into this thread, but was looking to optimize this resume timing as well, since it is affecting some of the host driven bus resume situations. Just had a quick question about where the 120ms delay is required... From what I'm gathering from the USB3 spec, the 120ms timeout is the recommended time for tU3WakeupRetryDelay ("Table 7-12. LTSSM State Transition Timeouts"). This is the retry time that the device will wait before re-issuing another (potential) LFPS U3 wake. My idea was to see if we could limit this delay only for when a SSUSB device is already connected to the root hub. (ignore if HSUSB device connected) We would be able to eliminate the delay for: 1. No device connected to root hub 2. Only HSUSB device connected Is that a possibility we can add on top of what Basavaraj is adding? Thanks Wesley Cheng
On 25.4.2023 3.09, Wesley Cheng wrote: > Hi Mathias, > > On 4/24/2023 8:05 AM, 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. >> >>> >>> 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 > > Thanks for bringing up this topic Basavaraj. > > Sorry for jumping into this thread, but was looking to optimize this resume timing as well, since it is affecting some of the host driven bus resume situations. Just had a quick question about where the 120ms delay is required... > > From what I'm gathering from the USB3 spec, the 120ms timeout is the recommended time for tU3WakeupRetryDelay ("Table 7-12. LTSSM State Transition Timeouts"). This is the retry time that the device will wait before re-issuing another (potential) LFPS U3 wake. > > My idea was to see if we could limit this delay only for when a SSUSB device is already connected to the root hub. (ignore if HSUSB device connected) We would be able to eliminate the delay for: > 1. No device connected to root hub > 2. Only HSUSB device connected > > Is that a possibility we can add on top of what Basavaraj is adding? > Sounds reasonable, Yes the 120ms was intended for the U3 wake delay for SuperSpeed devices. We should probably also check for CAS bit in xhci_pending_portevent() (I'll add that CAS check) -Mathias
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 > >
Hi Mathias, On 4/25/2023 2:04 AM, Mathias Nyman wrote: > On 25.4.2023 3.09, Wesley Cheng wrote: >> Hi Mathias, >> >> On 4/24/2023 8:05 AM, 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. >>> >>>> >>>> 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 >> >> Thanks for bringing up this topic Basavaraj. >> >> Sorry for jumping into this thread, but was looking to optimize this >> resume timing as well, since it is affecting some of the host driven >> bus resume situations. Just had a quick question about where the >> 120ms delay is required... >> >> From what I'm gathering from the USB3 spec, the 120ms timeout is the >> recommended time for tU3WakeupRetryDelay ("Table 7-12. LTSSM State >> Transition Timeouts"). This is the retry time that the device will >> wait before re-issuing another (potential) LFPS U3 wake. >> >> My idea was to see if we could limit this delay only for when a SSUSB >> device is already connected to the root hub. (ignore if HSUSB device >> connected) We would be able to eliminate the delay for: >> 1. No device connected to root hub >> 2. Only HSUSB device connected >> >> Is that a possibility we can add on top of what Basavaraj is adding? >> > > Sounds reasonable, > Yes the 120ms was intended for the U3 wake delay for SuperSpeed devices. > > We should probably also check for CAS bit in xhci_pending_portevent() > (I'll add that CAS check) > Thanks for the info. I'll make a change to add the checks I mentioned above and submit it as a separate patch for review. Thanks Wesley Cheng
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index a539e4dd54f7..a3ee80ee5d1e 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -1315,7 +1315,7 @@ int xhci_resume(struct xhci_hcd *xhci, int event) * the first wake signalling failed, give it that chance. */ pending_portevent = xhci_pending_portevent(xhci); - if (!pending_portevent) { + if (!pending_portevent && event == PM_EVENT_AUTO_RESUME) { msleep(120); pending_portevent = xhci_pending_portevent(xhci); }
xHC system resume time may increase due to a 120ms delay. A PME# signal will trigger the xHC host to resume runtime, and the host must wait for a missed U3 LFPS wake signal before re-checking for port activity. It may be necessary to wait only for auto-resume cases. Add a check only for runtime resume to avoid the delay for other PM events so that the resume time can be improved. Fixes: 253f588c70f6 ("xhci: Improve detection of device initiated wake signal.") Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> --- drivers/usb/host/xhci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)