libata: handle SEMB signature better

PATCH In review Tejun Heo 3 participants 7 messages Apr 14, 2009 – Apr 16, 2009 libata

Share & tools

Tejun Heo

author

2009-04-14 10:36 UTC #0 lore raw

PATCH 1/1 libata: handle SEMB signature better

WDC WD1600JS-62MHB5 successfully hits the window between ATA/ATAPI-7 and Serial ATA II standards and reports 3c/c3 signature which now is assigned to SEMB. Make ata_dev_classify() report ATA_DEV_SEMB on the sig and let ata_dev_read_id() work around it by trying IDENTIFY once.

This fixes bko#11579.

Signed-off-by Tejun Heo <[email protected]>

Reported-by David Haun <[email protected]>

Reported-by Lars Wirzenius <[email protected]>

Reported-by Juan Manuel <[email protected]>

 drivers/ata/libata-core.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

drivers/ata/libata-core.c +17 -2

index 065507c..8f7d217 100644
@@ -1231,6 +1231,9 @@ unsigned int ata_dev_classify(const struct ata_taskfile *tf)
 	 *
 	 * We follow the current spec and consider that 0x69/0x96
 	 * identifies a port multiplier and 0x3c/0xc3 a SEMB device.
+	 * Unfortunately, WDC WD1600JS-62MHB5 (a hard drive) reports
+	 * SEMB signature.  This is worked around in
+	 * ata_dev_read_id().
 	 */
 	if ((tf->lbam == 0) && (tf->lbah == 0)) {
 		DPRINTK("found ATA device by sig\n");

@@ -1248,8 +1251,8 @@ unsigned int ata_dev_classify(const struct ata_taskfile *tf)
 	}
 
 	if ((tf->lbam == 0x3c) && (tf->lbah == 0xc3)) {
-		printk(KERN_INFO "ata: SEMB device ignored\n");
-		return ATA_DEV_SEMB_UNSUP; /* not yet */
+		DPRINTK("found SEMB device by sig (could be ATA device)\n");
+		return ATA_DEV_SEMB;
 	}
 
 	DPRINTK("unknown device\n");

@@ -2080,6 +2083,7 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
 	struct ata_taskfile tf;
 	unsigned int err_mask = 0;
 	const char *reason;
+	bool is_semb = class == ATA_DEV_SEMB;
 	int may_fallback = 1, tried_spinup = 0;
 	int rc;

