Re: Plans on 1.99 release - USB issues

["Aleš Nesrsta" <starous@volny.cz>]

[-- Attachment #1: Type: text/plain, Size: 3885 bytes --]

Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> > The issue was that we were using getStatus every time we polled for new
> > devices which suposedly (I fixed few other bugs in the code I used at
> > the time so, I'm not sure) drove hub in my USB keyboard crazy. The
> > correct way is to poll interrupt pipe.
> >   
> Just to be clear: polling interrupt pipe on hub is what I have
> implemented in keylayouts, just so you don't implement it again
> > Another thing I added is the ability to do background transfers. It is
> > important for USB keyboard support because otherwise we lose messages on
> > keyboard interrupt pipe. It triggered a bug in uhci module. Now there
> > are 2 issues:
> > 1) code is new or modified. Needs testing.
> > 2) On yeeloong the data read from mass storage is sometimes corrupted.
> > Happens in mainline, not sure about other branches. It seems that it
> > wasn't the case before. It's either a regression (perhaps from my code
> > for partial transfers) or cache issues (some cache isn't correctly flushed)
> >   
> >> Could I help You with it - at least with testing ?
> >>   
> >>     
> > Yes, a test run of keylayouts branch on your hardware would be helpful.
> > Especially I'm interested if USB data corruption happens in your case too.

Hi Vladimir,

I made some tests on machine with UHCI with kbdlayouts branch (rev.
2424).
I did not notice any evident data corruption. But there were some
another odd results:

1.
My USB keyboard is low speed device (Genius KB-06XE, model no. K639).
Low speed device transfer was not properly handled - I made some simple
patch - see attachment, I did not commit it into repository.
Control type transfer with keyboard is working with this patch, but bulk
(interrupt) transfer returns always err=7 and in UHCI TD status&control
are set bits "CRC/Time Out Error" (bit 18) and "Stalled" (bit 22). It is
the same behavior as some normal full speed mass-storage devices do also
(I reported it in some previous e-mails). I have some idea right now -
probably there is bad handling of UHCI transfer status in uhci.c when
more than one bit is set - GRUB_USB_ERR_TIMEOUT is returned instead of
GRUB_USB_ERR_STALL. I will try to play with this part during weekend and
I will report to You if some success happens...

2.
Next bad thing is some problem with device attachment detection or
handling of newly attached device on non-root hubs. Behavior of this
problem looks little bit randomly and probably depends also on used port
of hub - some ports are often working, some not (but all hub ports are
working normally in Linux/Windows). I did not find reason yet.

3.
Sometimes partitions of USB mass storage devices were not properly
detected - maybe disk cache problem again?
Maybe it is data corruption which is reported by you - but I never
detected data corruption when I read data from files.

---
I am not sure if interrupt transfers can be handled via bulk queue on
OHCI - according specification, it should be handled via interrupt
pointers table which is currently not implemented in ohci.c. Did you
test background polling of interrupt pipe on some PC with OHCI?

---
At the end some maybe bad idea - if I am not wrong, two simultaneous
transfers can happen (be active in UHCI/OHCI) with current background
bulk/interrupt transfer. There is question if are all related functions
fully reentrant ? I.e., is correct handling of some shared data ?
For the first look I don't see such problem - with one exception:
donehead interrupt can be probably incorrectly handled in ohci.c - it
can be detected and misinterpreted by wrong call of
grub_ohci_check_transfer. The first aid in this case can be forcing of
OHCI driver into "bad OHCI" mode, i.e. donehead interrupt ignoring - in
this mode it should work properly.

Regards
Ales


[-- Attachment #2: usb_low_speed_100903_0 --]
[-- Type: text/x-patch, Size: 1124 bytes --]

--- kbdlayouts/grub-core/bus/usb/uhci.c	2010-09-03 22:13:28.000000000 +0200
+++ kbdlayouts_changed/grub-core/bus/usb/uhci.c	2010-09-01 21:10:24.000000000 +0200
@@ -414,7 +414,7 @@
 grub_uhci_transaction (struct grub_uhci *u, unsigned int endp,
 		       grub_transfer_type_t type, unsigned int addr,
 		       unsigned int toggle, grub_size_t size,
-		       grub_uint32_t data)
+		       grub_uint32_t data, grub_usb_speed_t speed)
 {
   grub_uhci_td_t td;
   static const unsigned int tf[] = { 0x69, 0xE1, 0x2D };
@@ -439,7 +439,8 @@
   td->linkptr = 1;
 
   /* Active!  Only retry a transfer 3 times.  */
-  td->ctrl_status = (1 << 23) | (3 << 27);
+  td->ctrl_status = (1 << 23) | (3 << 27) |
+                    ((speed == GRUB_USB_SPEED_LOW) ? (1 << 26) : 0);
 
   /* If zero bytes are transmitted, size is 0x7FF.  Otherwise size is
      size-1.  */
@@ -495,7 +496,8 @@
 
       td = grub_uhci_transaction (u, transfer->endpoint & 15, tr->pid,
 				  transfer->devaddr, tr->toggle,
-				  tr->size, tr->data);
+				  tr->size, tr->data,
+				  transfer->dev->speed);
       if (! td)
 	{
 	  grub_size_t actual = 0;