On 11/04/2011 05:55 AM, Sarah Sharp wrote:
On Thu, Nov 03, 2011 at 10:09:51AM +0800, Andiry Xu wrote:
On 11/03/2011 07:17 AM, Sarah Sharp wrote:
Hi Andiry,
I've been running into an issue with an xHCI host that's very strict
about the rule in the xHCI spec that a port status change event is not
generated if a change bit hasn't been cleared by software. What I think
are bugs in the xHCI driver are causing future port status changes to be
lost.
I'm having issues after the resume of a USB 2.0 hub, and I thought I'd
ping you on them, since you wrote the device suspend/resume code.
The USB 2.0 roothub port state diagram in Figure 34 in the xHCI spec
shows that PLC (port link state change) is set on the transition from U3
to resume. The port status change event handler even checks for that
bit being set for both USB 2.0 and USB 3.0 devices:
static void handle_port_status(struct xhci_hcd *xhci,
union xhci_trb *event)
...
if ((temp& PORT_PLC)&& (temp& PORT_PLS_MASK) == XDEV_RESUME) {
xhci_dbg(xhci, "port resume event for port %d\n", port_id);
However, before your USB 2.0 LPM changes were merged for 3.2, there was
no where in the code where the PLC bit was cleared for USB 2.0 devices,
AFAICT.
Oh. It's supposed to be cleared in GetPortStatus in
xhci_hub_control(), after driver writes U0 to PORTSC.
Maybe I missed it because my platform does not show this issue...
Ok, I doubled checked, and I see that PLC does get cleared in bus_resume
for USB 2.0 devices, but no where else. I think you did miss clearing
it in the GetPortStatus request...
In any case, can you look over the original device suspend/resume
patches and let me know if there's a reason why you didn't clear PLC?
If it's just a bug, I'll send off these commits to the stable tree:
6fd4562 xHCI: Clear PLC for USB2 root hub ports
d2f52c9 xHCI: test and clear RWC bit
Yes, we must clear PLC directly for all USB2 root hub ports since
usbcore does not handle it.
Please send the two patches to stable tree.
Ok, will do.
However, I'm wondering if driver need to clear PLC when device
transition from Resume to U0. From the figure 34 it seems PLC is set
to 1 during the transition. If this does not cause a port status
change event, driver may need to clear PLC in GetPortStatus in
xhci_hub_control(), after writes U0 to PORTSC and PLC is reported.
Please also check on this.
I think I get what you're saying. Would a patch like this fix it?
Well, I don't think it's what I suppose to mean. See below.
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 18432cc..1de2b1c 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -462,30 +462,32 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
if ((temp& PORT_PLS_MASK) == XDEV_U3
&& (temp& PORT_POWER))
status |= USB_PORT_STAT_SUSPEND;
}
if ((temp& PORT_PLS_MASK) == XDEV_RESUME) {
if ((temp& PORT_RESET) || !(temp& PORT_PE))
goto error;
if (!DEV_SUPERSPEED(temp)&& time_after_eq(jiffies,
bus_state->resume_done[wIndex])) {
xhci_dbg(xhci, "Resume USB2 port %d\n",
wIndex + 1);
bus_state->resume_done[wIndex] = 0;
Here port is in Resume state. PLC should be cleared at this moment, done
by handle_port_status(), with commit 6fd4562 merged.
temp1 = xhci_port_state_to_neutral(temp);
temp1&= ~PORT_PLS_MASK;
temp1 |= PORT_LINK_STROBE | XDEV_U0;
+ if ((temp1& PORT_PLC))
+ temp1 |= PORT_PLC;
Here port is still in Resume state, I think no PLC is set at this moment.
xhci_writel(xhci, temp1, port_array[wIndex]);
Here driver writes U0 to PORTSC to transition it to U0 state. I'm not
sure if PLC is set during this Resume->U0 transition.
So I wonder if a PLC check and clear step is needed here - after driver
writes U0 to PORTSC.
xhci_dbg(xhci, "set port %d resume\n",
wIndex + 1);
slot_id = xhci_find_slot_id_by_port(hcd, xhci,
wIndex + 1);
if (!slot_id) {
xhci_dbg(xhci, "slot_id is zero\n");
goto error;
}
xhci_ring_device(xhci, slot_id);
bus_state->port_c_suspend |= 1<< wIndex;
bus_state->suspended_ports&= ~(1<< wIndex);
}
}
I also think there's a potential race condition because the PLC bit can
race with another port status change bit to cause a port status event,
and it will take some time for the USB core to react to it. I can
envision the following scenario (although I think it's pretty unlikely
to be hit by anyone):
1. USB 2.0 device on port 1 has hardware LPM enabled, and it's currently
in U2.
2. A second device on port 2 shares a ganged power switch with port 1,
and it starts to draw too much current.
3. The xHC sets the overcurrent change bit for ports 1 and 2, and
generates a port status change event.
I don't know much about OCC mechanism. Shouldn't xHC report two port
status change events for port 1 and 2?
4. The event is handled and khubd is kicked. However, before it can
clear the OCC bit, the device on port 1 comes out of U2, and the PLC
bit is set.
Since OCC is set, the xHC won't generate a port status change event for
the PLC bit, and it will never have the chance to be cleared in the port
status change event handler. I think what we need to do is check if the
PLC bit is set for a USB 2.0 device in the function that clears any
other port status change bit, and make sure to clear PLC there.
Does that sound like it would work? Are there any other race conditions
with the PLC bit that I've missed?
If xHC reports a port status change event, handle_port_status() will
check PLC for USB2 root hub ports with commit 6fd4562.
So no matter what cause the port status change event, if PLC is set, it
will be cleared. Please correct me if I'm wrong.
Thanks,
Andiry
--
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
