Skip to contentSkip to conversation

libata: assume no device is attached if both IDENTIFYs are aborted

PATCH Applied Tejun Heo 4 participants 7 messages Mar 23, 2008 – Mar 26, 2008 libata

lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

PATCH 1/1 libata: assume no device is attached if both IDENTIFYs are aborted

This is to fix bugzilla #10254. QSI cdrom attached to pata_sis as secondary master appears as phantom device for the slave. Interestingly, instead of not setting DRQ after IDENTIFY which triggers NODEV_HINT, it aborts both IDENTIFY and IDENTIFY PACKET which makes EH retry.

Modify EH such that it assumes no device is attached if both flavors of IDENTIFY are aborted by the device. There really isn't much point in retrying when the device actively aborts the commands.

While at it, convert NODEV detection message to ata_dev_printk() to help debugging obscure detection problems.

This problem was reported by Jan Bücken.

---
Alan, does this look okay?

Thanks.

 drivers/ata/libata-core.c |   38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)
drivers/ata/libata-core.c +24 -14
index 4bbe31f..c9c5280 100644
@@ -2092,24 +2092,34 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
 				     id, sizeof(id[0]) * ATA_ID_WORDS, 0);
 	if (err_mask) {
 		if (err_mask & AC_ERR_NODEV_HINT) {
-			DPRINTK("ata%u.%d: NODEV after polling detection\n",
-				ap->print_id, dev->devno);
+			ata_dev_printk(dev, KERN_DEBUG,
+				       "NODEV after polling detection\n");
 			return -ENOENT;
 		}
 
-		/* Device or controller might have reported the wrong
-		 * device class.  Give a shot at the other IDENTIFY if
-		 * the current one is aborted by the device.
-		 */
-		if (may_fallback &&
-		    (err_mask == AC_ERR_DEV) && (tf.feature & ATA_ABORTED)) {
-			may_fallback = 0;
+		if ((err_mask == AC_ERR_DEV) && (tf.feature & ATA_ABORTED)) {
+			/* Device or controller might have reported
+			 * the wrong device class.  Give a shot at the
+			 * other IDENTIFY if the current one is
+			 * aborted by the device.
+			 */
+			if (may_fallback) {
+				may_fallback = 0;
 
-			if (class == ATA_DEV_ATA)
-				class = ATA_DEV_ATAPI;
-			else
-				class = ATA_DEV_ATA;
-			goto retry;
+				if (class == ATA_DEV_ATA)
+					class = ATA_DEV_ATAPI;
+				else
+					class = ATA_DEV_ATA;
+				goto retry;
+			}
+
+			/* Control reaches here iff the device aborted
+			 * both flavors of IDENTIFYs which happens
+			 * sometimes with phantom devices.
+			 */
+			ata_dev_printk(dev, KERN_DEBUG,
+				       "both IDENTIFYs aborted, assuming NODEV\n");
+			return -ENOENT;
 		}
 
 		rc = -EIO;
Acked-by on v1 1/1
#1 lore raw
Quoted 13 lines · On Sun, 23 Mar 2008 15:16:53 +0900 Tejun Heo <[email protected]> wrote:

This is to fix bugzilla #10254. QSI cdrom attached to pata_sis as secondary master appears as phantom device for the slave. Interestingly, instead of not setting DRQ after IDENTIFY which triggers NODEV_HINT, it aborts both IDENTIFY and IDENTIFY PACKET which makes EH retry.

Modify EH such that it assumes no device is attached if both flavors of IDENTIFY are aborted by the device. There really isn't much point in retrying when the device actively aborts the commands.

While at it, convert NODEV detection message to ata_dev_printk() to help debugging obscure detection problems.

Quoted 6 lines

This problem was reported by Jan Bücken.

---
Alan, does this look okay?

Applied
#2 lore raw
In reply to Tejun Heo
Quoted 17 lines · Tejun Heo

This is to fix bugzilla #10254. QSI cdrom attached to pata_sis as secondary master appears as phantom device for the slave. Interestingly, instead of not setting DRQ after IDENTIFY which triggers NODEV_HINT, it aborts both IDENTIFY and IDENTIFY PACKET which makes EH retry.

Modify EH such that it assumes no device is attached if both flavors of IDENTIFY are aborted by the device. There really isn't much point in retrying when the device actively aborts the commands.

While at it, convert NODEV detection message to ata_dev_printk() to help debugging obscure detection problems.

This problem was reported by Jan Bücken.

Quoted 7 lines

---
Alan, does this look okay?

Thanks.

 drivers/ata/libata-core.c |   38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

applied

Acked-by on v1 1/1
#3 lore raw
In reply to Tejun Heo

Tejun Heo wrote:
..

Quoted 3 lines

Modify EH such that it assumes no device is attached if both flavors of IDENTIFY are aborted by the device. There really isn't much point in retrying when the device actively aborts the commands.

..

And thus dies support for the few very early IDE drives that lacked IDENTIFY.

R.I.P. :)

(not that they would have worked with libata in the first place)

author
#4 lore raw
Quoted 5 lines · Mark Lord

Tejun Heo wrote:
..

Quoted 3 lines

Modify EH such that it assumes no device is attached if both flavors of IDENTIFY are aborted by the device. There really isn't much point in retrying when the device actively aborts the commands.

Quoted 8 lines

..

And thus dies support for the few very early IDE drives that lacked IDENTIFY.

R.I.P. :)

