[summary] Re: burncd 'blank' not terminating ?

[Luigi Rizzo]

On Wed, Dec 27, 2006 at 02:33:15AM +0100, Søren Schmidt wrote:
> Luigi Rizzo wrote:
> > summary: there was some discussion on how to
> > fix the problem, in 6.x, with "burncd -f /dev/acd0 -v blank" getting
> > stuck with this message
> >
> > 	blanking CD, please wait..
> >
> > This used to work on 4.x.
> >
> > 6.x changes in two places:
> >
> >  *  the ioctl handler, acd_get_progress() in /sys/dev/ata/atapi-cd.c
> >     does a much stricter error checking, expecting the ATA_SENSE_VALID
> >     bit set in response to the ATAPI_READ_CAPACITY call.
> >     None of my DVD drives do that:
> >
> > 	acd0: DVDR <HL-DT-ST DVDRAM GSA-4163B/A101> at ata1-master UDMA33
> > 	acd0: DVDR <HL-DT-ST DVDRAM GSA-H10N/JL10> at ata0-slave UDMA33
> >
> >     even though they do report a valid progress status if you disable
> >     the check for ATA_SENSE_VALID, and in fact they do work under 4.x
> >
> >  *  usr.sbin/burncd/burncd.c waits for a transition of CDRIOCGETPROGRESS
> >     from 90..99 to 0, which fails to be detected because the ioctl()
> >     always returns 0 when ATA_SENSE_VALID is not set.
> >
> > In private mail, Soren mentioned that the spec (whatever it is called)
> > says that the ATA_SENSE_VALID 'shall be set' when returning a valid value,
> > so he is slightly dubious on removing the check in atapi-cd.c
> >
> > Also, it seems that the proper way to check for completion is to issue
> > the TEST UNIT READY command, which is accessible through the CDIOCRESET
> > ioct.
> >
> > I suggest the following two fixes:
> > 1. change burncd.c as below, so that if CDRIOCGETPROGRESS does not return
> >    anything good, it calls CDIOCRESET to determine when the command
> >    is complete.
> >    This can be improved by calling CDIOCRESET unconditionally as a
> >    termination test
> >   
> FWIW just checking with TEST UNIT READY in general for progress will 
> fail for lots of devices 

really ? i thought it was the standard way to test for completion
(i don't have access to a recent atapi-X spec other than
this SFF-8020i r2.6 - circa 1996).
But i see you have put a workaround in act_fixate(), presumably
for a similar behaviour.

> progress reporting so the sum of doing both is of benefit.
> > 2. change atapi-cd.c to return something even if ATA_SENSE_VALID is
> >    unset. Apparently there is a lot of non-complying hardware around
> >    so restricting to what the spec says is like shooting ourselves
> >    in the foot.
> >   
> As I mentioned lots of devices does return garbage there so its more a 
> question on which pool of devices you want to have working and which you 
> wont, so its not a solution, rather a decision on which devices to 
> support properly, I chose those that follow specs.

a few comments:
1. we don't need to limit ourselves to support only one type of devices;
   and given the large set of non-compliant devices (basically all the
   ones i have!) at the very least we should have a user-settable
   sysctl (to ignore the ATA_SENSE_VALID bit) or a burncd option
   (to trust TEST UNIT READY). I'd rather not use quirks because
   the list might become huge.

2. these same non-compliant devices used to work on 4.x, so people will
   really perceive this as a regression, which is not good from a PR
   perspective;

3. again i have an old copy of the spec (SFF-8020i), but the description
   of the ATA_SENSE_VALID is ambiguous - it says
   (Table 137-Request Sense Standard Data)

	"A Valid bit of zero indicates that the information field is
	not as defined in this specification. A Valid bit of one
	indicates the information field contains valid information as
	defined in this Specification. ATAPI CD-ROM drives shall
	implement the Valid bit"

   So, it doesn't say the bit _must_ be '1', and i can imagine implementors
   that slightly deviate from the standard formatting (e.g. adding fields
   etc. ) setting the bit to 0. Yet, we could at least pass the info
   up to userland (including the VALID bit) and let the application
   decide what to do with it.

> >       Again, if burncd.c uses CDIOCRESET to determine the completion
> >    of the 'blank' operation, we don't care if the return value from
> >    CDRIOCGETPROGRESS is incorrect, because we don't rely on it.
> >   
> Right, but that will leave out reporting of progress which has been 
> asked for lots of times since some of this takes time.

the way i have it now, i do both CDRIOCGETPROGRESS and CDIOCRESET,
using either one as a termination signal.

	cheers
	luigi