Hi Sarah,
Thank you for your detailed comments. I will resend a modified one for your
review. And some suggestion I have some words to say:
On Wed, Mar 03, 2010 at 10:53:32AM -0800, Sarah Sharp wrote:
> > + xhci_stop_endpoints(xhci, wIndex + 1, 1, &flags);
> > + temp = xhci_port_state_to_neutral(temp);
> > + temp &= ~PORT_PLS_MASK;
> > + temp |= PORT_LINK_STROBE | XDEV_U3;
> > + xhci_writel(xhci, temp, addr);
> > +
> > + spin_unlock_irqrestore(&xhci->lock, flags);
> > + msleep(10); /* wait device to enter */
>
> Isn't this wait handled by the USB core?
This delay is handled in xhci hook. I use msleep to do delay.
> > + spin_lock_irqsave(&xhci->lock, flags);
> > +
> > + temp = xhci_readl(xhci, addr);
> > + set_bit(wIndex, &xhci->suspended_ports);
>
> Do you want to set this inside of the lock? I'm not sure how you use
> xhci->suspended_ports; do you need mutual exclusion?
This xhci->lock is held, when in xhci_hub_control hook. And the suspended_ports
need protection as other xhci member. This is similar as what ehci did.
> > + if (temp & XDEV_U3) {
> > + if ((temp & PORT_PE) == 0)
> > + goto error;
> > + if (DEV_SUPERSPEED(temp)) {
> > + temp = xhci_port_state_to_neutral(temp);
> > + temp &= ~PORT_PLS_MASK;
> > + temp |= PORT_LINK_STROBE | XDEV_U0;
> > + xhci_writel(xhci, temp, addr);
> > + } else {
> > + temp = xhci_port_state_to_neutral(temp);
> > + temp &= ~PORT_PLS_MASK;
> > + temp |= PORT_LINK_STROBE | XDEV_RESUME;
> > + xhci_writel(xhci, temp, addr);
> > +
> > + spin_unlock_irqrestore(&xhci->lock,
> > + flags);
> > + msleep(20);
>
> What's to stop some other part of the xHCI hub code from mucking with
> your port registers here?
I only use msleep to do delay and release lock to avoid potential issue.
--
Best Regards,
- Crane
--
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
