[2236] 2009-05-25 Christian Franke <address@hidden>

[Christian Franke]

Revision: 2236
          http://svn.sv.gnu.org/viewvc/?view=rev&root=grub&revision=2236
Author:   chrfranke
Date:     2009-05-25 19:36:50 +0000 (Mon, 25 May 2009)
Log Message:
-----------
2009-05-25  Christian Franke  <address@hidden>

        * disk/ata.c (grub_ata_wait_not_busy): Add debug output of status
        register.
        (grub_atapi_identify): Add wait after drive select.
        (grub_ata_identify): Do more strict status register check before
        calling grub_atapi_identify ().  Suppress error message if status
        register is 0x00 after command failure.  Add status register
        check after PIO read to avoid bogus identify due to stuck DRQ.
        Thanks to Pavel Roskin for testing.
        (grub_device_initialize): Remove unsafe status register check.
        Thanks to 'phcoder' for problem report and patch.
        Prevent sign extension in debug message.

Modified Paths:
--------------
    trunk/grub2/ChangeLog
    trunk/grub2/disk/ata.c

Modified: trunk/grub2/ChangeLog
===================================================================
--- trunk/grub2/ChangeLog       2009-05-24 08:39:29 UTC (rev 2235)
+++ trunk/grub2/ChangeLog       2009-05-25 19:36:50 UTC (rev 2236)
@@ -1,3 +1,17 @@
+2009-05-25  Christian Franke  <address@hidden>
+
+       * disk/ata.c (grub_ata_wait_not_busy): Add debug output of status
+       register.
+       (grub_atapi_identify): Add wait after drive select.
+       (grub_ata_identify): Do more strict status register check before
+       calling grub_atapi_identify ().  Suppress error message if status
+       register is 0x00 after command failure.  Add status register
+       check after PIO read to avoid bogus identify due to stuck DRQ.
+       Thanks to Pavel Roskin for testing.
+       (grub_device_initialize): Remove unsafe status register check.
+       Thanks to 'phcoder' for problem report and patch.
+       Prevent sign extension in debug message.
+
 2009-05-23  Colin D Bennett  <address@hidden>
 
        Cleaned up `include/grub/normal.h'.  Grouped prototypes by

Modified: trunk/grub2/disk/ata.c
===================================================================
--- trunk/grub2/disk/ata.c      2009-05-24 08:39:29 UTC (rev 2235)
+++ trunk/grub2/disk/ata.c      2009-05-25 19:36:50 UTC (rev 2236)
@@ -41,11 +41,14 @@
   grub_millisleep (1);
 
   int i = 1;
-  while (grub_ata_regget (dev, GRUB_ATA_REG_STATUS) & GRUB_ATA_STATUS_BUSY)
+  grub_uint8_t sts;
+  while ((sts = grub_ata_regget (dev, GRUB_ATA_REG_STATUS))
+        & GRUB_ATA_STATUS_BUSY)
     {
       if (i >= milliseconds)
         {
-         grub_dprintf ("ata", "timeout: %dms\n", milliseconds);
+         grub_dprintf ("ata", "timeout: %dms, status=0x%x\n",
+                       milliseconds, sts);
          return grub_error (GRUB_ERR_TIMEOUT, "ATA timeout");
        }
 
@@ -151,6 +154,7 @@
     return grub_errno;
 
   grub_ata_regset (dev, GRUB_ATA_REG_DISK, 0xE0 | dev->device << 4);
+  grub_ata_wait ();
   if (grub_ata_check_ready (dev))
     {
       grub_free (info);
@@ -248,6 +252,7 @@
   info16 = (grub_uint16_t *) info;
 
   grub_ata_regset (dev, GRUB_ATA_REG_DISK, 0xE0 | dev->device << 4);
+  grub_ata_wait ();
   if (grub_ata_check_ready (dev))
     {
       grub_free (info);
@@ -259,24 +264,39 @@
 
   if (grub_ata_wait_drq (dev, 0, GRUB_ATA_TOUT_STD))
     {
-      if (grub_ata_regget (dev, GRUB_ATA_REG_ERROR) & 0x04) /* ABRT */
-       {
-         /* Device without ATA IDENTIFY, try ATAPI.  */
-         grub_free(info);
-         grub_errno = GRUB_ERR_NONE;
-         return grub_atapi_identify (dev);
-       }
+      grub_free (info);
+      grub_errno = GRUB_ERR_NONE;
+      grub_uint8_t sts = grub_ata_regget (dev, GRUB_ATA_REG_STATUS);
+
+      if ((sts & (GRUB_ATA_STATUS_BUSY | GRUB_ATA_STATUS_DRQ
+         | GRUB_ATA_STATUS_ERR)) == GRUB_ATA_STATUS_ERR
+         && (grub_ata_regget (dev, GRUB_ATA_REG_ERROR) & 0x04 /* ABRT */))
+       /* Device without ATA IDENTIFY, try ATAPI.  */
+       return grub_atapi_identify (dev);
+
+      else if (sts == 0x00)
+       /* No device, return error but don't print message.  */
+       return GRUB_ERR_UNKNOWN_DEVICE;
+
       else
-       {
-         /* Error.  */
-         grub_free(info);
-         return grub_error (GRUB_ERR_UNKNOWN_DEVICE,
-                            "device can not be identified");
-       }
+       /* Other Error.  */
+       return grub_error (GRUB_ERR_UNKNOWN_DEVICE,
+                          "device can not be identified");
     }
 
   grub_ata_pio_read (dev, info, GRUB_DISK_SECTOR_SIZE);
 
+  /* Re-check status to avoid bogus identify data due to stuck DRQ.  */
+  grub_uint8_t sts = grub_ata_regget (dev, GRUB_ATA_REG_STATUS);
+  if (sts & (GRUB_ATA_STATUS_BUSY | GRUB_ATA_STATUS_DRQ | GRUB_ATA_STATUS_ERR))
+    {
+      grub_dprintf ("ata", "bad status=0x%x\n", sts);
+      grub_free (info);
+      /* No device, return error but don't print message.  */
+      grub_errno = GRUB_ERR_NONE;
+      return GRUB_ERR_UNKNOWN_DEVICE;
+    }
+
   /* Now it is certain that this is not an ATAPI device.  */
   dev->atapi = 0;
 
@@ -334,26 +354,12 @@
   grub_ata_regset (dev, GRUB_ATA_REG_DISK, dev->device << 4);
   grub_ata_wait ();
 
-  /* If status is 0x00, it is safe to assume that there
-     is no device (or only a !READY) device connected.  */
-  grub_int8_t sts = grub_ata_regget (dev, GRUB_ATA_REG_STATUS);
-  grub_dprintf ("ata", "status=0x%x\n", sts);
-  if (sts == 0x00)
-    {
-      grub_free(dev);
-      return 0;
-    }
-
   /* Try to detect if the port is in use by writing to it,
      waiting for a while and reading it again.  If the value
-     was preserved, there is a device connected.
-     But this tests often detects a second (slave) device
-     connected to a SATA controller which supports only one
-     (master) device.  In this case, the status register
-     check above usually works.  */
+     was preserved, there is a device connected.  */
   grub_ata_regset (dev, GRUB_ATA_REG_SECTORS, 0x5A);  
   grub_ata_wait ();
-  grub_int8_t sec = grub_ata_regget (dev, GRUB_ATA_REG_SECTORS);
+  grub_uint8_t sec = grub_ata_regget (dev, GRUB_ATA_REG_SECTORS);
   grub_dprintf ("ata", "sectors=0x%x\n", sec);
   if (sec != 0x5A)
     {
@@ -361,6 +367,12 @@
       return 0;
     }
 
+  /* The above test may detect a second (slave) device
+     connected to a SATA controller which supports only one
+     (master) device.  It is not safe to use the status register
+     READY bit to check for controller channel existence.  Some
+     ATAPI commands (RESET, DIAGNOSTIC) may clear this bit.  */
+
   /* Use the IDENTIFY DEVICE command to query the device.  */
   if (grub_ata_identify (dev))
     {