[PATCH libata-dev-2.6 1/2] ata_dev_identify() check status fix

[PATCH libata-dev-2.6 1/2] ata_dev_identify() check status fix

[Albert Lee]

[-- Attachment #1: Type: text/plain, Size: 4186 bytes --]

Hi Jeff,

    In the ata_dev_identify() function, ata_chk_status() is used to check whether an error has occurred.
However, in some error situation, the device's status register always reads 0x0. (Please see attached dmesg below.)
And the error found by the state machine is not seen by ata_dev_identify().

Changes:
- use qc->private_data to carry the drv_stat returned from the state machine.
- ata_qc_complete_noop() saves the drv_stat to *(qc->private_data).
- ata_dev_identify() use the status returned from ata_qc_complete_noop(), instead of ata_chk_status().
- noisy PIO error printk() changed to DPRINTK()

Attached please find the patch 1/2 against the libata-dev-2.6 tree for your review. Thanks.

Albert

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---------------------------------------
dmesg:
libata version 1.10 loaded.
pata_pdc2027x version 0.58
pata_pdc2027x: PLL input clock 32783 kHz
ata_device_add: ENTER
ata_host_add: ENTER
ata_port_start: prd alloc, virt c0000001ded82000, dma 19ae0000
ata1: PATA max UDMA/133 cmd 0xDE400 ctl 0xDDC02 bmdma 0xDEC00 irq 134
ata_host_add: ENTER
ata_port_start: prd alloc, virt c0000001de8c6000, dma 19af0000
ata2: PATA max UDMA/133 cmd 0xDE800 ctl 0xDE002 bmdma 0xDEC08 irq 134
ata_device_add: probe begin
ata_device_add: ata1: probe begin
ata_bus_reset: ENTER, host 1, port 0
   ata_pio_devchk: dev check found dev 0
   ata_pio_devchk: dev check found dev 1       ==> Phantom dev 1 passed ata_devchk()
   ata_bus_softreset: ata1: bus reset via SRST
   ata_dev_classify: found ATAPI device by sig
   ata_dev_classify: found ATAPI device by sig ==> Phantom dev 1 passed the EDD signature check
ata_bus_reset: EXIT
ata_dev_identify: ENTER, host 1, dev 0
   ata_dev_select: ENTER, ata1: device 0, wait 1
   ata_dev_identify: do ATAPI identify
     ata_dev_select: ENTER, ata1: device 0, wait 1
     ata_exec_command_pio: ata1: cmd 0xA1
     ata_pio_sector: data read
     ata_qc_complete: EXIT
       ata1: dev 0 cfg 49:0f00 82:4210 83:4000 84:4000 85:0000 86:0000 87:0000 88:0407
       ata_dump_id: 49==0x0f00  53==0x0006  63==0x0007  64==0x0003  75==0x0000
       ata_dump_id: 80==0x0078  81==0x0000  82==0x4210  83==0x4000  84==0x4000
       ata_dump_id: 88==0x0407  93==0x4000
     ata1: dev 0 ATAPI, max UDMA/33
ata_dev_identify: EXIT, drv_stat = 0x50

ata_dev_identify: ENTER, host 1, dev 1
   ata_dev_select: ENTER, ata1: device 1, wait 1
   ata_dev_identify: do ATAPI identify
     ata_dev_select: ENTER, ata1: device 1, wait 1
     ata_exec_command_pio: ata1: cmd 0xA1
     ata1: PIO error, drv_stat 0x0  ==> error. Phantom dev 1 not responding to the IDENTIFY PACKET DEVICE command
     ata_qc_complete: EXIT          ==> drv_stat 0x0, error not seen by ata_dev_identify().
     ata1: dev 1 cfg 49:0000 82:0000 83:0000 84:0000 85:0000 86:0000 87:0000 88:0000
       ata1: no dma/lba   ==> incorrect dev->id[] dumped by ata_dev_identify()
     ata1: dev 1 not supported, ignoring
ata_dev_identify: EXIT, err

---------------------------------------

--- libata-dev-2.6/drivers/scsi/libata-core.c	2005-03-31 21:20:17.000000000 +0800
+++ libata-dev-2.6-mod/drivers/scsi/libata-core.c	2005-04-01 11:21:22.000000000 +0800
@@ -952,7 +952,7 @@
  	unsigned int major_version;
  	u16 tmp;
  	unsigned long xfer_modes;
-	u8 status;
+	u8 status = 0;
  	unsigned int using_edd;
  	DECLARE_COMPLETION(wait);
  	struct ata_queued_cmd *qc;
@@ -995,6 +995,7 @@
  	}

  	qc->waiting = &wait;
