From owner-freebsd-bugs@FreeBSD.ORG Fri Apr 11 19:50:02 2008 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 59EA81065671 for ; Fri, 11 Apr 2008 19:50:02 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 2D5638FC23 for ; Fri, 11 Apr 2008 19:50:02 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.14.2/8.14.2) with ESMTP id m3BJo2Wr081019 for ; Fri, 11 Apr 2008 19:50:02 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.2/8.14.1/Submit) id m3BJo2QY081018; Fri, 11 Apr 2008 19:50:02 GMT (envelope-from gnats) Resent-Date: Fri, 11 Apr 2008 19:50:02 GMT Resent-Message-Id: <200804111950.m3BJo2QY081018@freefall.freebsd.org> Resent-From: FreeBSD-gnats-submit@FreeBSD.org (GNATS Filer) Resent-To: freebsd-bugs@FreeBSD.org Resent-Reply-To: FreeBSD-gnats-submit@FreeBSD.org, Dan Lukes Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 87B921065670 for ; Fri, 11 Apr 2008 19:41:34 +0000 (UTC) (envelope-from dan@kulesh.obluda.cz) Received: from kulesh.obluda.cz (kulesh.obluda.cz [193.179.199.51]) by mx1.freebsd.org (Postfix) with ESMTP id E19DC8FC20 for ; Fri, 11 Apr 2008 19:41:33 +0000 (UTC) (envelope-from dan@kulesh.obluda.cz) Received: from kulesh.obluda.cz (localhost. [127.0.0.1]) by kulesh.obluda.cz (8.14.2/8.14.2) with ESMTP id m3BJURnB016897 for ; Fri, 11 Apr 2008 21:30:27 +0200 (CEST) (envelope-from dan@kulesh.obluda.cz) Received: (from root@localhost) by kulesh.obluda.cz (8.14.2/8.14.2/Submit) id m3BJUQGh016896; Fri, 11 Apr 2008 21:30:26 +0200 (CEST) (envelope-from dan) Message-Id: <200804111930.m3BJUQGh016896@kulesh.obluda.cz> Date: Fri, 11 Apr 2008 21:30:26 +0200 (CEST) From: Dan Lukes To: FreeBSD-gnats-submit@FreeBSD.org X-Send-Pr-Version: 3.113 Cc: Subject: kern/122670: [ PATCH ] broken acd_get_progress = ioctl CDRIOCGETPROGRESS logic X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Dan Lukes List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Apr 2008 19:50:02 -0000 >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: