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?
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;
temp1 = xhci_port_state_to_neutral(temp);
temp1 &= ~PORT_PLS_MASK;
temp1 |= PORT_LINK_STROBE | XDEV_U0;
+ if ((temp1 & PORT_PLC))
+ temp1 |= PORT_PLC;
xhci_writel(xhci, temp1, port_array[wIndex]);
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.
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?
Sarah Sharp
--
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
