On Thu, Aug 25, 2011 at 01:27:45PM -0400, Alan Stern wrote:
> On Thu, 25 Aug 2011, Sarah Sharp wrote:
>
> > > > + } else {
> > > > + if (portchange & USB_PORT_STAT_C_BH_RESET) {
> > > > + clear_port_feature(hub->hdev, port1,
> > > > + USB_PORT_FEAT_C_BH_PORT_RESET);
> > > > +
> > > > + if (portchange & USB_PORT_STAT_C_LINK_STATE)
> > > > + clear_port_feature(hub->hdev, port1,
> > > > + USB_PORT_FEAT_C_PORT_LINK_STATE);
> > > > +
> > >
> > > Why do you need to clear these features specially for a warm reset?
> > > Can't they be cleared the same way as USB_PORT_FEAT_C_RESET?
> >
> > I don't understand your question. Are you trying to say that the xHCI
> > host controller should clear the BH reset connect change when the USB
> > core asks it to clear the RESET change? Or are you suggesting something
> > else?
>
> I meant it's weird to see those two clear_port_feature() calls sitting
> by themselves like that. They should occur along with the
> clear_port_feature for C_RESET; all the status-change features should
> be cleared in the same place in the code.
Yes, that looked weird to me too. I tried the patch as-is, and it did
fix the issue with BH reset not getting cleared. However, when I
removed the lines that clear the BH reset change and link state change
(while leaving in the return statement when the BH reset change bit was
set), the USB core would not clear the BH reset change.
I think it's because xhci-hub.c is not actually indicating there's a
change on the port when the BH reset change bit is set:
int xhci_hub_status_data(struct usb_hcd *hcd, char *buf)
{
...
/* Initial status is no changes */
retval = (max_ports + 8) / 8;
memset(buf, 0, retval);
status = 0;
mask = PORT_CSC | PORT_PEC | PORT_OCC | PORT_PLC;
spin_lock_irqsave(&xhci->lock, flags);
/* For each port, did anything change? If so, set that bit in buf. */
for (i = 0; i < max_ports; i++) {
temp = xhci_readl(xhci, port_array[i]);
if (temp == 0xffffffff) {
retval = -ENODEV;
break;
}
if ((temp & mask) != 0 ||
(bus_state->port_c_suspend & 1 << i) ||
(bus_state->resume_done[i] && time_after_eq(
jiffies, bus_state->resume_done[i]))) {
buf[(i + 1) / 8] |= 1 << (i + 1) % 8;
status = 1;
}
}
spin_unlock_irqrestore(&xhci->lock, flags);
return status ? retval : 0;
}
Note that the mask does not include the warm port reset bit (PORT_WRC).
When I applied the attached patch to include the warm port reset bit
change in the mask on top of Andiry's patch (plus the modifications I
made), the USB core did clear the BH reset change.
When I reverted Andiry's patch, and applied my mask bit patch, the USB
core cleared the BH reset fine. I think we should go with the simpler
fix for 3.1 and the stable trees, and maybe Andiry's patch (modified to
remove the clearing of the change bits) should be queued for 3.2.
Andiry, maybe you want to change the HUB_BH_RESET_TIME to 20 ms? With
the incremental backoff, I think an initial test after 100 ms is too
long.
I'm also wondering if the patch can include the bit about not trying a
warm reset change if a device is disconnected. In the instance where I
see the BH reset get set by the core, I'm actually disconnecting a USB
3.0 hub, and the port status register reports a connect status change
(with a disconnected status), and link state change with a link status
of SS_INACTIVE. I don't think the warm reset should get driven by the
USB core when the port reports the device as disconnected. The link
state change should just get cleared by the USB core.
Sarah Sharp
>From aaf7d6786c7e73356c2a10fee18726b0634e66df Mon Sep 17 00:00:00 2001
From: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
Date: Thu, 25 Aug 2011 17:31:42 -0700
Subject: [PATCH] xhci: Set change bit when warm reset change is set.
This seems to fix the issue with the BH reset change not being cleared
in the USB core (without Andiry's patch to change the USB core).
Andiry, I think you're only trying to set the port change bit when the
port reset change completes from the internal timeout, but the warm port
status change bit can be set when the hot reset fails, so the USB core
needs to clear that bit.
I'm not sure if this is the correct fix, but this fixes the issue I had
with the BH reset change not being cleared. Here's the log after this
patch is applied, and I disconnect a USB 3.0 hub:
Aug 25 17:25:23 talon kernel: [ 7696.333353] xhci_hcd 0000:00:14.0: Port Status Change Event for port 4
Aug 25 17:25:23 talon kernel: [ 7696.333360] xhci_hcd 0000:00:14.0: resume root hub
Aug 25 17:25:23 talon kernel: [ 7696.333380] usb usb3: usb wakeup-resume
Aug 25 17:25:23 talon kernel: [ 7696.333385] usb usb3: usb auto-resume
Aug 25 17:25:23 talon kernel: [ 7696.340427] xhci_hcd 0000:00:14.0: Port Status Change Event for port 8
Aug 25 17:25:23 talon kernel: [ 7696.340454] hub 4-0:1.0: state 7 ports 4 chg 0000 evt 0010
Aug 25 17:25:23 talon kernel: [ 7696.340463] xhci_hcd 0000:00:14.0: get port status, actual port 3 status = 0x4202c0
Aug 25 17:25:23 talon kernel: [ 7696.340466] xhci_hcd 0000:00:14.0: Get port status returned 0x4102c0
Aug 25 17:25:23 talon kernel: [ 7696.340474] xhci_hcd 0000:00:14.0: clear port connect change, actual port 3 status = 0x4002c0
Aug 25 17:25:23 talon kernel: [ 7696.340481] xhci_hcd 0000:00:14.0: clear port link state change, actual port 3 status = 0x2c0
Aug 25 17:25:23 talon kernel: [ 7696.340485] hub 4-0:1.0: warm reset port 4
Aug 25 17:25:23 talon kernel: [ 7696.343895] hub 3-0:1.0: hub_resume
Aug 25 17:25:23 talon kernel: [ 7696.343905] xhci_hcd 0000:00:14.0: get port status, actual port 0 status = 0x2a0
Aug 25 17:25:23 talon kernel: [ 7696.343908] xhci_hcd 0000:00:14.0: Get port status returned 0x100
Aug 25 17:25:23 talon kernel: [ 7696.343914] xhci_hcd 0000:00:14.0: get port status, actual port 1 status = 0x2a0
Aug 25 17:25:23 talon kernel: [ 7696.343917] xhci_hcd 0000:00:14.0: Get port status returned 0x100
Aug 25 17:25:23 talon kernel: [ 7696.343921] xhci_hcd 0000:00:14.0: get port status, actual port 2 status = 0x2a0
Aug 25 17:25:23 talon kernel: [ 7696.343924] xhci_hcd 0000:00:14.0: Get port status returned 0x100
Aug 25 17:25:23 talon kernel: [ 7696.343929] xhci_hcd 0000:00:14.0: get port status, actual port 3 status = 0x202a0
Aug 25 17:25:23 talon kernel: [ 7696.343931] xhci_hcd 0000:00:14.0: Get port status returned 0x10100
Aug 25 17:25:23 talon kernel: [ 7696.343935] hub 3-0:1.0: port 4: status 0100 change 0001
Aug 25 17:25:23 talon kernel: [ 7696.343941] xhci_hcd 0000:00:14.0: clear port connect change, actual port 3 status = 0x2a0
Aug 25 17:25:23 talon kernel: [ 7696.363850] xhci_hcd 0000:00:14.0: get port status, actual port 3 status = 0x2b0
Aug 25 17:25:23 talon kernel: [ 7696.363857] xhci_hcd 0000:00:14.0: Get port status returned 0x2b0
Aug 25 17:25:23 talon kernel: [ 7696.363865] hub 4-0:1.0: port 4, status 02c0, change 0041, 5.0 Gb/s
Aug 25 17:25:23 talon kernel: [ 7696.363870] usb 4-4: USB disconnect, device number 2
Aug 25 17:25:23 talon kernel: [ 7696.363874] usb 4-4: unregistering device
Aug 25 17:25:23 talon kernel: [ 7696.363878] usb 4-4: unregistering interface 4-4:1.0
...
Aug 25 17:25:23 talon kernel: [ 7696.365038] xhci_hcd 0000:00:14.0: get port status, actual port 3 status = 0x2b0
Aug 25 17:25:23 talon kernel: [ 7696.365042] xhci_hcd 0000:00:14.0: Get port status returned 0x2b0
Aug 25 17:25:23 talon kernel: [ 7696.365049] xhci_hcd 0000:00:14.0: Dropped 1 ep ctxs, flags = 0x1, 2 now active.
Aug 25 17:25:23 talon kernel: [ 7696.365054] xhci_hcd 0000:00:14.0: Freeing ring at ffff8801431f52a0
Aug 25 17:25:23 talon kernel: [ 7696.365058] xhci_hcd 0000:00:14.0: Freeing DMA segment at ffff880003d34800 (virtual) 0x3d34800 (DMA)
Aug 25 17:25:23 talon kernel: [ 7696.365063] xhci_hcd 0000:00:14.0: Freeing priv segment structure at ffff88012e4bd080
Aug 25 17:25:23 talon kernel: [ 7696.365067] xhci_hcd 0000:00:14.0: No TTs to free.
Aug 25 17:25:23 talon kernel: [ 7696.365071] xhci_hcd 0000:00:14.0: Freeing ring at ffff8801431f5cc0
Aug 25 17:25:23 talon kernel: [ 7696.365075] xhci_hcd 0000:00:14.0: Freeing DMA segment at ffff88007fb6e000 (virtual) 0x7fb6e000 (DMA)
Aug 25 17:25:23 talon kernel: [ 7696.365079] xhci_hcd 0000:00:14.0: Freeing priv segment structure at ffff88012e4bdb00
Aug 25 17:25:23 talon kernel: [ 7696.403765] xhci_hcd 0000:00:14.0: get port status, actual port 3 status = 0x2b0
Aug 25 17:25:23 talon kernel: [ 7696.403771] xhci_hcd 0000:00:14.0: Get port status returned 0x2b0
Aug 25 17:25:24 talon kernel: [ 7696.439488] xhci_hcd 0000:00:14.0: Port Status Change Event for port 8
Aug 25 17:25:24 talon kernel: [ 7696.443671] xhci_hcd 0000:00:14.0: get port status, actual port 3 status = 0x2802a0
Aug 25 17:25:24 talon kernel: [ 7696.443677] xhci_hcd 0000:00:14.0: Get port status returned 0x3002a0
Aug 25 17:25:24 talon kernel: [ 7696.483581] xhci_hcd 0000:00:14.0: get port status, actual port 3 status = 0x2802a0
Aug 25 17:25:24 talon kernel: [ 7696.483588] xhci_hcd 0000:00:14.0: Get port status returned 0x3002a0
Aug 25 17:25:24 talon kernel: [ 7696.523503] xhci_hcd 0000:00:14.0: get port status, actual port 3 status = 0x2802a0
Aug 25 17:25:24 talon kernel: [ 7696.523509] xhci_hcd 0000:00:14.0: Get port status returned 0x3002a0
Aug 25 17:25:24 talon kernel: [ 7696.523518] hub 4-0:1.0: debounce: port 4: total 100ms stable 100ms status 0x2a0
Aug 25 17:25:24 talon kernel: [ 7696.523523] hub 4-0:1.0: state 7 ports 4 chg 0000 evt 0010
Aug 25 17:25:24 talon kernel: [ 7696.523529] xhci_hcd 0000:00:14.0: get port status, actual port 3 status = 0x2802a0
Aug 25 17:25:24 talon kernel: [ 7696.523532] xhci_hcd 0000:00:14.0: Get port status returned 0x3002a0
Aug 25 17:25:24 talon kernel: [ 7696.523535] hub 4-0:1.0: reset change on port 4
Aug 25 17:25:24 talon kernel: [ 7696.523541] xhci_hcd 0000:00:14.0: clear port reset change, actual port 3 status = 0x802a0
Aug 25 17:25:24 talon kernel: [ 7696.523545] hub 4-0:1.0: warm reset change on port 4
Aug 25 17:25:24 talon kernel: [ 7696.523551] xhci_hcd 0000:00:14.0: clear port warm(BH) reset change, actual port 3 status = 0x2a0
Aug 25 17:25:24 talon kernel: [ 7696.523557] hub 3-0:1.0: state 7 ports 4 chg 0010 evt 0000
Aug 25 17:25:24 talon kernel: [ 7696.523562] xhci_hcd 0000:00:14.0: get port status, actual port 3 status = 0x2a0
Aug 25 17:25:24 talon kernel: [ 7696.523565] xhci_hcd 0000:00:14.0: Get port status returned 0x100
Aug 25 17:25:24 talon kernel: [ 7696.523568] hub 3-0:1.0: port 4, status 0100, change 0000, 12 Mb/s
Signed-off-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
---
drivers/usb/host/xhci-hub.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 70ee7cc..fab543f 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -761,7 +761,8 @@ int xhci_hub_status_data(struct usb_hcd *hcd, char *buf)
memset(buf, 0, retval);
status = 0;
- mask = PORT_CSC | PORT_PEC | PORT_OCC | PORT_PLC;
+ /* XXX: why not PORT_RC for a reset change? */
+ mask = PORT_CSC | PORT_PEC | PORT_OCC | PORT_PLC | PORT_WRC;
spin_lock_irqsave(&xhci->lock, flags);
/* For each port, did anything change? If so, set that bit in buf. */
--
1.7.4.1
