2009-05-25 Christian Franke <franke@computer.org>

	* 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.

diff --git a/ChangeLog b/ChangeLog
index eac7ca7079081ce809c6c0f69e46338a73c5aa29..60eb17dd3ffdc0609354fc5c9f659588008ccae6 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2009-05-25  Christian Franke  <franke@computer.org>
+
+       * 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  <colin@gibibit.com>
 
        Cleaned up `include/grub/normal.h'.  Grouped prototypes by

diff --git a/disk/ata.c b/disk/ata.c
index ea42d59518e2cf5260961abb07dc9e21b9af69f2..b186f6dc9ac0fbc5a0b40411286ef94ccb657690 100644 (file)
--- a/disk/ata.c
+++ b/disk/ata.c
@@ -41,11 +41,14 @@ grub_ata_wait_not_busy (struct grub_ata_device *dev, int milliseconds)
   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 @@ grub_atapi_identify (struct grub_ata_device *dev)
     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 @@ grub_ata_identify (struct grub_ata_device *dev)
   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 @@ grub_ata_identify (struct grub_ata_device *dev)
 
   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_device_initialize (int port, int device, int addr, int addr2)
   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 @@ grub_ata_device_initialize (int port, int device, int addr, int addr2)
       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))
     {