+	qc->private_data = &status;
  	qc->complete_fn = ata_qc_complete_noop;

  	spin_lock_irqsave(&ap->host_set->lock, flags);
@@ -1006,7 +1007,6 @@
  	else
  		wait_for_completion(&wait);

-	status = ata_chk_status(ap);
  	if (status & ATA_ERR) {
  		/*
  		 * arg!  EDD works for all test cases, but seems to return
@@ -2496,8 +2496,7 @@
  	assert(qc != NULL);

  	drv_stat = ata_chk_status(ap);
-	printk(KERN_WARNING "ata%u: PIO error, drv_stat 0x%x\n",
-	       ap->id, drv_stat);
+	DPRINTK("ata%u: PIO error, drv_stat 0x%x\n", ap->id, drv_stat);

  	ap->pio_task_state = PIO_ST_IDLE;

@@ -2770,6 +2769,7 @@

  static int ata_qc_complete_noop(struct ata_queued_cmd *qc, u8 drv_stat)
  {
+	*((u8*)qc->private_data) = drv_stat;
  	return 0;
  }



[-- Attachment #2: id_fix.diff --]
[-- Type: text/plain, Size: 1157 bytes --]

--- libata-dev-2.6/drivers/scsi/libata-core.c	2005-03-31 21:20:17.000000000 +0800
+++ libata-dev-2.6-mod/drivers/scsi/libata-core.c	2005-04-01 11:21:22.000000000 +0800
@@ -952,7 +952,7 @@
 	unsigned int major_version;
 	u16 tmp;
 	unsigned long xfer_modes;
-	u8 status;
+	u8 status = 0;
 	unsigned int using_edd;
 	DECLARE_COMPLETION(wait);
 	struct ata_queued_cmd *qc;
@@ -995,6 +995,7 @@
 	}
 
 	qc->waiting = &wait;
+	qc->private_data = &status;
 	qc->complete_fn = ata_qc_complete_noop;
 
 	spin_lock_irqsave(&ap->host_set->lock, flags);
@@ -1006,7 +1007,6 @@
 	else
 		wait_for_completion(&wait);
 
-	status = ata_chk_status(ap);
 	if (status & ATA_ERR) {
 		/*
 		 * arg!  EDD works for all test cases, but seems to return
@@ -2496,8 +2496,7 @@
 	assert(qc != NULL);
 
 	drv_stat = ata_chk_status(ap);
-	printk(KERN_WARNING "ata%u: PIO error, drv_stat 0x%x\n",
-	       ap->id, drv_stat);
+	DPRINTK("ata%u: PIO error, drv_stat 0x%x\n", ap->id, drv_stat);
 
 	ap->pio_task_state = PIO_ST_IDLE;
 
@@ -2770,6 +2769,7 @@
 
 static int ata_qc_complete_noop(struct ata_queued_cmd *qc, u8 drv_stat)
 {
+	*((u8*)qc->private_data) = drv_stat;
 	return 0;
 }
 

Re: [PATCH libata-dev-2.6 1/2] ata_dev_identify() check status fix

[Brett Russ]

Albert Lee wrote:
> -    printk(KERN_WARNING "ata%u: PIO error, drv_stat 0x%x\n",
> -           ap->id, drv_stat);
> +    DPRINTK("ata%u: PIO error, drv_stat 0x%x\n", ap->id, drv_stat);

You'd probably want this to stay a printk so it persists when ATA_DEBUG 
is off, right?

BR

Re: [PATCH libata-dev-2.6 1/2] ata_dev_identify() check status fix

[Albert Lee]

Brett Russ wrote:

> Albert Lee wrote:
> 
>> -    printk(KERN_WARNING "ata%u: PIO error, drv_stat 0x%x\n",
>> -           ap->id, drv_stat);
>> +    DPRINTK("ata%u: PIO error, drv_stat 0x%x\n", ap->id, drv_stat);
> 
> 
> You'd probably want this to stay a printk so it persists when ATA_DEBUG 
> is off, right?
> 
> BR

Hi Brett,

The problem is ata_pio_error() doesn't know whether the error condition is normal or not.
Only the caller that issued the command knows what's going on.
Please see the attached dmesg for an example.
If we print out error message each time ata_pio_error() is called, it might be too noisy.

Albert
============================================================================================================
Mar 31 18:57:28 linux kernel: libata version 1.10 loaded.
Mar 31 18:57:28 linux kernel: pata_pdc2027x version 0.58
Mar 31 18:57:28 linux kernel: pata_pdc2027x: PLL input clock 65552 kHz
Mar 31 18:57:28 linux kernel: ata1: PATA max UDMA/133 cmd 0xDE400 ctl 0xDDC02 bmdma 0xDEC00 irq 134
Mar 31 18:57:28 linux kernel: ata2: PATA max UDMA/133 cmd 0xDE800 ctl 0xDE002 bmdma 0xDEC08 irq 134
Mar 31 18:57:28 linux kernel: ata1: dev 0 cfg 49:0f00 82:4210 83:4000 84:4000 85:0000 86:0000 87:0000 88:0407
Mar 31 18:57:28 linux kernel: ata1: dev 0 ATAPI, max UDMA/33
Mar 31 18:57:29 linux kernel: ata1: PIO error, drv_stat 0x0    <== We have single master (dev 0) configuration here.
                                <== The error was caused by phantom slave device. (please ref ATA-4 9.16.1 and 8.9.5)
                                <== It is normal and the end users should not see this as error.
Mar 31 18:57:29 linux kernel: ata1: dev 0 configured for UDMA/33
Mar 31 18:57:29 linux kernel: scsi11 : pata_pdc2027x
Mar 31 18:57:29 linux kernel: ATA: abnormal status 0x8 on port 0xDE807
Mar 31 18:57:29 linux kernel: ata2: disabling port
Mar 31 18:57:29 linux kernel: scsi12 : pata_pdc2027x
Mar 31 18:57:29 linux kernel:   Vendor: IBM       Model: CD-RW 1234        Rev: 123
Mar 31 18:57:29 linux kernel:   Type:   CD-ROM                             ANSI SCSI revision: 05
Mar 31 18:57:29 linux kernel: Attached scsi generic sg3 at scsi11, channel 0, id 0, lun 0,  type 5
Mar 31 18:57:29 linux /etc/hotplug/scsi_generic.agent[22556]: cd: can't cd to /sys/class/scsi_generic/sg3/device/block
Mar 31 18:57:29 linux kernel: sr0: scsi3-mmc drive: 52x/52x writer cd/rw xa/form2 cdda tray
Mar 31 18:57:29 linux kernel: Attached scsi CD-ROM sr0 at scsi11, channel 0, id 0, lun 0
Mar 31 18:57:29 linux /etc/hotplug/block.agent[22638]: new block device /block/sr0

Re: [PATCH libata-dev-2.6 1/2] ata_dev_identify() check status fix

[Albert Lee]

[-- Attachment #1: Type: text/plain, Size: 952 bytes --]

Hi Jeff,

    Revised since lowlevel driver might use qc->private_data. qc->upper_private_data added instead.

Desc:
    In the ata_dev_identify() function, ata_chk_status() is used to check whether an error has occurred.
However, in some error situation, the device's status register always reads 0x0.
(Please see previously attached dmesg for detail.)
And the error found by the state machine is not seen by ata_dev_identify().

Changes:
- upper_private_data added to ata_queued_cmd
- use qc->upper_private_data to carry the drv_stat returned from the state machine.
- ata_qc_complete_noop() saves the drv_stat to *(qc->upper_private_data).
- ata_dev_identify() use the status returned from ata_qc_complete_noop(), instead of ata_chk_status().
- noisy PIO error printk() changed to DPRINTK()

Attached please find the revised patch 1/2 against the libata-dev-2.6 tree for your review. Thanks.

Albert

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

[-- Attachment #2: id_fix2.diff --]
[-- Type: text/plain, Size: 1211 bytes --]

--- 1.74/include/linux/libata.h	2005-03-08 13:59:27 +08:00
+++ 1.75/include/linux/libata.h	2005-04-02 22:22:11 +08:00
@@ -256,6 +256,9 @@
 
 	struct completion	*waiting;
 
+	/* reserved for owner of this ata_queued_cmd */
+	void			*upper_private_data;
+
 	void			*private_data;
 };
 
