[PATCH] EHCI/USBMS corrections

* [PATCH] EHCI/USBMS corrections
@ 2013-12-13  9:09 Aleš Nesrsta
  2013-12-16 12:53 ` Melki Christian (consultant)
  0 siblings, 1 reply; 5+ messages in thread
From: Aleš Nesrsta @ 2013-12-13  9:09 UTC (permalink / raw)
  To: The development of GNU GRUB

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

I found serious mistake in grub_ehci_check_transfer which probably
causes bad behavior explained in ML thread "flash drive timing out,
can't boot vmlinuz/initrd (coreboot payload)".

The problem is:

If we cannot use interrupts, we can detect finish of EHCI transfer only
by polling of QH's TD overlay. Unfortunately, the state of most
important bits Active and Halted is the same at the beginning of the
transfer and at the successful finish (both bits are zero in both cases).

Current code causes randomly false detection of transfer finish and this
subsequently confuses USBMS and possibly other higher levels.

Additionally, I found possible mistake in USBMS - "tag" number probably
should not change during possible retries of CBW send phase.

Attached patch should solve these problems and includes also some
cosmetic changes related to better debugging when "usb" filter is used.

===

diff -purB '--exclude=.git*' ./grub/grub-core/bus/usb/ehci.c
./grub_patched/grub-core/bus/usb/ehci.c

--- ./grub/grub-core/bus/usb/ehci.c	2013-12-13 08:37:58.898205106 +0100
+++ ./grub_patched/grub-core/bus/usb/ehci.c	2013-12-12
23:39:39.048697014 +0100
@@ -1483,6 +1483,8 @@ grub_ehci_parse_halt (grub_usb_controlle
   else if ((token & GRUB_EHCI_STATUS_MISSDMF) != 0)
     err = GRUB_USB_ERR_DATA;

+  grub_dprintf ("usb", "EHCI-halt: token=%08x\n", token);
+
   return err;
 }

@@ -1513,7 +1515,7 @@ grub_ehci_check_transfer (grub_usb_contr
   struct grub_ehci *e = dev->data;
   struct grub_ehci_transfer_controller_data *cdata =
     transfer->controller_data;
-  grub_uint32_t token;
+  grub_uint32_t token, token_ftd;

   grub_dprintf ("ehci",
 		"check_transfer: EHCI STATUS=%08x, cdata=%p, qh=%p\n",
@@ -1541,13 +1543,18 @@ grub_ehci_check_transfer (grub_usb_contr
     return grub_ehci_parse_notrun (dev, transfer, actual);

   token = grub_le_to_cpu32 (cdata->qh_virt->td_overlay.token);
+  /* If the transfer consist from only one TD, we should check */
+  /* if the TD was really executed and deactivated - to prevent */
+  /* false detection of transfer finish. */
+  token_ftd = grub_le_to_cpu32 (cdata->td_first_virt->token);

   /* Detect QH halted */
   if ((token & GRUB_EHCI_STATUS_HALTED) != 0)
     return grub_ehci_parse_halt (dev, transfer, actual);

   /* Detect QH not active - QH is not active and no next TD */
-  if ((token & GRUB_EHCI_STATUS_ACTIVE) == 0)
+  if (token && ((token & GRUB_EHCI_STATUS_ACTIVE) == 0)
+	&& ((token_ftd & GRUB_EHCI_STATUS_ACTIVE) == 0))
     {
       /* It could be finish at all or short packet condition */
       if ((grub_le_to_cpu32 (cdata->qh_virt->td_overlay.next_td)
diff -purB '--exclude=.git*' ./grub/grub-core/bus/usb/usbtrans.c
./grub_patched/grub-core/bus/usb/usbtrans.c
--- ./grub/grub-core/bus/usb/usbtrans.c	2013-12-13 08:37:58.901538939 +0100
+++ ./grub_patched/grub-core/bus/usb/usbtrans.c	2013-12-12
22:45:44.077547313 +0100
@@ -302,7 +302,7 @@ grub_usb_bulk_finish_readwrite (grub_usb
     toggle = transfer->transactions[transfer->last_trans].toggle ? 0 : 1;
   else
     toggle = dev->toggle[transfer->endpoint]; /* Nothing done, take
original */
-  grub_dprintf ("usb", "bulk: toggle=%d\n", toggle);
+  grub_dprintf ("usb", "bulk: toggle[%d]=%d\n", transfer->endpoint,
toggle);
   dev->toggle[transfer->endpoint] = toggle;

   if (transfer->dir == GRUB_USB_TRANSFER_TYPE_IN)
@@ -342,9 +342,9 @@ grub_usb_bulk_readwrite_packetize (grub_
 				   grub_transfer_type_t type,
 				   grub_size_t size, char *data)
 {
-  grub_size_t actual, transferred;
+  grub_size_t actual=0, transferred;
   grub_usb_err_t err = GRUB_USB_ERR_NONE;
-  grub_size_t current_size, position;
+  grub_size_t current_size=0, position;
   grub_size_t max_bulk_transfer_len = MAX_USB_TRANSFER_LEN;
   grub_size_t max;

@@ -369,7 +369,11 @@ grub_usb_bulk_readwrite_packetize (grub_
     }

   if (!err && transferred != size)
+	{
     err = GRUB_USB_ERR_DATA;
+    grub_dprintf ("usb", "short packet: size=%d,
transferred=%d\n\t\tcurrent_size=%d, actual=%d\n",
+      size, transferred, current_size, actual);
+	}
   return err;
 }

diff -purB '--exclude=.git*' ./grub/grub-core/disk/usbms.c
./grub_patched/grub-core/disk/usbms.c
--- ./grub/grub-core/disk/usbms.c	2013-12-13 08:37:58.928209769 +0100
+++ ./grub_patched/grub-core/disk/usbms.c	2013-12-12 21:50:07.680775667
+0100
@@ -298,6 +298,8 @@ grub_usbms_transfer_bo (struct grub_scsi
   grub_usb_err_t errCSW = GRUB_USB_ERR_NONE;
   int retrycnt = 3 + 1;

+  tag++;
+
  retry:
   retrycnt--;
   if (retrycnt == 0)
@@ -306,7 +308,7 @@ grub_usbms_transfer_bo (struct grub_scsi
   /* Setup the request.  */
   grub_memset (&cbw, 0, sizeof (cbw));
   cbw.signature = grub_cpu_to_le32 (0x43425355);
-  cbw.tag = tag++;
+  cbw.tag = tag;
   cbw.transfer_length = grub_cpu_to_le32 (size);
   cbw.flags = (!read_write) << GRUB_USBMS_DIRECTION_BIT;
   cbw.lun = scsi->lun; /* In USB MS CBW are LUN bits on another place
than in SCSI CDB, both should be set correctly. */
@@ -328,6 +330,7 @@ grub_usbms_transfer_bo (struct grub_scsi
    * XXX: Error recovery is maybe still not fully correct. */
   err = grub_usb_bulk_write (dev->dev, dev->out,
 			     sizeof (cbw), (char *) &cbw);
+  grub_dprintf ("usb", "write: %d %d\n", err, GRUB_USB_ERR_STALL);
   if (err)
     {
       if (err == GRUB_USB_ERR_STALL)
@@ -335,7 +338,7 @@ grub_usbms_transfer_bo (struct grub_scsi
 	  grub_usb_clear_halt (dev->dev, dev->out->endp_addr);
 	  goto CheckCSW;
 	}
-      return grub_error (GRUB_ERR_IO, "USB Mass Storage request failed");
+      goto retry;
     }

   /* Read/write the data, (maybe) according to specification.  */
OD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread