[PATCH #upstream-fixes] libata: assume no device is attached if both IDENTIFYs are aborted

[PATCH #upstream-fixes] libata: assume no device is attached if both IDENTIFYs are aborted

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

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Jan Bücken <jb.faq@gmx.de>
---
Alan, does this look okay?

Thanks.

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

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 4bbe31f..c9c5280 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -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;

Re: [PATCH #upstream-fixes] libata: assume no device is attached if both IDENTIFYs are aborted

[Alan Cox]

On Sun, 23 Mar 2008 15:16:53 +0900
Tejun Heo <htejun@gmail.com> 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.
> 
> This problem was reported by Jan Bücken.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Jan Bücken <jb.faq@gmx.de>
> ---
> Alan, does this look okay?

Acked-by: Alan Cox <alan@redhat.com>

Re: [PATCH #upstream-fixes] libata: assume no device is attached if both IDENTIFYs are aborted

[Jeff Garzik]

Tejun Heo 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.
> 
> This problem was reported by Jan Bücken.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Jan Bücken <jb.faq@gmx.de>
> ---
> Alan, does this look okay?
> 
> Thanks.
> 
>  drivers/ata/libata-core.c |   38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)

applied

Re: [PATCH #upstream-fixes] libata: assume no device is attached if both IDENTIFYs are aborted

[Mark Lord]

Tejun Heo wrote:
..
> 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)

Acked-by: Mark Lord <mlord@pobox.com>

Re: [PATCH #upstream-fixes] libata: assume no device is attached if both IDENTIFYs are aborted

[Tejun Heo]

Mark Lord wrote:
> Tejun Heo wrote:
> ..
>> 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)

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.  :-)

-- 
tejun

Re: [PATCH #upstream-fixes] libata: assume no device is attached if both IDENTIFYs are aborted

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>> Tejun Heo wrote:
>> ..
>>> 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)
> 
> 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

Re: [PATCH #upstream-fixes] libata: assume no device is attached if both IDENTIFYs are aborted

[Alan Cox]

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