--- 1.134/drivers/scsi/libata-core.c	2005-03-08 13:59:27 +08:00
+++ 1.135/drivers/scsi/libata-core.c	2005-04-02 22:30:53 +08:00
@@ -995,6 +995,7 @@
 	}
 
 	qc->waiting = &wait;
+	qc->upper_private_data = &status;
 	qc->complete_fn = ata_qc_complete_noop;
 
 	spin_lock_irqsave(&ap->host_set->lock, flags);
@@ -1006,7 +1007,6 @@
 	else
 		wait_for_completion(&wait);
 
-	status = ata_chk_status(ap);
 	if (status & ATA_ERR) {
 		/*
 		 * arg!  EDD works for all test cases, but seems to return
@@ -2496,8 +2496,7 @@
 	assert(qc != NULL);
 
 	drv_stat = ata_chk_status(ap);
-	printk(KERN_WARNING "ata%u: PIO error, drv_stat 0x%x\n",
-	       ap->id, drv_stat);
+	DPRINTK("ata%u: PIO error, drv_stat 0x%x\n", ap->id, drv_stat);
 
 	ap->pio_task_state = PIO_ST_IDLE;
 
@@ -2770,6 +2769,7 @@
 
 static int ata_qc_complete_noop(struct ata_queued_cmd *qc, u8 drv_stat)
 {
+	*((u8*)qc->upper_private_data) = drv_stat;
 	return 0;
 }
 

Re: [PATCH libata-dev-2.6 1/2] ata_dev_identify() check status fix

[Jeff Garzik]

Could we not pass the Status value via struct ata_taskfile ?

	Jeff

Re: [PATCH libata-dev-2.6 1/2] ata_dev_identify() check status fix

[Albert Lee]

Jeff Garzik wrote:
> Could we not pass the Status value via struct ata_taskfile ?
> 
> 

Jeff,

I'm worried about:

1. struct ata_taskfile is part of struct ata_queued_cmd. The lifecycle of ata_queued_cmd is over
    after __ata_qc_complete() is called. So, when wait_for_completion() returns,
    qc has already been recycled for next use.

2. The following code is seen in ata_pio_error():
      ata_qc_complete(qc, drv_stat | ATA_ERR);
    It looks a good design of libata to me: ata_qc_complete() gets both qc and an additional status parameter.
The status paramerter is mixed of real hardware status and error found by the software.
Instead put the software error to qc->tf.status, maybe we could keep this good design and leave
struct ata_taskfile for the hardware only?


Albert
=====================
(The code in context.)

	spin_lock_irqsave(&ap->host_set->lock, flags);
	rc = ata_qc_issue(qc);
	spin_unlock_irqrestore(&ap->host_set->lock, flags);

	if (rc)
		goto err_out;
	else
		wait_for_completion(&wait);

	// When wait_for_completion() returns,
	// the life cycle of qc was over and
	// qc->tag == ATA_TAG_POISON; here.
	// Maybe we should not access qc here.
	status = qc->tf.status;

	if (status & ATA_ERR) {
		/*
		 * arg!  EDD works for all test cases, but seems to return
		 * the ATA signature for some ATAPI devices.  Until the
		 * reason for this is found and fixed, we fix up the mess
		 * here.  If IDENTIFY DEVICE returns command aborted
		 * (as ATAPI devices do), then we issue an
		 * IDENTIFY PACKET DEVICE.
		 *
		 * ATA software reset (SRST, the default) does not appear
		 * to have this problem.
		 */
		if ((using_edd) && (qc->tf.command == ATA_CMD_ID_ATA)) {
			u8 err = ata_chk_err(ap);
			if (err & ATA_ABORTED) {
				dev->class = ATA_DEV_ATAPI;
				//
				// We should call ata_qc_new_init()
				// to revive qc here?
				//
				qc->cursg = 0;
				qc->cursg_ofs = 0;
				qc->cursect = 0;
				qc->nsect = 1;
				goto retry;
			}
		}
		goto err_out;
	}