[PATCH 2/3] libata - PIO error handling fix

[PATCH 2/3] libata - PIO error handling fix

[Albert Lee]

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

Hi, Jeff:

  Tested burning CD-RW with libata-dev-2.6 and cdrecord:
1. ATAPI DMA mode - tested OK
2. ATAPI PIO mode - test failed when cdrecord finishes burning and issues MODE_SELECT to the device.

  After checking the log, it showed that MODE_SELECT caused ata_pio_complete() to return error.
However, the error is not handled by ata_pio_task().

Attached please find the patch for ata_pio_task() error handling for your review.
(The patch is against the libata-dev-2.6 tree. )

Changes in the patch:
1. End the PIO task when PIO_ST_IDLE state is entered
2. End the PIO task after PIO_ST_TMOUT and PIO_ST_ERR state handled by ata_pio_error()
3. Remove the first "if" statement to handle the error condition returned from 
  ata_pio_block(), ata_pio_complete() and ata_pio_poll().

  Change #2  is not so necessary since ata_pio_error() will put the cmd to  PIO_ST_IDLE state
after the error condition is handled. The change just saves a function call to queue_work().

Tested OK on on my machine with pdc20275 and ASUS CD-RW drive.

Albert

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
diff -Nru libata-dev-2.6-ori/drivers/scsi/libata-core.c libata-dev-2.6/drivers/scsi/libata-core.c
--- libata-dev-2.6-ori/drivers/scsi/libata-core.c 2004-11-30 12:52:28.000000000 +0800
+++ libata-dev-2.6/drivers/scsi/libata-core.c 2004-12-02 18:57:48.719220063 +0800
@@ -2399,6 +2399,9 @@
  unsigned long timeout = 0;
 
  switch (ap->pio_task_state) {
+ case PIO_ST_IDLE:
+  return;
+
  case PIO_ST:
   ata_pio_block(ap);
   break;
@@ -2415,18 +2418,14 @@
  case PIO_ST_TMOUT:
  case PIO_ST_ERR:
   ata_pio_error(ap);
-  break;
+  return;
  }
 
- if ((ap->pio_task_state != PIO_ST_IDLE) &&
-     (ap->pio_task_state != PIO_ST_TMOUT) &&
-     (ap->pio_task_state != PIO_ST_ERR)) {
-  if (timeout)
-   queue_delayed_work(ata_wq, &ap->pio_task,
-        timeout);
-  else
-   queue_work(ata_wq, &ap->pio_task);
- }
+ if (timeout)
+  queue_delayed_work(ata_wq, &ap->pio_task,
+       timeout);
+ else
+  queue_work(ata_wq, &ap->pio_task);
 }
 
 static void atapi_request_sense(struct ata_port *ap, struct ata_device *dev,

[-- Attachment #2: ata_pio_task.patch --]
[-- Type: application/octet-stream, Size: 1055 bytes --]

diff -Nru libata-dev-2.6-ori/drivers/scsi/libata-core.c libata-dev-2.6/drivers/scsi/libata-core.c
--- libata-dev-2.6-ori/drivers/scsi/libata-core.c	2004-11-30 12:52:28.000000000 +0800
+++ libata-dev-2.6/drivers/scsi/libata-core.c	2004-12-02 18:57:48.719220063 +0800
@@ -2399,6 +2399,9 @@
 	unsigned long timeout = 0;
 
 	switch (ap->pio_task_state) {
+	case PIO_ST_IDLE:
+		return;
+
 	case PIO_ST:
 		ata_pio_block(ap);
 		break;
@@ -2415,18 +2418,14 @@
 	case PIO_ST_TMOUT:
 	case PIO_ST_ERR:
 		ata_pio_error(ap);
-		break;
+		return;
 	}
 
-	if ((ap->pio_task_state != PIO_ST_IDLE) &&
-	    (ap->pio_task_state != PIO_ST_TMOUT) &&
-	    (ap->pio_task_state != PIO_ST_ERR)) {
-		if (timeout)
-			queue_delayed_work(ata_wq, &ap->pio_task,
-					   timeout);
-		else
-			queue_work(ata_wq, &ap->pio_task);
-	}
+	if (timeout)
+		queue_delayed_work(ata_wq, &ap->pio_task,
+				   timeout);
+	else
+		queue_work(ata_wq, &ap->pio_task);
 }
 
 static void atapi_request_sense(struct ata_port *ap, struct ata_device *dev,

Re: [PATCH 2/3] libata - PIO error handling fix

[Bartlomiej Zolnierkiewicz]

On Wed, 15 Dec 2004 17:50:59 +0800, Albert Lee <albertcc@tw.ibm.com> wrote:
> Hi, Jeff:
> 
>   Tested burning CD-RW with libata-dev-2.6 and cdrecord:
> 1. ATAPI DMA mode - tested OK
> 2. ATAPI PIO mode - test failed when cdrecord finishes burning and issues MODE_SELECT to the device.
> 
>   After checking the log, it showed that MODE_SELECT caused ata_pio_complete() to return error.
> However, the error is not handled by ata_pio_task().
> 
> Attached please find the patch for ata_pio_task() error handling for your review.
> (The patch is against the libata-dev-2.6 tree. )
> 
> Changes in the patch:
> 1. End the PIO task when PIO_ST_IDLE state is entered
> 2. End the PIO task after PIO_ST_TMOUT and PIO_ST_ERR state handled by ata_pio_error()
> 3. Remove the first "if" statement to handle the error condition returned from
>   ata_pio_block(), ata_pio_complete() and ata_pio_poll().

Jeff, 3. is the same issue + fix that was reported earlier by Tim.
It seems that we have both somehow forgot about it. :-|

>   Change #2  is not so necessary since ata_pio_error() will put the cmd to  PIO_ST_IDLE state
> after the error condition is handled. The change just saves a function call to queue_work().
> 
> Tested OK on on my machine with pdc20275 and ASUS CD-RW drive.

Patch looks fine for me.

Bartlomiej