(not that they would have worked with libata in the first place)

How did they work anyway? By specifying geometry manually to the driver? I think we can do that. We just need another cute HORKAGE. ATA_HORKAGE_NO_IDENITFY and some massaging around EH to handle it. Heh... That's gonna be a silly but fun project. :-)

Signature · 1 line
tejun
#5 lore raw
Quoted 11 lines · Tejun Heo
Quoted 10 lines · Mark Lord

Tejun Heo wrote:
..

Quoted 3 lines

Modify EH such that it assumes no device is attached if both flavors of IDENTIFY are aborted by the device. There really isn't much point in retrying when the device actively aborts the commands.

..

And thus dies support for the few very early IDE drives that lacked IDENTIFY.

Quoted 8 lines
Quoted 3 lines

R.I.P. :)

(not that they would have worked with libata in the first place)

How did they work anyway? By specifying geometry manually to the driver? I think we can do that. We just need another cute HORKAGE. ATA_HORKAGE_NO_IDENITFY and some massaging around EH to handle it. Heh... That's gonna be a silly but fun project. :-)

..

Geometry from CMOS, BIOS, partition table, or kernel command line.

But I haven't owned one for perhaps 15 years or more, and they really are NOT WORTH IT in libata -- all of those nice little ata_id_* macros would be affected.

If one did want to do it, I think the best approach would be to generate a fake drive ID block, and populate it with suitably pre-ATA1 era default values. Then the rest of libata would not require modifications.

Cheers

#6 lore raw
Quoted 7 lines
Quoted 4 lines

How did they work anyway? By specifying geometry manually to the driver? I think we can do that. We just need another cute HORKAGE. ATA_HORKAGE_NO_IDENITFY and some massaging around EH to handle it. Heh... That's gonna be a silly but fun project. :-)

..

Geometry from CMOS, BIOS, partition table, or kernel command line.

Thats the only bit we really need - I've already got the rest of the support code back to MFM drives sitting in my test tree waiting a finish and polish. The actual probe code for the initial detect is quite different anyway - you either rely on BIOS settings or you watch TRKZ and try to restore to track zero.

The needed fake block is pretty small

        memset(buf, 0, 512);
        id[0] = 0x8000; /* ATA */  
        id[1] = d->cyls;
        id[2] = 0xC837;
        id[3] = d->heads;
        id[6] = d->sectors;
        memset(id + 10, " ", 20);
        /* FIXME: Set some kind of unique serial */
        memcpy(id + 23, "ATAHD001", 8);
        memcpy(id + 27, "ATA HD EMULATION OF MFM/RLL             ", 40);
        id[47] = 0x8000;
        id[49] = 0x30;

In fact if we had the proposed memcpy_to_sg/memcpy_from_sg type interfaces discussed between Matthew Wilcox and Jeff in the download microcode thread we could actually intercept IDENTIFY in the MFM driver and fix it up there (after all we can't issue it to the MFM/RLL controller).

Alan