@@ -2090,6 +2094,8 @@ retry:
 	ata_tf_init(dev, &tf);
 
 	switch (class) {
+	case ATA_DEV_SEMB:
+		class = ATA_DEV_ATA;	/* some hard drives report SEMB sig */
 	case ATA_DEV_ATA:
 		tf.command = ATA_CMD_ID_ATA;
 		break;

@@ -2119,6 +2125,7 @@ retry:
 	else
 		err_mask = ata_do_dev_read_id(dev, &tf, id);
 
+	err_mask |= AC_ERR_DEV;
 	if (err_mask) {
 		if (err_mask & AC_ERR_NODEV_HINT) {
 			ata_dev_printk(dev, KERN_DEBUG,

@@ -2126,6 +2133,14 @@ retry:
 			return -ENOENT;
 		}
 
+		if (is_semb) {
+			ata_dev_printk(dev, KERN_INFO, "IDENTIFY failed on "
+				       "device w/ SEMB sig, disabled\n");
+			/* SEMB is not supported yet */
+			*p_class = ATA_DEV_SEMB_UNSUP;
+			return 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

Jeff Garzik

2009-04-14 12:12 UTC #1 lore raw

Quoted 49 lines · Tejun Heo

This fixes bko#11579.

Signed-off-by Tejun Heo <[email protected]>

Reported-by David Haun <[email protected]>

Reported-by Lars Wirzenius <[email protected]>

Reported-by Juan Manuel <[email protected]>

 drivers/ata/libata-core.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

drivers/ata/libata-core.c +6 -2

index 065507c..8f7d217 100644
@@ -1231,6 +1231,9 @@ unsigned int ata_dev_classify(const struct ata_taskfile *tf)
 	 *
 	 * We follow the current spec and consider that 0x69/0x96
 	 * identifies a port multiplier and 0x3c/0xc3 a SEMB device.
+	 * Unfortunately, WDC WD1600JS-62MHB5 (a hard drive) reports
+	 * SEMB signature.  This is worked around in
+	 * ata_dev_read_id().
 	 */
 	if ((tf->lbam == 0) && (tf->lbah == 0)) {
 		DPRINTK("found ATA device by sig\n");

@@ -1248,8 +1251,8 @@ unsigned int ata_dev_classify(const struct ata_taskfile *tf)
 	}
 
 	if ((tf->lbam == 0x3c) && (tf->lbah == 0xc3)) {
-		printk(KERN_INFO "ata: SEMB device ignored\n");
-		return ATA_DEV_SEMB_UNSUP; /* not yet */
+		DPRINTK("found SEMB device by sig (could be ATA device)\n");
+		return ATA_DEV_SEMB;
 	}
 
 	DPRINTK("unknown device\n");

@@ -2080,6 +2083,7 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
 	struct ata_taskfile tf;
 	unsigned int err_mask = 0;
 	const char *reason;
+	bool is_semb = class == ATA_DEV_SEMB;
 	int may_fallback = 1, tried_spinup = 0;
 	int rc;

drivers/ata/libata-core.c retry: · line 2101

Quoted 8 lines

drivers/ata/libata-core.c +2 -0

@@ -2090,6 +2094,8 @@ retry:
 	ata_tf_init(dev, &tf);
 
 	switch (class) {
+	case ATA_DEV_SEMB:
+		class = ATA_DEV_ATA;	/* some hard drives report SEMB sig */
 	case ATA_DEV_ATA:
 		tf.command = ATA_CMD_ID_ATA;
 		break;

Why assign class now, as opposed to after IDENTIFY DEVICE succeeds / fails?

drivers/ata/libata-core.c retry: · line 2131

Quoted 8 lines

drivers/ata/libata-core.c +1 -0

@@ -2119,6 +2125,7 @@ retry:
 	else
 		err_mask = ata_do_dev_read_id(dev, &tf, id);
 
+	err_mask |= AC_ERR_DEV;
 	if (err_mask) {
 		if (err_mask & AC_ERR_NODEV_HINT) {
 			ata_dev_printk(dev, KERN_DEBUG,

Why is err_mask being unconditionally set?

I would think this would make more sense:

	if (!err_mask && class == ATA_DEV_SEMB)
		class = ATA_DEV_ATA;
	else if (err_mask) {
		if (err_mask & AC_ERR_NODEV_HINT) {
		...

Thanks,

Jeff

Tejun Heo

author

2009-04-14 21:04 UTC #2 lore raw

Hello, Jeff.

Quoted 8 lines · Jeff Garzik

drivers/ata/libata-core.c retry: · line 2101
Quoted 6 lines
     switch (class) {
drivers/ata/libata-core.c +2 -0
+    case ATA_DEV_SEMB:
+        class = ATA_DEV_ATA;    /* some hard drives report SEMB sig */
     case ATA_DEV_ATA:
         tf.command = ATA_CMD_ID_ATA;
         break;
Why assign class now, as opposed to after IDENTIFY DEVICE succeeds / fails?

The class variable indicates which class the code is gonna try not the one which is discovered, so it's more consistent to set it before trying it out.

Quoted 11 lines

drivers/ata/libata-core.c retry: · line 2131

Quoted 9 lines

drivers/ata/libata-core.c +1 -0

@@ -2119,6 +2125,7 @@ retry:
     else
         err_mask = ata_do_dev_read_id(dev, &tf, id);
 
+    err_mask |= AC_ERR_DEV;
     if (err_mask) {
         if (err_mask & AC_ERR_NODEV_HINT) {
             ata_dev_printk(dev, KERN_DEBUG,

Why is err_mask being unconditionally set?

Oh.. c**p, that's debug code I forgot to remove. Thanks for spotting it. :-)

Quoted 7 lines

I would think this would make more sense:

    if (!err_mask && class == ATA_DEV_SEMB)
        class = ATA_DEV_ATA;
    else if (err_mask) {
        if (err_mask & AC_ERR_NODEV_HINT) {
        ...

I made the code a fast exit path because I didn't know how an actual SEMB device would respond to it. If the device timeouts IDENTIFYs, retrying can be quite painful. ATA drives w/ SEMB signature being extrmemely rare, I think it's better to trade their slight chance of detection failure but then again it's not like we have a lot of SEMB devices. What do you think?

Thanks.

Signature · 1 line

tejun

Jeff Garzik

2009-04-14 21:08 UTC #3 lore raw

Quoted 36 lines · Tejun Heo

Hello, Jeff.
Quoted 7 lines · Jeff Garzik
drivers/ata/libata-core.c retry: · line 2101
Quoted 6 lines
     switch (class) {
drivers/ata/libata-core.c +2 -0
+    case ATA_DEV_SEMB:
+        class = ATA_DEV_ATA;    /* some hard drives report SEMB sig */
     case ATA_DEV_ATA:
         tf.command = ATA_CMD_ID_ATA;
         break;
Why assign class now, as opposed to after IDENTIFY DEVICE succeeds / fails?
The class variable indicates which class the code is gonna try not the one which is discovered, so it's more consistent to set it before trying it out.
Quoted 9 lines
drivers/ata/libata-core.c retry: · line 2131
Quoted 8 lines
drivers/ata/libata-core.c +1 -0
@@ -2119,6 +2125,7 @@ retry:
     else
         err_mask = ata_do_dev_read_id(dev, &tf, id);
 
+    err_mask |= AC_ERR_DEV;
     if (err_mask) {
         if (err_mask & AC_ERR_NODEV_HINT) {
             ata_dev_printk(dev, KERN_DEBUG,
Why is err_mask being unconditionally set?
Oh.. c**p, that's debug code I forgot to remove. Thanks for spotting it. :-)
Quoted 7 lines
I would think this would make more sense:
    if (!err_mask && class == ATA_DEV_SEMB)
        class = ATA_DEV_ATA;
    else if (err_mask) {
        if (err_mask & AC_ERR_NODEV_HINT) {
        ...

Quoted 6 lines

I made the code a fast exit path because I didn't know how an actual SEMB device would respond to it. If the device timeouts IDENTIFYs, retrying can be quite painful. ATA drives w/ SEMB signature being extrmemely rare, I think it's better to trade their slight chance of detection failure but then again it's not like we have a lot of SEMB devices. What do you think?

That's fine -- it is mainly the unconditional err_mask assignment that confused me. I was trying to figure out the rest of the changes, in the context of that variable assignment :)

Jeff

Tejun Heo

author

2009-04-14 21:22 UTC #4 lore raw

PATCH 1/1 libata: handle SEMB signature better

This fixes bko#11579.

UPDATED: Jeff spotted stupid bug caused by lingering debug code.

Signed-off-by Tejun Heo <[email protected]>

Reported-by David Haun <[email protected]>

Reported-by Lars Wirzenius <[email protected]>

Reported-by Juan Manuel <[email protected]>

---
 drivers/ata/libata-core.c |   18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

drivers/ata/libata-core.c +16 -2

index 065507c..a61af38 100644
@@ -1231,6 +1231,9 @@ unsigned int ata_dev_classify(const struct ata_taskfile *tf)
 	 *
 	 * We follow the current spec and consider that 0x69/0x96
 	 * identifies a port multiplier and 0x3c/0xc3 a SEMB device.
+	 * Unfortunately, WDC WD1600JS-62MHB5 (a hard drive) reports
+	 * SEMB signature.  This is worked around in
+	 * ata_dev_read_id().
 	 */
 	if ((tf->lbam == 0) && (tf->lbah == 0)) {
 		DPRINTK("found ATA device by sig\n");

@@ -1248,8 +1251,8 @@ unsigned int ata_dev_classify(const struct ata_taskfile *tf)
 	}
 
 	if ((tf->lbam == 0x3c) && (tf->lbah == 0xc3)) {
-		printk(KERN_INFO "ata: SEMB device ignored\n");
-		return ATA_DEV_SEMB_UNSUP; /* not yet */
+		DPRINTK("found SEMB device by sig (could be ATA device)\n");
+		return ATA_DEV_SEMB;
 	}
 
 	DPRINTK("unknown device\n");

@@ -2080,6 +2083,7 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
 	struct ata_taskfile tf;
 	unsigned int err_mask = 0;
 	const char *reason;
+	bool is_semb = class == ATA_DEV_SEMB;
 	int may_fallback = 1, tried_spinup = 0;
 	int rc;

@@ -2090,6 +2094,8 @@ retry:
 	ata_tf_init(dev, &tf);
 
 	switch (class) {
+	case ATA_DEV_SEMB:
+		class = ATA_DEV_ATA;	/* some hard drives report SEMB sig */
 	case ATA_DEV_ATA:
 		tf.command = ATA_CMD_ID_ATA;
 		break;

@@ -2126,6 +2132,14 @@ retry:
 			return -ENOENT;
 		}
 
+		if (is_semb) {
+			ata_dev_printk(dev, KERN_INFO, "IDENTIFY failed on "
+				       "device w/ SEMB sig, disabled\n");
+			/* SEMB is not supported yet */
+			*p_class = ATA_DEV_SEMB_UNSUP;
+			return 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

Borislav Petkov

2009-04-16 07:37 UTC #5 lore raw

Hi Tejun,

Quoted 9 lines · Tejun Heo

WDC WD1600JS-62MHB5 successfully hits the window between ATA/ATAPI-7 and Serial ATA II standards and reports 3c/c3 signature which now is assigned to SEMB. Make ata_dev_classify() report ATA_DEV_SEMB on the sig and let ata_dev_read_id() work around it by trying IDENTIFY once.
This fixes bko#11579.
UPDATED: Jeff spotted stupid bug caused by lingering debug code.

by the way, this patch fixes the problem here with a similar WDC which reports the same signature 3c/c3 and gets ignored as a SEMB device initially. With your patch it looks much better:

Apr 16 09:30:05 liondog kernel: ahci 0000:00:11.0: version 3.0
Apr 16 09:30:05 liondog kernel: ahci 0000:00:11.0: PCI INT A -> GSI 22 (level, low) -> IRQ 22
Apr 16 09:30:05 liondog kernel: ahci 0000:00:11.0: AHCI 0001.0100 32 slots 6 ports 3 Gbps 0x3f impl SATA mode
Apr 16 09:30:05 liondog kernel: ahci 0000:00:11.0: flags: 64bit ncq sntf ilck pm led clo pmp pio slum part 
Apr 16 09:30:05 liondog kernel: scsi0 : ahci
Apr 16 09:30:05 liondog kernel: scsi1 : ahci
Apr 16 09:30:05 liondog kernel: scsi2 : ahci
Apr 16 09:30:05 liondog kernel: scsi3 : ahci
Apr 16 09:30:05 liondog kernel: scsi4 : ahci
Apr 16 09:30:05 liondog kernel: scsi5 : ahci
Apr 16 09:30:05 liondog kernel: ata1: SATA max UDMA/133 abar m1024@0xfe9ff800 port 0xfe9ff900 irq 22
Apr 16 09:30:05 liondog kernel: ata2: SATA max UDMA/133 abar m1024@0xfe9ff800 port 0xfe9ff980 irq 22
Apr 16 09:30:05 liondog kernel: ata3: SATA max UDMA/133 abar m1024@0xfe9ff800 port 0xfe9ffa00 irq 22
Apr 16 09:30:05 liondog kernel: ata4: SATA max UDMA/133 abar m1024@0xfe9ff800 port 0xfe9ffa80 irq 22
Apr 16 09:30:05 liondog kernel: ata5: SATA max UDMA/133 abar m1024@0xfe9ff800 port 0xfe9ffb00 irq 22
Apr 16 09:30:05 liondog kernel: ata6: SATA max UDMA/133 abar m1024@0xfe9ff800 port 0xfe9ffb80 irq 22
Apr 16 09:30:05 liondog kernel: ata1: SATA link down (SStatus 0 SControl 300)
Apr 16 09:30:05 liondog kernel: ata2: softreset failed (device not ready)
Apr 16 09:30:05 liondog kernel: ata2: failed due to HW bug, retry pmp=0
Apr 16 09:30:05 liondog kernel: ata2: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
Apr 16 09:30:05 liondog kernel: ata2.00: ATA-8: WDC WD2500AAJS-62B4A0, 01.03A01, max UDMA/133
Apr 16 09:30:05 liondog kernel: ata2.00: 488397168 sectors, multi 16: LBA48 NCQ (depth 31/32)
Apr 16 09:30:05 liondog kernel: ata2.00: configured for UDMA/133

Thanks.

Signature · 2 lines

Regards/Gruss,
    Boris.

Jeff Garzik

Applied

2009-04-16 19:22 UTC #6 lore raw

In reply to Tejun Heo

Quoted 9 lines · Tejun Heo

WDC WD1600JS-62MHB5 successfully hits the window between ATA/ATAPI-7 and Serial ATA II standards and reports 3c/c3 signature which now is assigned to SEMB. Make ata_dev_classify() report ATA_DEV_SEMB on the sig and let ata_dev_read_id() work around it by trying IDENTIFY once.
This fixes bko#11579.
UPDATED: Jeff spotted stupid bug caused by lingering debug code.

Quoted 7 lines

Signed-off-by Tejun Heo <[email protected]>
Reported-by David Haun <[email protected]>
Reported-by Lars Wirzenius <[email protected]>
Reported-by Juan Manuel <[email protected]>
---
 drivers/ata/libata-core.c |   18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

applied