On Tue, Sep 13, 2011 at 06:01:32PM +0200, Harald Brennich wrote:
> Hello Sarah,
>
> The patches you mailed me fix
> Bug 41752 - USB 3.0 port does not recognize external disk
Ok, great!
> However,
> [Bug 42472] USB 3.0 port does not detect disconnect
> is not fixed.
> I however succeeded by replacing
> if (portchange & USB_PORT_STAT_C_BH_RESET)
> return 0;
> in the warm reset branch of the patch by
> if (portchange & USB_PORT_STAT_C_BH_RESET) {
> /* clear link state and connection interrupts */
> if (portchange & USB_PORT_STAT_C_LINK_STATE)
> clear_port_feature(hub->hdev, port1,
> USB_PORT_FEAT_C_PORT_LINK_STATE);
> if (portchange & USB_PORT_STAT_C_CONNECTION) {
> clear_port_feature(hub->hdev, port1,
> USB_PORT_FEAT_C_CONNECTION);
> dev_warn(hub->intfdev,
> "device on port %d bounced (status = 0x%x) during wait for reset
> (%d msec)\n",
> port1, portstatus, delay_time);
> }
> return 0;
> }
I think you might also need the attached patch; please apply
it on top of the other two.
> Will the patches you mailed me by incorporated in some future kernel
> version?
Yes, I will send all three patches off to Greg, and it will be in the
3.2 kernel release. However, I think Andiry's warm reset extension is a
bit big to backport to the stable kernels.
Sarah Sharp
> Am 12.09.2011, 21:24 Uhr, schrieb Sarah Sharp
> <sarah.a.sharp@xxxxxxxxxxxxxxx>:
>
> >Hi Harald,
> >
> >It looks like the warm port reset took longer to complete than the USB
> >core expected. The xHCI driver tried to issue an address device command
> >while the reset was still going on, so it's no wonder it failed.
> >
> >Can you try applying the attached patches? The first one extends the
> >time the USB core will wait for a warm reset, and the second one makes
> >the USB core issue a warm reset if a disconnect is reported with the
> >link in Inactive during a hot reset.
> >
> >Sarah Sharp
> >
> >On Thu, Sep 08, 2011 at 08:05:54AM -0700, Sarah Sharp wrote:
> >>(Switching to email)
> >>
> >>On Thu, Sep 08, 2011 at 12:04:06PM +0000,
> >>bugzilla-daemon@xxxxxxxxxxxxxxxxxxx wrote:
> >>> https://bugzilla.kernel.org/show_bug.cgi?id=42472
> >>>
> >>>
> >>>
> >>>
> >>>
> >>> --- Comment #2 from Harald Brennich <harald.brennich@xxxxxx>
> >>2011-09-08 12:03:56 ---
> >>> Hello Sarah,
> >>> the Renesas (formerly NEC) uPD720200 USB 3.0 Host works fine
> >>with the following
> >>> change in function hub_port_wait_reset:
> >>
> >>Look, I really don't think this is the correct solution. If a device
> >>disconnects in the middle of a port reset, then we need to treat it as
> >>if it's a new device. Someone could have yanked out the device during
> >>the port reset and inserted a new one.
> >>
> >>Let me take a look at why the warm port reset patch didn't work and get
> >>back to you next week, after I'm back from a conference.
> >>
> >>Do other USB 3.0 devices work under this host controller? Is it just
> >>this device?
> >>
> >>Sarah Sharp
> >>
> >>> Current version:
> >>> static int hub_port_wait_reset(struct usb_hub *hub, int port1,
> >>> struct usb_device *udev, unsigned int delay)
> >>> {
> >>> int delay_time, ret;
> >>> u16 portstatus;
> >>> u16 portchange;
> >>>
> >>> for (delay_time = 0;
> >>> delay_time < HUB_RESET_TIMEOUT;
> >>> delay_time += delay) {
> >>> /* wait to give the device a chance to reset */
> >>> msleep(delay);
> >>>
> >>> /* read and decode port status */
> >>> ret = hub_port_status(hub, port1, &portstatus, &portchange);
> >>> if (ret < 0)
> >>> return ret;
> >>>
> >>> /* Device went away? */
> >>> if (!(portstatus & USB_PORT_STAT_CONNECTION))
> >>> return -ENOTCONN;
> >>>
> >>> /* bomb out completely if the connection bounced */
> >>> if ((portchange & USB_PORT_STAT_C_CONNECTION))
> >>> return -ENOTCONN;
> >>>
> >>> /* if we`ve finished resetting, then break out of the loop */
> >>> if (!(portstatus & USB_PORT_STAT_RESET) &&
> >>> (portstatus & USB_PORT_STAT_ENABLE)) {
> >>> if (hub_is_wusb(hub))
> >>> udev->speed = USB_SPEED_WIRELESS;
> >>> else if (hub_is_superspeed(hub->hdev))
> >>> udev->speed = USB_SPEED_SUPER;
> >>> else if (portstatus & USB_PORT_STAT_HIGH_SPEED)
> >>> udev->speed = USB_SPEED_HIGH;
> >>> else if (portstatus & USB_PORT_STAT_LOW_SPEED)
> >>> udev->speed = USB_SPEED_LOW;
> >>> else
> >>> udev->speed = USB_SPEED_FULL;
> >>> return 0;
> >>> }
> >>>
> >>> /* switch to the long delay after two short delay failures */
> >>> if (delay_time >= 2 * HUB_SHORT_RESET_TIME)
> >>> delay = HUB_LONG_RESET_TIME;
> >>>
> >>> dev_dbg (hub->intfdev,
> >>> "port %d not reset yet, waiting %dms\n",
> >>> port1, delay);
> >>> }
> >>> }
> >>>
> >>> Modified version:
> >>> In the first modification (see report 41752), the portchange
> >>was ignored. Then
> >>> the external drive could be used, but the disconnect was not
> >>detected. This is
> >>> now resolved with checking portstatus for
> >>USB_PORT_STAT_C_CONNECTION and
> >>> USB_PORT_FEAT_C_PORT_LINK_STATE (the flags set by the uPD720200).
> >>> Is there a part in the USB spec that disallows disconnecting a
> >>port by the hub
> >>> during RESET? Or do you have another solution?
> >>>
> >>> static int hub_port_wait_reset(struct usb_hub *hub, int port1,
> >>> struct usb_device *udev, unsigned int delay)
> >>> {
> >>> int delay_time, ret;
> >>> u16 portstatus;
> >>> u16 portchange;
> >>>
> >>> for (delay_time = 0;
> >>> delay_time < HUB_RESET_TIMEOUT;
> >>> delay_time += delay) {
> >>> /* wait to give the device a chance to reset */
> >>> msleep(delay);
> >>>
> >>> /* read and decode port status */
> >>> ret = hub_port_status(hub, port1, &portstatus, &portchange);
> >>> if (ret < 0)
> >>> return ret;
> >>>
> >>> /* if we`ve finished resetting, then break out of the loop */
> >>> if (!(portstatus & USB_PORT_STAT_RESET) &&
> >>> (portstatus & USB_PORT_STAT_ENABLE)) {
> >>> /* ADDED 2011.09.08 BRE BEGIN */
> >>> /* Device went away? */
> >>> if (!(portstatus & USB_PORT_STAT_CONNECTION))
> >>> return -ENOTCONN;
> >>> if (portchange & USB_PORT_STAT_C_CONNECTION)
> >>> clear_port_feature(hub->hdev, port1,
> >>> USB_PORT_FEAT_C_CONNECTION);
> >>> if (portchange & USB_PORT_STAT_C_LINK_STATE)
> >>> clear_port_feature(hub->hdev, port1,
> >>> USB_PORT_FEAT_C_PORT_LINK_STATE);
> >>> /* ADDED 2011.09.08 BRE END */
> >>> if (hub_is_wusb(hub))
> >>> udev->speed = USB_SPEED_WIRELESS;
> >>> else if (hub_is_superspeed(hub->hdev))
> >>> udev->speed = USB_SPEED_SUPER;
> >>> else if (portstatus & USB_PORT_STAT_HIGH_SPEED)
> >>> udev->speed = USB_SPEED_HIGH;
> >>> else if (portstatus & USB_PORT_STAT_LOW_SPEED)
> >>> udev->speed = USB_SPEED_LOW;
> >>> else
> >>> udev->speed = USB_SPEED_FULL;
> >>> return 0;
> >>> }
> >>>
> >>> /* switch to the long delay after two short delay failures */
> >>> if (delay_time >= 2 * HUB_SHORT_RESET_TIME)
> >>> delay = HUB_LONG_RESET_TIME;
> >>>
> >>> dev_dbg (hub->intfdev,
> >>> "port %d not reset yet, waiting %dms\n",
> >>> port1, delay);
> >>> }
> >>>
> >>> /* ADDED 2011.09.05 BRE BEGIN */
> >>> /* Device went away? */
> >>> if (!(portstatus & USB_PORT_STAT_CONNECTION))
> >>> return -ENOTCONN;
> >>> /* bomb out completely if the connection bounced */
> >>> if ((portchange & USB_PORT_STAT_C_CONNECTION))
> >>> return -ENOTCONN;
> >>> /* ADDED 2011.09.05 BRE END */
> >>> return -EBUSY;
> >>> }
> >>>
> >>> --
> >>> Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
> >>> ------- You are receiving this mail because: -------
> >>> You are the assignee for the bug.
> >>--
> >>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
>
>
> --
> Erstellt mit Operas revolutionärem E-Mail-Modul: http://www.opera.com/mail/
>From 3a961d90d769ec5ef3b103ee8b3dfec2bb142bb4 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.
Sometimes, when a USB 3.0 device is disconnected, the Intel Panther Point
xHCI host controller will report a link state change with the state set
to "SS.Inactive". This causes the xHCI host controller to issue a warm
port reset, which doesn't finish before the USB core times out while
waiting for it to complete.
When the warm port reset does complete, and the xHC gives back a port
status change event, the xHCI driver kicks khubd. However, it fails to
set the bit indicating there is a change event for that port because the
logic in xhci-hub.c doesn't check for the warm port reset bit.
After that, the warm port status change bit is never cleared by the USB
core, and the xHC stops reporting port status change bits. (The xHCI spec
says it shouldn't report more port events until all change bits are
cleared.) This means any port changes when a new device is connected will
never be reported, and the port will seem "dead" until the xHCI driver is
unloaded and reloaded, or the computer is rebooted. Fix this by making
the xHCI driver set the port change bit when a warm port reset change bit
is set.
A better solution would be to make the USB core handle warm port reset in
differently, merging the current code with the standard port reset code
that does an incremental backoff on the timeout, and tries to complete the
port reset two more times before giving up. That more complicated fix
will be merged next window, and this fix will be backported to stable.
This should be backported to kernels as old as 3.0, since that was the
first kernel with commit a11496ebf37534177d67222285e8debed7a39788
"xHCI: warm reset support".
Signed-off-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
---
drivers/usb/host/xhci-hub.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 1e96d1f..723f823 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -761,7 +761,7 @@ 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;
+ 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
