kern/122670: [ PATCH ] broken acd_get_progress = ioctl CDRIOCGETPROGRESS logic

[Dan Lukes]

>Number:         122670
>Category:       kern
>Synopsis:       [ PATCH ] broken acd_get_progress = ioctl CDRIOCGETPROGRESS logic
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Fri Apr 11 19:50:01 UTC 2008
>Closed-Date:
>Last-Modified:
>Originator:     Dan Lukes
>Release:        FreeBSD 6.3-STABLE i386
>Organization:
Obludarium
>Environment:
System: FreeBSD 6.3-STABLE i386: Tue Apr 8 02:07:56 CEST 2008
sys/dev/ata/atapi-cd.c,v 1.179.2.9 2007/11/22 07:50:36 sos Exp

BUT the same code is in RELENG-7 and HEAD, so it apply for it as well

>Description:

	Some CD / DVD operation take long time to complete
- for example BLANK or FORMAT. The application program call the apropriate
ioctl, then wait for the completion. The only avaiable function that can be
used for test of the completioness is ioctl CDRIOCGETPROGRESS
It is implemented in kernel's acd_get_progress function. 
Unfortunately, current logic of such function disallow the application
program to detect the command completion reliably.
	
 --------------------

Current logic description:

        acd_get_progress uses "READ CAPACITY" command. If the previous operation
is in progress, then READ CAPACITY is rejected. The acd_complete then will autoissue 
the REQUEST SENSE command to obtain sense data.

        If the returned sense key is 0 (NO SENSE) or 2 (NOT READY) then sense 
specific data will contain the operation progress data.

MINOR PROBLEM

        The code doesn't check the sense key value, despite the fact that other
values than 0|2 are possible and sense specific data has other meaning
in that case. The random data may be reported to caller as valid progress 
responses in such cases.

        The value of sense key needs to be checked before sense specific data are
interpreted as progress indicator value.

MAJOR PROBLEM

        If the command has been completed already, then READ CAPACITY didn't
fail and REQUEST SENSE has not been called at all.

        Unfortunately, the acd_get_progress code doesn't check this specific 
situation and return 0% as valid progress value. 

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

	It's hard for application program to use that value as sign of completion. The
burncd, the only base system program for blanking/formatting CD, wait for either 100%
or for >90% folowed by 0%. It's nice try, but it doesn't work for several CD/DVD 
writers. Some of them are so fast, so burncd miss all >=90% steps. Some of them use
too wide progress steps, so no >90% step issued.

	The burncd may wait for completion forever.

	It's not burncd fault - it do the best it can. Then only way to allow 
applications to use acd_get_progress reliably is to modify it's logic.


>How-To-Repeat:

	Try to blank or format (using ioctl CDRIOCBLANK or CDRIOCFORMAT, you may use
'burncd' for it) on PLEXTOR PX-716A or LITE-ON SHM-165H6S or several others. 
The operation will sucesfully completed, but burncd never ends waiting forever 
for command completion.


>Fix:

	All problems disapear with simple change in acd_get_progress logic - 
it may continue to return 0% for errors with one exception - the error 
"no command in progress" needs to be reported as 100% (in the fact, previous command
has been completed, so it's progress is 100%).

	The following fix handle both problems:

1. it doesn't return random progress values when device return sense
	code that contain sense specific code, but other
	than progress data

2. it return 100% when no command in progress instead of 0%


--- sys/dev/ata/atapi-cd.c.ORIG	2008-04-08 01:16:23.000000000 +0200
+++ sys/dev/ata/atapi-cd.c	2008-04-08 02:16:57.000000000 +0200
@@ -1219,6 +1219,7 @@
 		       0, 0, 0, 0, 0, 0, 0, 0 };
     struct ata_request *request;
     int8_t dummy[8];
+    int cc=0;
 
     if (!(request = ata_alloc_request()))
 	return ENOMEM;
@@ -1231,14 +1232,32 @@
     request->flags = ATA_R_ATAPI | ATA_R_READ;
     request->timeout = 30;
     ata_queue_request(request);
-    if (!request->error &&
-	request->u.atapi.sense.specific & ATA_SENSE_SPEC_VALID)
-	*finished = ((request->u.atapi.sense.specific2 |
-		     (request->u.atapi.sense.specific1 << 8)) * 100) / 65535;
-    else
-	*finished = 0;
+    
+    cc = ENXIO; /* in the case of error or improper sense key (so progress data are not avaiable) */
+    *finished = 0;
+    
+    if (!request->error) {
+        if (request->u.atapi.sense.key == 0 ) { 
+            /* possible only when REQUEST SENSE has not been called
+            * e.g. no command in progress / all commands completed) 
+            */
+            *finished = 100;
+            cc = 0;
+        } else if (request->u.atapi.sense.key == ATA_SENSE_NO_SENSE || request->u.atapi.sense.key == ATA_SENSE_NOT_READY) {
+            if (request->u.atapi.sense.specific & ATA_SENSE_SPEC_VALID ) {
+                /* we have valid progress data */
+                *finished = ((request->u.atapi.sense.specific2 |
+	            (request->u.atapi.sense.specific1 << 8)) * 100) / 65535;
+            } else {
+                /* operation in progres, but progress data not avaiable */
+                *finished = 0;
+            }
+            cc = 0;
+        }
+    }
+
     ata_free_request(request);
-    return 0;
+    return cc;
 }
 
 static int
--- patch ends here ---

>Release-Note:
>Audit-Trail:
>Unformatted: