On Wed, Jul 07, 2010 at 10:10:17AM -0600, Fajun Chen wrote:
> Hi Sarah,
>
> Good catch! I meant to use "1 << USB_PORT_FEAT_SUPERSPEED" to match super
> speed after port reset.
>
> USB_PORT_STAT_SUPER_SPEED is not defined in 2.6.34 but defined in 2.6.35-rc3
> as 0x8000, so port status bit 15 is checked for super speed.
Ah, ok, then the patch for 2.6.34 should be checking for
(1 << USB_PORT_FEAT_SUPERSPEED). And the patch to fix 2.6.35 should
check for USB_PORT_STAT_SUPER_SPEED.
> USB_PORT_FEAT_SUPERSPEED is defined in 2.6.34 as 11, so port status bit 11
> is checked for super speed.
>
> Based on my debugging trace, port status control message returns 0x903 after
> port reset. Based on portspeed() function in 2.6.34, it should be
> interpreted as super speed, but it will not be interpreted as super speed by
> your patch or 2.6.35 since bit 15 is not set!
I don't understand this paragraph. Under 2.6.34, the xHCI port speed
function will return (1 << USB_PORT_FEAT_SUPERSPEED), plus other port
status bits. The patch to fix 2.6.34 should check whether bit 11, or
(1 << USB_PORT_FEAT_SUPERSPEED) is set.
Under 2.6.35-rc3, the xHCI port speed function will return
USB_PORT_STAT_SUPER_SPEED (bit 15 set), plus other port status bits. So
the fix for 2.6.35-rc3 should check for USB_PORT_STAT_SUPER_SPEED.
> Based on USB3.0 spec, port status bit 10-12 represents port speed where
> value 0 is super speed. Can you clarify the difference in these kernel
> releases and USB3.0 spec? Does NEC host controller violate the spec?
No, the NEC host does not violate the spec. The xHCI driver translates
the port status bits in the hardware to something that khubd can
understand. USB_PORT_STAT_SUPER_SPEED and USB_PORT_FEAT_SUPERSPEED were
always meant to be internal values only used by the USB core. They have
nothing to do with the USB specification. (In fact,
USB_PORT_FEAT_HIGHSPEED is also not defined in the USB 2.0
specification, it was only defined for internal USB core usage.) See
commit 288ead45fa6637e959015d055304f521cbbc0575.
I know that the format for port status is different for USB 3.0 and USB
2.0 hubs. However, khubd doesn't currently support USB 3.0 hubs. The
xHCI roothub is treated like a USB 2.0 hub instead, it just has some USB
3.0 ports on it. So the port status returned to khubd should be
interpreted with the USB 2.0 spec instead of the USB 3.0 spec.
I know this needs to be fixed. What should happen is the xHCI driver
emmulates two roothubs: a USB 2.0 hub and a USB 3.0 hub. Then all the
code that looks at the port status and port changed bits will need to be
special cased for if the hub is a USB 3.0 hub. But the fakery works for
now, so it's there until someone (probably me) cleans it up.
Sarah Sharp
> On Wed, Jul 7, 2010 at 4:18 AM, Sarah Sharp
> <sarah.a.sharp@xxxxxxxxxxxxxxx>wrote:
>
> > On Wed, Jun 30, 2010 at 06:28:05PM -0600, Fajun Chen wrote:
> > > Hi Sarah,
> > >
> > > I've traced the speed downgrade issue to port reset and the
> > interpretation
> > > of port status in USB hub driver code. The port status was 0x903 after
> > port
> > > reset and the existing code (hub_port_wait_reset function) interpreted
> > this
> > > as full speed, the USB3.0 port was disabled due to the speed change after
> > > reset. USB2.0 spec, USB3.0 spec and xHCI spec all have different
> > definitions
> > > of port status, so this is a little ambiguous. However, the same hub code
> > > interpret port status 0x903 as super speed when the drive was first
> > powered
> > > up. The following patch adds super speed mapping from port status, no
> > more
> > > speed downgrade issue with the fix and I can get USB3.0 performance in my
> > > system.
> >
> > Thanks for catching this. You're correct that hub.c is missing a case
> > for SuperSpeed after a port reset. But I'm not convinced this is quite
> > the right fix.
> >
> > > -bash-3.00$ diff -urNb
> > ../../../../linux-2.6.34-stock/drivers/usb/core/hub.c
> > > hub.c
> > > --- ../../../../linux-2.6.34-stock/drivers/usb/core/hub.c
> > 2010-06-16
> > > 13:20:39.000000000 -0600
> > > +++ hub.c 2010-06-30 18:08:09.000000000 -0600
> > > @@ -1972,6 +1972,8 @@
> > > (portstatus & USB_PORT_STAT_ENABLE)) {
> > > if (hub_is_wusb(hub))
> > > udev->speed = USB_SPEED_WIRELESS;
> > > + else if (portstatus & USB_PORT_FEAT_SUPERSPEED)
> > > + 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)
> >
> > Why are you checking USB_PORT_FEAT_SUPERSPEED, instead of
> > USB_PORT_STAT_SUPER_SPEED? The xhci_port_speed() function will return
> > (1 << USB_PORT_FEAT_SUPERSPEED) for the portstatus of a SuperSpeed port,
> > so I'm not sure why this patch worked.
> >
> > #define USB_PORT_FEAT_SUPERSPEED 11
> > #define USB_PORT_STAT_SUPER_SPEED 0x8000
> >
> > I think it only worked because USB_PORT_STAT_INDICATOR,
> > USB_PORT_STAT_ENABLE, and USB_PORT_STAT_CONNECTION was set, which
> > matched the #define of USB_PORT_FEAT_SUPERSPEED to decimal 11, or
> > 0x1011. I think if you had reset a configured high speed device with
> > this patch, it would have been detected as a SuperSpeed device. I think
> > the correct patch is to test for
> > (portstatus & USB_PORT_STAT_SUPER_SPEED).
> >
> > The patch also won't apply against 2.6.35, because
> > USB_PORT_FEAT_SUPERSPEED was removed by commit 288ead4. So I think it's
> > better to use USB_PORT_STAT_SUPER_SPEED for fixes to both kernels.
> >
> > Can you try the attached patch and see if it fixes your issue?
> >
> > Sarah Sharp
> >
> > > On Tue, Jun 29, 2010 at 4:48 PM, Fajun Chen <fajun.chen@xxxxxxxxxxx>
> > wrote:
> > >
> > > > Hi Sarah,
> > > >
> > > > I simplified the procedure once more to repeat the speed downgrade
> > > > problem. No command timeout is required, a simple port reset can
> > downgrade
> > > > the port speed from super speed to high speed. Please check out the
> > logs
> > > > attached. Can you tell if this is a host controller/driver issue or
> > USB3.0
> > > > bridge issue? Note that power cycling the bridge can not recover super
> > > > speed, so I suspect this is a host issue.
> > > >
> > > > Here're some highlights of the log:
> > > > dmesg.log.0, Line 468:
> > > > Jun 29 15:22:20 2 user.debug kernel: [1277846540 885125] xhci_hcd
> > > > 0000:03:00.0: set port reset, actual port 0 status = 0x1311
> > > >
> > > > dmesg.log.0 Line 726-727:
> > > > Jun 29 15:22:21 2 user.info kernel: [1277846541 961874] xhci_hcd
> > > > 0000:03:00.0: Can't reset device (slot ID 1) in
> > enabled/disabled
> > > > state
> > > > Jun 29 15:22:21 2 user.info kernel: [1277846541 961933] xhci_hcd
> > > > 0000:03:00.0: Not freeing device rings.
> > > > // Reset device at wrong state. Can we prevent it from happening?
> > > >
> > > > dmesg.log.0, Line 730:
> > > > Jun 29 15:22:21 2 user.info kernel: [1277846541 962094] usb 9-3: new
> > high
> > > > speed USB device using xhci_hcd and add ress 0
> > > > // Based on the trace in another test log (not attached): Get Port
> > Status
> > > > control request returned status 0x503. So the hub did detect high speed
> > > > device.
> > > > // Question: which information should be used for port status? Get Port
> > > > Status control message or Port Status register?
> > > >
> > > > Any insights from the logs attached? Really appreciate your help!
> > > >
> > > > Thanks,
> > > > Fajun
> > > >
> > > >
> > > >
> > > >
> >
--
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
