From: Anshuman Gupta <[email protected]>
This patch will improve the variable auto-resume latency of an usb-port.
When xhci gets a port status change event interrupt due to PORT_PLC
(port link state transition), linux Host controller driver drives the
resume signalling on the bus for the amount of time defined by
USB_REUME_TIMEOUT(40ms) macro.
This 40ms delay for resume signalling is in acceptable limit, but
it get worse when xhci goes for polling mode in order to detect other
events on its ports and modify rh_timer timer with a variable time out of
1ms to (HZ/4)ms.
drivers/usb/core/hcd.c line 799
mod_timer (&hcd->rh_timer, (jiffies/(HZ/4) + 1) * (HZ/4)).
Due to above variable timeout usb auto-resume latency varies from
40ms to ~300ms.
Log Snippet:
~128ms latency
[ 53.112049] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0000
[ 53.229200] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0004
[ 53.240177] usb 1-2: usb wakeup-resume
[ 53.240195] usb 1-2: finish resume
[ 53.240357] usb usb1-port2: resume, status 0
-----------------------------------------------------------------
~300ms latency
[ 59.946620] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0000
[ 59.979341] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0000
[ 60.229342] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0004
[ 60.251321] usb 1-2: usb wakeup-resume
[ 60.251335] usb 1-2: finish resume
[ 60.251539] usb usb1-port2: resume, status 0
This variable resume latency can be optimized, as in case of PORT_PLC
change event rh_timer has already been modified with USB_RESUME_TIMEOUT
(40ms) delay,leaving the rest to GetPortStatus and started polling for
root hub status (invoking usb_hcd_poll_rh_status).
We can avoid polling as we have already modified rh_timer with
delay of 40ms.
This patch set the HCD_FLAG_POLL_RH to hcd->flags after modification of
rh_timer, and avoids polling of root hub status. so rh_timer can fire
after 40ms and usb device auto-resuem latency will be around 40ms.
Signed-off-by: Anshuman Gupta <[email protected]>
---
drivers/usb/host/xhci-ring.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index cc368ad..3b7349a 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1677,9 +1677,14 @@ static void handle_port_status(struct xhci_hcd *xhci,
bus_state->resume_done[faked_port_index] = jiffies +
msecs_to_jiffies(USB_RESUME_TIMEOUT);
set_bit(faked_port_index, &bus_state->resuming_ports);
+ /* Do the rest in GetPortStatus after resume time delay.
+ * Avoid polling roothub status before that so that a
+ * usb device auto-resume latency around ~40ms.
+ */
+ set_bit(HCD_FLAG_POLL_RH, &hcd->flags);
mod_timer(&hcd->rh_timer,
bus_state->resume_done[faked_port_index]);
- /* Do the rest in GetPortStatus */
+ bogus_port_status = true;
}
}
--
2.7.4
On 23.08.2017 09:18, [email protected] wrote:
> From: Anshuman Gupta <[email protected]>
>
> This patch will improve the variable auto-resume latency of an usb-port.
>
> When xhci gets a port status change event interrupt due to PORT_PLC
> (port link state transition), linux Host controller driver drives the
> resume signalling on the bus for the amount of time defined by
> USB_REUME_TIMEOUT(40ms) macro.
>
> This 40ms delay for resume signalling is in acceptable limit, but
> it get worse when xhci goes for polling mode in order to detect other
> events on its ports and modify rh_timer timer with a variable time out of
> 1ms to (HZ/4)ms.
>
> drivers/usb/core/hcd.c line 799
> mod_timer (&hcd->rh_timer, (jiffies/(HZ/4) + 1) * (HZ/4)).
>
> Due to above variable timeout usb auto-resume latency varies from
> 40ms to ~300ms.
>
> Log Snippet:
> ~128ms latency
> [ 53.112049] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0000
> [ 53.229200] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0004
> [ 53.240177] usb 1-2: usb wakeup-resume
> [ 53.240195] usb 1-2: finish resume
> [ 53.240357] usb usb1-port2: resume, status 0
> -----------------------------------------------------------------
> ~300ms latency
> [ 59.946620] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0000
> [ 59.979341] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0000
> [ 60.229342] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0004
> [ 60.251321] usb 1-2: usb wakeup-resume
> [ 60.251335] usb 1-2: finish resume
> [ 60.251539] usb usb1-port2: resume, status 0
>
> This variable resume latency can be optimized, as in case of PORT_PLC
> change event rh_timer has already been modified with USB_RESUME_TIMEOUT
> (40ms) delay,leaving the rest to GetPortStatus and started polling for
> root hub status (invoking usb_hcd_poll_rh_status).
> We can avoid polling as we have already modified rh_timer with
> delay of 40ms.
>
> This patch set the HCD_FLAG_POLL_RH to hcd->flags after modification of
> rh_timer, and avoids polling of root hub status. so rh_timer can fire
> after 40ms and usb device auto-resuem latency will be around 40ms.
>
> Signed-off-by: Anshuman Gupta <[email protected]>
> ---
Thanks,
adding and sending forward after 4.14-rc1
-Mathias
On Wed, 23 Aug 2017, Mathias Nyman wrote:
> On 23.08.2017 09:18, [email protected] wrote:
> > From: Anshuman Gupta <[email protected]>
> >
> > This patch will improve the variable auto-resume latency of an usb-port.
> >
> > When xhci gets a port status change event interrupt due to PORT_PLC
> > (port link state transition), linux Host controller driver drives the
> > resume signalling on the bus for the amount of time defined by
> > USB_REUME_TIMEOUT(40ms) macro.
> >
> > This 40ms delay for resume signalling is in acceptable limit, but
> > it get worse when xhci goes for polling mode in order to detect other
> > events on its ports and modify rh_timer timer with a variable time out of
> > 1ms to (HZ/4)ms.
> >
> > drivers/usb/core/hcd.c line 799
> > mod_timer (&hcd->rh_timer, (jiffies/(HZ/4) + 1) * (HZ/4)).
> >
> > Due to above variable timeout usb auto-resume latency varies from
> > 40ms to ~300ms.
> >
> > Log Snippet:
> > ~128ms latency
> > [ 53.112049] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0000
> > [ 53.229200] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0004
> > [ 53.240177] usb 1-2: usb wakeup-resume
> > [ 53.240195] usb 1-2: finish resume
> > [ 53.240357] usb usb1-port2: resume, status 0
> > -----------------------------------------------------------------
> > ~300ms latency
> > [ 59.946620] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0000
> > [ 59.979341] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0000
> > [ 60.229342] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0004
> > [ 60.251321] usb 1-2: usb wakeup-resume
> > [ 60.251335] usb 1-2: finish resume
> > [ 60.251539] usb usb1-port2: resume, status 0
> >
> > This variable resume latency can be optimized, as in case of PORT_PLC
> > change event rh_timer has already been modified with USB_RESUME_TIMEOUT
> > (40ms) delay,leaving the rest to GetPortStatus and started polling for
> > root hub status (invoking usb_hcd_poll_rh_status).
> > We can avoid polling as we have already modified rh_timer with
> > delay of 40ms.
> >
> > This patch set the HCD_FLAG_POLL_RH to hcd->flags after modification of
> > rh_timer, and avoids polling of root hub status. so rh_timer can fire
> > after 40ms and usb device auto-resuem latency will be around 40ms.
> >
> > Signed-off-by: Anshuman Gupta <[email protected]>
> > ---
>
> Thanks,
> adding and sending forward after 4.14-rc1
This patch description doesn't make sense. If you want to avoid
resetting the root-hub timer by 250 ms, you should _clear_ the
HCD_FLAG_POLL_RH flag, not _set_ it! See this (slightly tricky) code
in usb_hcd_poll_rh_status():
if (hcd->uses_new_polling ? HCD_POLL_RH(hcd) :
(length == 0 && hcd->status_urb != NULL))
mod_timer (&hcd->rh_timer, (jiffies/(HZ/4) + 1) * (HZ/4));
Since xhci-hcd does set the uses_new_polling flag, the "if" condition
just tests the HCD_FLAG_POLL_RH bit.
Alan Stern
On 23.08.2017 17:30, Alan Stern wrote:
> On Wed, 23 Aug 2017, Mathias Nyman wrote:
>
>> On 23.08.2017 09:18, [email protected] wrote:
>>> From: Anshuman Gupta <[email protected]>
>>>
>>> This patch will improve the variable auto-resume latency of an usb-port.
>>>
>>> When xhci gets a port status change event interrupt due to PORT_PLC
>>> (port link state transition), linux Host controller driver drives the
>>> resume signalling on the bus for the amount of time defined by
>>> USB_REUME_TIMEOUT(40ms) macro.
>>>
>>> This 40ms delay for resume signalling is in acceptable limit, but
>>> it get worse when xhci goes for polling mode in order to detect other
>>> events on its ports and modify rh_timer timer with a variable time out of
>>> 1ms to (HZ/4)ms.
>>>
>>> drivers/usb/core/hcd.c line 799
>>> mod_timer (&hcd->rh_timer, (jiffies/(HZ/4) + 1) * (HZ/4)).
>>>
>>> Due to above variable timeout usb auto-resume latency varies from
>>> 40ms to ~300ms.
>>>
>>> Log Snippet:
>>> ~128ms latency
>>> [ 53.112049] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0000
>>> [ 53.229200] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0004
>>> [ 53.240177] usb 1-2: usb wakeup-resume
>>> [ 53.240195] usb 1-2: finish resume
>>> [ 53.240357] usb usb1-port2: resume, status 0
>>> -----------------------------------------------------------------
>>> ~300ms latency
>>> [ 59.946620] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0000
>>> [ 59.979341] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0000
>>> [ 60.229342] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0004
>>> [ 60.251321] usb 1-2: usb wakeup-resume
>>> [ 60.251335] usb 1-2: finish resume
>>> [ 60.251539] usb usb1-port2: resume, status 0
>>>
>>> This variable resume latency can be optimized, as in case of PORT_PLC
>>> change event rh_timer has already been modified with USB_RESUME_TIMEOUT
>>> (40ms) delay,leaving the rest to GetPortStatus and started polling for
>>> root hub status (invoking usb_hcd_poll_rh_status).
>>> We can avoid polling as we have already modified rh_timer with
>>> delay of 40ms.
>>>
>>> This patch set the HCD_FLAG_POLL_RH to hcd->flags after modification of
>>> rh_timer, and avoids polling of root hub status. so rh_timer can fire
>>> after 40ms and usb device auto-resuem latency will be around 40ms.
>>>
>>> Signed-off-by: Anshuman Gupta <[email protected]>
>>> ---
>>
>> Thanks,
>> adding and sending forward after 4.14-rc1
>
> This patch description doesn't make sense. If you want to avoid
> resetting the root-hub timer by 250 ms, you should _clear_ the
> HCD_FLAG_POLL_RH flag, not _set_ it! See this (slightly tricky) code
> in usb_hcd_poll_rh_status():
>
> if (hcd->uses_new_polling ? HCD_POLL_RH(hcd) :
> (length == 0 && hcd->status_urb != NULL))
> mod_timer (&hcd->rh_timer, (jiffies/(HZ/4) + 1) * (HZ/4));
>
> Since xhci-hcd does set the uses_new_polling flag, the "if" condition
> just tests the HCD_FLAG_POLL_RH bit.
The description could be a bit clearer, I agree, but the issue it
fixes seems correct.
We want start the rh polling when a HS port is found in resume state,
and we want to time it so that the it runs just after resume is complete (40ms)
This is why we have the mod_timer(&hcd->rh_timer, bus_state->resume_done[faked_port_index]);
This would all be fine, but the xhci event handler has a bug and it calls
usb_hcd_poll_rh_status(hcd) manually right after modifying rh_timer.
This causes hub thread to read port status before resume is ready, and we
need to wait for another 250ms before the rh thread can handle the port.
What this patch in the end does is just removing the manual usb_hcd_poll_rh_status(hcd)
in this case.
Thanks
-Mathias
From: Anshuman Gupta <[email protected]>
This patch will improve the variable auto-resume latency of an usb-port.
When xhci gets a HS port status change event interrupt due to PORT_PLC
(port link state transition), linux Host controller driver drives the
resume signalling on the bus for the amount of time defined by
USB_RESUME_TIMEOUT(40ms) macro.
This 40ms delay for resume signalling is in acceptable limit, but
it get worse when xhci goes for polling mode in order to detect other
events on its ports and modify rh_timer timer with a variable time out of
1ms to (HZ/4)ms.
drivers/usb/core/hcd.c line 799
mod_timer (&hcd->rh_timer, (jiffies/(HZ/4) + 1) * (HZ/4)).
Due to above variable timeout usb auto-resume latency varies from
40ms to ~300ms(when HZ=1000).
Log Snippet:
~128ms latency
[ 53.112049] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0000
[ 53.229200] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0004
[ 53.240177] usb 1-2: usb wakeup-resume
[ 53.240195] usb 1-2: finish resume
[ 53.240357] usb usb1-port2: resume, status 0
-----------------------------------------------------------------
~300ms latency
[ 59.946620] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0000
[ 59.979341] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0000
[ 60.229342] hub 1-0:1.0: state 7 ports 12 chg 0000 evt 0004
[ 60.251321] usb 1-2: usb wakeup-resume
[ 60.251335] usb 1-2: finish resume
[ 60.251539] usb usb1-port2: resume, status 0
xhci bug behind the higher latency:
When HS port is resuming, xhci passes the resume event to hub event
thread after USB_RESUME_TIMEOUT delay. it modify rh_timer with
USB_RESUME_TIMEOUT delay, and set the polling mode flag and invoke
usb_hcd_poll_rh_status() function manually.
This eventually makes hub event thread to read the HS port status
before it resumes, since polling mode flag is set, host controller driver
modify rh_timer again with (HZ/4)ms delay, This makes hub event thread to
wait for (HZ/4)ms. which causes higher auto-resume latency.
This variable resume latency can be optimized, as in case of HS port
resume event rh_timer has already been modified with USB_RESUME_TIMEOUT
(40ms) delay, leaving the rest to GetPortStatus.
We can avoid calling usb_hcd_poll_rh_status() function manually in case
of HS port resume event.
This patch set the HCD_FLAG_POLL_RH to hcd->flags after modification of
rh_timer, and avoid calling usb_hcd_poll_rh_status(), This makes to call
usb_hcd_poll_rh_status() by rh_timer and passing the HS port resume event
to hub event thread after USB_RESUME_TIMEOUT delay.
Signed-off-by: Anshuman Gupta <[email protected]>
---
Changes in v2
- Make the commit message more clearer.
- Changing the comment.
drivers/usb/host/xhci-ring.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index cc368ad..0bccff1 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1677,9 +1677,14 @@ static void handle_port_status(struct xhci_hcd *xhci,
bus_state->resume_done[faked_port_index] = jiffies +
msecs_to_jiffies(USB_RESUME_TIMEOUT);
set_bit(faked_port_index, &bus_state->resuming_ports);
+ /* Do the rest in GetPortStatus after resume time delay.
+ * Avoid calling usb_hcd_poll_rh_status before that,so
+ * an usb port can auto-resume after resume time delay.
+ */
+ set_bit(HCD_FLAG_POLL_RH, &hcd->flags);
mod_timer(&hcd->rh_timer,
bus_state->resume_done[faked_port_index]);
- /* Do the rest in GetPortStatus */
+ bogus_port_status = true;
}
}
--
2.7.4
Hi
On 12.09.2017 08:57, anshuman gupta wrote:
> I have addressed the feedback on the patch description with reference to
> thread https://patchwork.kernel.org/patch/9916641/ with [PATCH V2]
> please provide your review comments.
>
I'm taking the patch and sending it forward to Greg after the merge window closes
(when 4.14-rc1 is out)
Thanks
-Mathias