On Thu, 2011-08-25 at 19:10 -0700, Sarah Sharp wrote:
> 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.
>
OK, but I'm wondering why the usbcore will report BH reset change for me
even without the two patches.
> 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 not sure 20ms is enough for a BH reset. I'll run some tests with
different timeout.
> 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.
>
Yes, seems warm reset is only needed when a device is connected. I'll
modify the code.
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
