svn commit: r219845 - head/sys/dev/usb/controller

[Gavin Atkinson]

Hi,

Date: Mon Mar 21 21:16:25 2011
New Revision: 219845
URL: http://svn.freebsd.org/changeset/base/219845
- Bugfix: Fix a EHCI hardware race, where the hardware computed data toggle
value is updated after that we read it in the queue-head. This patch can
fix problems with BULK timeouts. The issue was found on a Nvidia chipset.
head/sys/dev/usb/controller/ehci.c
Modified: head/sys/dev/usb/controller/ehci.c
==============================================================================
--- head/sys/dev/usb/controller/ehci.c Mon Mar 21 21:16:12 2011 (r219844)
+++ head/sys/dev/usb/controller/ehci.c Mon Mar 21 21:16:25 2011 (r219845)
@@ -1180,6 +1180,26 @@ _ehci_remove_qh(ehci_qh_t *sqh, ehci_qh_
return (last);
}
+static void
+ehci_data_toggle_update(struct usb_xfer *xfer, uint16_t actlen, uint16_t xlen)
+{
+ uint8_t full = (actlen == xlen);

style(9) strongly advises against initialising variables in the
declaration.

+ uint8_t dt;
+
+ /* count number of full packets */
+ dt = (actlen / xfer->max_packet_size) & 1;
+
+ /* cumpute remainder */
+ actlen = actlen % xfer->max_packet_size;
+
+ if (actlen > 0)
+ dt ^= 1; /* short packet at the end */
+ else if (!full)

Especially in this case - this would seem to be better written as

else if (actlen != xlen)

+ dt ^= 1; /* zero length packet at the end */
+
+ xfer->endpoint->toggle_next ^= dt;
+}

As an aside, over the years there are several PRs about bulk timeouts on
EHCI on nVidia - is it possible that this fixes any of them? Some of
the PRs date back to 6.x and 7.x - if I'm understanding the change
properly, it seems that it is possible that this could be the cause? Do
you have any opinions on whether this could fix any of those PRs?

Thanks,

Gavin

[Hans Petter Selasky]

Post by Gavin Atkinson
Hi,

Date: Mon Mar 21 21:16:25 2011
New Revision: 219845
URL: http://svn.freebsd.org/changeset/base/219845
- Bugfix: Fix a EHCI hardware race, where the hardware computed data
toggle value is updated after that we read it in the queue-head. This
patch can fix problems with BULK timeouts. The issue was found on a
Nvidia chipset.
head/sys/dev/usb/controller/ehci.c
Modified: head/sys/dev/usb/controller/ehci.c
=========================================================================
===== --- head/sys/dev/usb/controller/ehci.c Mon Mar 21 21:16:12
2011 (r219844) +++ head/sys/dev/usb/controller/ehci.c Mon Mar 21
*sqh, ehci_qh_
return (last);
}
+static void
+ehci_data_toggle_update(struct usb_xfer *xfer, uint16_t actlen, uint16_t
xlen) +{
+ uint8_t full = (actlen == xlen);

Hi Gavin,

Post by Gavin Atkinson
style(9) strongly advises against initialising variables in the
declaration.

Thanks for pointing out.

Post by Gavin Atkinson

+ uint8_t dt;
+
+ /* count number of full packets */
+ dt = (actlen / xfer->max_packet_size) & 1;
+
+ /* cumpute remainder */
+ actlen = actlen % xfer->max_packet_size;
+
+ if (actlen > 0)
+ dt ^= 1; /* short packet at the end */
+ else if (!full)

Especially in this case - this would seem to be better written as

Should be "oldactlen != xlen". I will try to fix the style issues, probably
not before tomorrow, because I'm busy tonight.

Post by Gavin Atkinson
else if (actlen != xlen)

+ dt ^= 1; /* zero length packet at the end */
+
+ xfer->endpoint->toggle_next ^= dt;
+}

As an aside, over the years there are several PRs about bulk timeouts on
EHCI on nVidia - is it possible that this fixes any of them?

Yes, I believe so. I'm not sure about 6.x and 7.x. What I caught red-handed
was that the final DATA toggle value was not properly written back to the
queue head at the time the last TD was not active. The data toggle only has
two values 0 or 1, so there is a 50% chance it will cause a timeout when the
race happens.

Post by Gavin Atkinson
Some of
the PRs date back to 6.x and 7.x - if I'm understanding the change
properly, it seems that it is possible that this could be the cause? Do
you have any opinions on whether this could fix any of those PRs?

--HPS