[PATCH #upstream-fixes] libata: handle SEMB signature better

[PATCH #upstream-fixes] libata: handle SEMB signature better

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

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: David Haun <drhaun88@gmail.com>
Reported-by: Lars Wirzenius <liw@liw.fi>
Reported-by: Juan Manuel <jmcarranza@gmail.com>
--
 drivers/ata/libata-core.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 065507c..8f7d217 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -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

Re: [PATCH #upstream-fixes] libata: handle SEMB signature better

[Jeff Garzik]

Tejun Heo wrote:
> 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 <tj@kernel.org>
> Reported-by: David Haun <drhaun88@gmail.com>
> Reported-by: Lars Wirzenius <liw@liw.fi>
> Reported-by: Juan Manuel <jmcarranza@gmail.com>
> --
>  drivers/ata/libata-core.c |   19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 065507c..8f7d217 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -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;

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


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

Re: [PATCH #upstream-fixes] libata: handle SEMB signature better

[Tejun Heo]

Hello, Jeff.

Jeff Garzik wrote:
>>      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?

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.

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

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

-- 
tejun

Re: [PATCH #upstream-fixes] libata: handle SEMB signature better

[Jeff Garzik]

Tejun Heo wrote:
> Hello, Jeff.
> 
> Jeff Garzik wrote:
>>>      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?
> 
> 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.
> 
>>> @@ -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.  :-)
> 
>> 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?

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

[PATCH UPDATED #upstream-fixes] libata: handle SEMB signature better

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

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: David Haun <drhaun88@gmail.com>
Reported-by: Lars Wirzenius <liw@liw.fi>
Reported-by: Juan Manuel <jmcarranza@gmail.com>
---
 drivers/ata/libata-core.c |   18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 065507c..a61af38 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -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

Re: [PATCH UPDATED #upstream-fixes] libata: handle SEMB signature better

[Borislav Petkov]

Hi Tejun,

On Wed, Apr 15, 2009 at 06:21:10AM +0900, Tejun Heo wrote:
> 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.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH UPDATED #upstream-fixes] libata: handle SEMB signature better

[Jeff Garzik]

Tejun Heo wrote:
> 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.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: David Haun <drhaun88@gmail.com>
> Reported-by: Lars Wirzenius <liw@liw.fi>
> Reported-by: Juan Manuel <jmcarranza@gmail.com>
> ---
>  drivers/ata/libata-core.c |   18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)

applied