2.5.69-70 ide-cd to guarantee fault-free CD/DVD burning experience?

2.5.69-70 ide-cd to guarantee fault-free CD/DVD burning experience?

[Andy Polyakov]

Linux is setting new landmarks all the time. Linux 2.5 is taking CD/DVD
burning to whole new level by *guaranteeing* fault-free burning
experience. No more hassle with overburns, underruns, poorly supported
media, positioning errors, power calibration failures, you name it...
It just works [by keeping the user-land totally unaware of errors
conditions raised by the logical unit]. Welcome to the future:-)

Well, I'm probably pushing this joke too far:-) In such case accept the
apologies along with this patch which makes it possible to access the
sense data returned by IDE CD/DVD units from user-land with SG_IO ioctl.
As for the last part, req->data?req->data:req->buffer. I'm not sure if
it's "the right thing(tm)" to do, but an error condition (dereferencing
of NULL pointer to be specific) is raised otherwise, whenever call to
bio_map_user in drivers/block/scsi_ioctl.c fails. The patch is
applicable at least to 2.5.69 and 2.5.70.

As for 69-ac. SG_IO doesn't work there at all (kernel logs "confused,
missing data" and then "N residual after xfer"). As far as I can tell
drivers/block/scsi_ioctl.c needs a "face lift," but that's AC's(?)
concern.

As for scsi_ioctl.c in more general sense. It apparently doesn't comply
with SG HOWTO, in particular it mis-interprets time-out values.
Background information and patch is available at
http://fy.chalmers.se/~appro/linux/DVD+RW/scsi_ioctl-2.5.69.patch.
There're couple of other issues, usage of 'bytes' variable in access_ok
and DMA being off when bio_map_user fails, that needs some further
discussion in my opinion. As for discussion. Please note that I'm not
on the linux-kernel list so that keep me on Cc:.

Cheers. A.
8<--------8<--------8<--------8<--------8<--------8<--------8<--------
--- ./drivers/ide/ide-cd.c.orig	Mon May  5 01:53:14 2003
+++ ./drivers/ide/ide-cd.c	Mon May 26 17:06:09 2003
@@ -667,7 +667,8 @@
 		void *sense = &info->sense_data;
 		
 		if (failed && failed->sense)
-			sense = failed->sense;
+			sense = failed->sense,
+			failed->sense_len=rq->sense_len;
 
 		cdrom_analyze_sense_data(drive, failed, sense);
 	}
@@ -723,7 +724,7 @@
 		 * scsi status byte
 		 */
 		if ((rq->flags & REQ_BLOCK_PC) && !rq->errors)
-			rq->errors = CHECK_CONDITION;
+			rq->errors = CHECK_CONDITION<<1;
 
 		/* Check for tray open. */
 		if (sense_key == NOT_READY) {
@@ -1471,8 +1472,13 @@
 		/* Keep count of how much data we've moved. */
 		rq->data += thislen;
 		rq->data_len -= thislen;
+#if 0
 		if (rq->cmd[0] == GPCMD_REQUEST_SENSE)
 			rq->sense_len++;
+#else
+		if (rq->flags & REQ_SENSE)
+			rq->sense_len+=thislen;
+#endif
 	} else {
 confused:
 		printk ("%s: cdrom_pc_intr: The drive "
@@ -1609,7 +1615,7 @@
 
 static void post_transform_command(struct request *req)
 {
-	char *ibuf = req->buffer;
+	char *ibuf = req->data?req->data:req->buffer;
 	u8 *c = req->cmd;
 
 	if (!blk_pc_request(req))

Re: 2.5.69-70 ide-cd to guarantee fault-free CD/DVD burning experience?

[Jens Axboe]

On Wed, May 28 2003, Andy Polyakov wrote:
> Linux is setting new landmarks all the time. Linux 2.5 is taking CD/DVD
> burning to whole new level by *guaranteeing* fault-free burning
> experience. No more hassle with overburns, underruns, poorly supported
> media, positioning errors, power calibration failures, you name it...
> It just works [by keeping the user-land totally unaware of errors
> conditions raised by the logical unit]. Welcome to the future:-)

;-))

> Well, I'm probably pushing this joke too far:-) In such case accept the
> apologies along with this patch which makes it possible to access the
> sense data returned by IDE CD/DVD units from user-land with SG_IO ioctl.
> As for the last part, req->data?req->data:req->buffer. I'm not sure if
> it's "the right thing(tm)" to do, but an error condition (dereferencing
> of NULL pointer to be specific) is raised otherwise, whenever call to
> bio_map_user in drivers/block/scsi_ioctl.c fails. The patch is
> applicable at least to 2.5.69 and 2.5.70.
> 
> As for 69-ac. SG_IO doesn't work there at all (kernel logs "confused,
> missing data" and then "N residual after xfer"). As far as I can tell
> drivers/block/scsi_ioctl.c needs a "face lift," but that's AC's(?)
> concern.

69-ac doesn't have the data direction fix yet probably, after the rq-dyn
alloc patch.

> As for scsi_ioctl.c in more general sense. It apparently doesn't comply
> with SG HOWTO, in particular it mis-interprets time-out values.
> Background information and patch is available at
> http://fy.chalmers.se/~appro/linux/DVD+RW/scsi_ioctl-2.5.69.patch.
> There're couple of other issues, usage of 'bytes' variable in access_ok
> and DMA being off when bio_map_user fails, that needs some further
> discussion in my opinion. As for discussion. Please note that I'm not
> on the linux-kernel list so that keep me on Cc:.

Well care to expand on these?

> 
> Cheers. A.
> 8<--------8<--------8<--------8<--------8<--------8<--------8<--------
> --- ./drivers/ide/ide-cd.c.orig	Mon May  5 01:53:14 2003
> +++ ./drivers/ide/ide-cd.c	Mon May 26 17:06:09 2003
> @@ -667,7 +667,8 @@
>  		void *sense = &info->sense_data;
>  		
>  		if (failed && failed->sense)
> -			sense = failed->sense;
> +			sense = failed->sense,
> +			failed->sense_len=rq->sense_len;
>  
>  		cdrom_analyze_sense_data(drive, failed, sense);
>  	}

Looks good, applied.

> @@ -723,7 +724,7 @@
>  		 * scsi status byte
>  		 */
>  		if ((rq->flags & REQ_BLOCK_PC) && !rq->errors)
> -			rq->errors = CHECK_CONDITION;
> +			rq->errors = CHECK_CONDITION<<1;
>  
>  		/* Check for tray open. */
>  		if (sense_key == NOT_READY) {

Ditto, but lets use SAM_CHECK_CONDITION instead, I hate this bit
shifting crap.

> @@ -1471,8 +1472,13 @@
>  		/* Keep count of how much data we've moved. */
>  		rq->data += thislen;
>  		rq->data_len -= thislen;
> +#if 0
>  		if (rq->cmd[0] == GPCMD_REQUEST_SENSE)
>  			rq->sense_len++;
> +#else
> +		if (rq->flags & REQ_SENSE)
> +			rq->sense_len+=thislen;
> +#endif
>  	} else {
>  confused:
>  		printk ("%s: cdrom_pc_intr: The drive "

Hmm confused, care to expand?

> @@ -1609,7 +1615,7 @@
>  
>  static void post_transform_command(struct request *req)
>  {
> -	char *ibuf = req->buffer;
> +	char *ibuf = req->data?req->data:req->buffer;
>  	u8 *c = req->cmd;
>  
>  	if (!blk_pc_request(req))

That is bad, though. I've changed this to just bail on !ibuf.

-- 
Jens Axboe

Re: 2.5.69-70 ide-cd to guarantee fault-free CD/DVD burning experience?

[Jens Axboe]

On Wed, May 28 2003, Andy Polyakov wrote:
> As for scsi_ioctl.c in more general sense. It apparently doesn't comply
> with SG HOWTO, in particular it mis-interprets time-out values.
> Background information and patch is available at
> http://fy.chalmers.se/~appro/linux/DVD+RW/scsi_ioctl-2.5.69.patch.

Looks straight forward, applied.

-- 
Jens Axboe

Re: 2.5.69-70 ide-cd to guarantee fault-free CD/DVD burning experience?

[Jens Axboe]

On Wed, May 28 2003, Jens Axboe wrote:
> > @@ -1609,7 +1615,7 @@
> >  
> >  static void post_transform_command(struct request *req)
> >  {
> > -	char *ibuf = req->buffer;
> > +	char *ibuf = req->data?req->data:req->buffer;
> >  	u8 *c = req->cmd;
> >  
> >  	if (!blk_pc_request(req))
> 
> That is bad, though. I've changed this to just bail on !ibuf.

Sorry I misread that, ->data is the one we want. I'm wondering how this
got mixed up... So to clarify:

	char *ibuf = req->data;

	if (!blk_pc_request(req))
		return;
	if (!ibuf)
		return;

-- 
Jens Axboe

Re: 2.5.69-70 ide-cd to guarantee fault-free CD/DVD burning experience?

[Andy Polyakov]

> > As for scsi_ioctl.c in more general sense. It apparently doesn't comply
> > with SG HOWTO, in particular it mis-interprets time-out values.
> > Background information and patch is available at
> > http://fy.chalmers.se/~appro/linux/DVD+RW/scsi_ioctl-2.5.69.patch.
> > There're couple of other issues, usage of 'bytes' variable in access_ok

	bytes = (hdr.dxfer_len + 511) & ~511;
	... access_ok(VERIFY_READ, uaddr, bytes)...
                                          ^^^^^ Shouldn't this be
hdr.dxfer_len? At least that's what memcpy-ed to kalloc-ated buffer.

> > and DMA being off when bio_map_user fails,

While tracing problems down I've commented out call to bio_map_user. But
of course it could have failed for more legitimate reasons, couldn't it?
Reasons such as user buffer residing in non DMA-capable region(?) or
being misaligned. But in either case I noticed that DMA is never engaged
on that buffer allocated with kmalloc. The question is if it's
intentional? If answer is yes, then the case is dismissed. If not, then
it should be looked into. Now I don't know if it's apporpriate to
complement GPF_USER with GFP_DMA, but it might be appropriate to retry
bio_map_user on buffer. I'm actually stepping out of my competence
domains here...

> > @@ -1471,8 +1472,13 @@
> >               /* Keep count of how much data we've moved. */
> >               rq->data += thislen;
> >               rq->data_len -= thislen;
> > +#if 0
> >               if (rq->cmd[0] == GPCMD_REQUEST_SENSE)
> >                       rq->sense_len++;
> > +#else
> > +             if (rq->flags & REQ_SENSE)
> > +                     rq->sense_len+=thislen;
> > +#endif
> >       } else {
> >  confused:
> >               printk ("%s: cdrom_pc_intr: The drive "
> 
> Hmm confused, care to expand?

rq->sense_len++ is obviously bogus as user-land will only get the first
byte of the sense data [so that you can tell apart deferred and
immediate errors, but you can't tell what was actually wrong]. As for
"if (rq->cmd[0] == GPCMD_REQUEST_SENSE)" vs. "if (rq->flags &
REQ_SENSE)." User-land is permitted to issue REQUEST SENSE on it's own
behalf, isn't it? With "rq->cmd[0] == GPCMD_REQUEST_SENSE" kernel will
provide user-land with sense buffer with bogus data (even if it's
zeros:-). "rq->flags & REQ_SENSE" implies "rq->cmd[0] ==
GPCMD_REQUEST_SENSE" as it happens only when kernel itself pulls the
sense data on behalf of failed command and that's exactly what should be
returned to user. Or is it #if 0/#else/#endif which is confusing? Well,
we don't have to keep that, it's just left-overs from my working copy...

> Sorry I misread that, ->data is the one we want. I'm wondering how this
> got mixed up... So to clarify:
> 
>         char *ibuf = req->data;
> 
>         if (!blk_pc_request(req))
>                 return;
>         if (!ibuf)
>                 return;

But req->data is assigned NULL every time bio_map_user succeeds! Just
follow it in sg_io():

	buffer=NULL; ...
	bio=bio_map_user(...);
	if (!bio) buffer=kmalloc(...);
	rq->data=buffer;

So that if(!req->data) is true most of the time [as bio_map_user
succeeds most of the time]... As for req->buffer. Given that only first
4 bytes/32 bits are manipulated it's actually safe to dereference it
directly, isn't it? A.

Re: 2.5.69-70 ide-cd to guarantee fault-free CD/DVD burning experience?

[Jens Axboe]

On Wed, May 28 2003, Andy Polyakov wrote:
> > > As for scsi_ioctl.c in more general sense. It apparently doesn't comply
> > > with SG HOWTO, in particular it mis-interprets time-out values.
> > > Background information and patch is available at
> > > http://fy.chalmers.se/~appro/linux/DVD+RW/scsi_ioctl-2.5.69.patch.
> > > There're couple of other issues, usage of 'bytes' variable in access_ok
> 
> 	bytes = (hdr.dxfer_len + 511) & ~511;
> 	... access_ok(VERIFY_READ, uaddr, bytes)...
>                                           ^^^^^ Shouldn't this be
> hdr.dxfer_len? At least that's what memcpy-ed to kalloc-ated buffer.

Yes!

> > > and DMA being off when bio_map_user fails,
> 
> While tracing problems down I've commented out call to bio_map_user. But
> of course it could have failed for more legitimate reasons, couldn't it?
> Reasons such as user buffer residing in non DMA-capable region(?) or
> being misaligned. But in either case I noticed that DMA is never engaged

Correct.

> on that buffer allocated with kmalloc. The question is if it's
> intentional? If answer is yes, then the case is dismissed. If not, then
> it should be looked into. Now I don't know if it's apporpriate to

Depends on the lower level driver, for ide-cd yes kmalloc'ed data will
not be dma'ed to. We require a valid bio setup for that, usually the bio
mapping will fail exactly because the length/alignment isn't correct for
ide-cd.

sr will dma to the kmalloced buffer just fine.

> complement GPF_USER with GFP_DMA, but it might be appropriate to retry
> bio_map_user on buffer. I'm actually stepping out of my competence
> domains here...

It's usually not worth it. If the buffer is < 4 bytes, we don't dma. Big
deal. It's the programs responsibility to make sure that data + length
is appropriately aligned for dma operations akin to O_DIRECT for
instance. And they already do that, so...

> > > @@ -1471,8 +1472,13 @@
> > >               /* Keep count of how much data we've moved. */
> > >               rq->data += thislen;
> > >               rq->data_len -= thislen;
> > > +#if 0
> > >               if (rq->cmd[0] == GPCMD_REQUEST_SENSE)
> > >                       rq->sense_len++;
> > > +#else
> > > +             if (rq->flags & REQ_SENSE)
> > > +                     rq->sense_len+=thislen;
> > > +#endif
> > >       } else {
> > >  confused:
> > >               printk ("%s: cdrom_pc_intr: The drive "
> > 
> > Hmm confused, care to expand?
> 
> rq->sense_len++ is obviously bogus as user-land will only get the first
> byte of the sense data [so that you can tell apart deferred and
> immediate errors, but you can't tell what was actually wrong]. As for
> "if (rq->cmd[0] == GPCMD_REQUEST_SENSE)" vs. "if (rq->flags &
> REQ_SENSE)." User-land is permitted to issue REQUEST SENSE on it's own
> behalf, isn't it? With "rq->cmd[0] == GPCMD_REQUEST_SENSE" kernel will
> provide user-land with sense buffer with bogus data (even if it's
> zeros:-). "rq->flags & REQ_SENSE" implies "rq->cmd[0] ==
> GPCMD_REQUEST_SENSE" as it happens only when kernel itself pulls the
> sense data on behalf of failed command and that's exactly what should be
> returned to user. Or is it #if 0/#else/#endif which is confusing? Well,
> we don't have to keep that, it's just left-overs from my working copy...

Ah good point on the REQ_SENSE bit, completely agree. The if 0 thing
cannot go in obviously, I'll kill that along the way.

> > Sorry I misread that, ->data is the one we want. I'm wondering how this
> > got mixed up... So to clarify:
> > 
> >         char *ibuf = req->data;
> > 
> >         if (!blk_pc_request(req))
> >                 return;
> >         if (!ibuf)
> >                 return;
> 
> But req->data is assigned NULL every time bio_map_user succeeds! Just
> follow it in sg_io():
> 
> 	buffer=NULL; ...
> 	bio=bio_map_user(...);
> 	if (!bio) buffer=kmalloc(...);
> 	rq->data=buffer;

Hmm it looks pretty bogus actually, in most cases we have already
removed ->bio at this point.

> So that if(!req->data) is true most of the time [as bio_map_user
> succeeds most of the time]... As for req->buffer. Given that only first
> 4 bytes/32 bits are manipulated it's actually safe to dereference it
> directly, isn't it? A.

->buffer is not to be used in this context, so forget that. It's a relic
from when the pre-transform allocated extra data and we copied back and
freed in post-transform. That was killed, and I'm really wondering
whether we shouldn't just kill the post-transform completely too. For
reference, this is what is should look like...

===== drivers/ide/ide-cd.c 1.46 vs edited =====
--- 1.46/drivers/ide/ide-cd.c	Thu May  8 10:39:34 2003
+++ edited/drivers/ide/ide-cd.c	Wed May 28 19:02:10 2003
@@ -1609,12 +1609,19 @@
 
 static void post_transform_command(struct request *req)
 {
-	char *ibuf = req->buffer;
 	u8 *c = req->cmd;
+	char *ibuf;
 
 	if (!blk_pc_request(req))
 		return;
 
+	if (rq->data)
+		ibuf = rq->data;
+	else if (rq->bio)
+		ibuf = bio_data(rq->bio);
+	else
+		return;
+
 	/*
 	 * set ansi-revision and response data as atapi
 	 */
@@ -1664,8 +1671,8 @@
 		if (dma_error)
 			return DRIVER(drive)->error(drive, "dma error", stat);
 
+		post_transform_command(rq);
 		end_that_request_chunk(rq, 1, rq->data_len);
-		rq->data_len = 0;
 		goto end_request;
 	}
 
@@ -1687,6 +1694,7 @@
 	if ((stat & DRQ_STAT) == 0) {
 		if (rq->data_len)
 			printk("%s: %u residual after xfer\n", __FUNCTION__, rq->data_len);
+		post_transform_command(rq);
 		goto end_request;
 	}
 
@@ -1765,9 +1773,6 @@
 	return ide_started;
 
 end_request:
-	if (!rq->data_len)
-		post_transform_command(rq);
-
 	spin_lock_irqsave(&ide_lock, flags);
 	blkdev_dequeue_request(rq);
 	end_that_request_last(rq);

-- 
Jens Axboe

Re: 2.5.69-70 ide-cd to guarantee fault-free CD/DVD burning experience?

[Andy Polyakov]

> > ... I noticed that DMA is never engaged
> > on that buffer allocated with kmalloc. The question is if it's
> > intentional? If answer is yes, then the case is dismissed. If not, then
> > it should be looked into...
> 
> Depends on the lower level driver, for ide-cd yes kmalloc'ed data will
> not be dma'ed to. We require a valid bio setup for that, usually the bio
> mapping will fail exactly because the length/alignment isn't correct for
> ide-cd.
> 
> > ... it might be appropriate to retry
> > bio_map_user on buffer. I'm actually stepping out of my competence
> > domains here...
> 
> It's usually not worth it. If the buffer is < 4 bytes, we don't dma. Big
> deal.

Well, I'm concerned rather about cases when user buffer ends up in non
DMA-able memory than small or misaligned buffers. I mean those who have
system with loads of RAM didn't do anything wrong, yet they get
"punished." But I'm not actually insisting! Just saying that it *might*
be worth reconsidering "ide-cd won't do dma without bio setup" clause or
retry bio_map_user on kalloc-ed buffer. At least for transfers not
smaller than say 2K:-)

Cheers. A.

Re: 2.5.69-70 ide-cd to guarantee fault-free CD/DVD burning experience?

[Jens Axboe]

On Thu, May 29 2003, Andy Polyakov wrote:
> > > ... I noticed that DMA is never engaged
> > > on that buffer allocated with kmalloc. The question is if it's
> > > intentional? If answer is yes, then the case is dismissed. If not, then
> > > it should be looked into...
> > 
> > Depends on the lower level driver, for ide-cd yes kmalloc'ed data will
> > not be dma'ed to. We require a valid bio setup for that, usually the bio
> > mapping will fail exactly because the length/alignment isn't correct for
> > ide-cd.
> > 
> > > ... it might be appropriate to retry
> > > bio_map_user on buffer. I'm actually stepping out of my competence
> > > domains here...
> > 
> > It's usually not worth it. If the buffer is < 4 bytes, we don't dma. Big
> > deal.
> 
> Well, I'm concerned rather about cases when user buffer ends up in non
> DMA-able memory than small or misaligned buffers. I mean those who have
> system with loads of RAM didn't do anything wrong, yet they get
> "punished." But I'm not actually insisting! Just saying that it *might*
> be worth reconsidering "ide-cd won't do dma without bio setup" clause or
> retry bio_map_user on kalloc-ed buffer. At least for transfers not
> smaller than say 2K:-)

This is bogus. Applications already dish out the right buffers and in
the right lengths. It's the logical thing to do.

You are making up a problem that doesn't exist.

-- 
Jens Axboe

Re: 2.5.69-70 ide-cd to guarantee fault-free CD/DVD burning experience?

[Andy Polyakov]

> ... accept ... patch which makes it possible to access the
> sense data returned by IDE CD/DVD units from user-land with SG_IO ioctl.

The originally proposed modifications were indeed sufficient to get
DVD+RW units working, but apparently not DVD-RW ones:-( Note though
that [another] problem discussed here is not specific to DVD-RW
recordings. It's generic bug/deficiency. Once a packet commands is
terminated with an error condition the whole bio should be purged at
once and not only the first chunk as it's currently implemented.

Attached patch should be considered as a "denoting" patch, not
"final." Well, because it was verified with single application,
growisofs of dvd+rw-tools, which uses mmap-ed, in other words 
page-aligned, buffer(s). I mean I'm not 100% sure if hard_nr_sectors
is appropriate even for general case of 4-byte aligned buffers...
Then if-statement should probably be extended even to REQ_PC case...

Cheers. A.
8<--------8<--------8<--------8<--------8<--------8<--------8<--------
--- ./drivers/ide/ide-cd.c.orig Tue Jun  3 12:21:56 2003
+++ ./drivers/ide/ide-cd.c      Wed Jun  4 16:14:41 2003
@@ -657,6 +657,9 @@
        struct request *rq = HWGROUP(drive)->rq;
        int nsectors = rq->hard_cur_sectors;
 
+       if (rq->flags&REQ_BLOCK_PC)
+               nsectors = rq->hard_nr_sectors;	/* purge it all ... */
+       else
        if ((rq->flags & REQ_SENSE) && uptodate) {
                /*
                 * For REQ_SENSE, "rq->buffer" points to the original failed

Re: 2.5.69-70 ide-cd to guarantee fault-free CD/DVD burning experience?

[Jens Axboe]

On Wed, Jun 04 2003, Andy Polyakov wrote:
> > ... accept ... patch which makes it possible to access the
> > sense data returned by IDE CD/DVD units from user-land with SG_IO ioctl.
> 
> The originally proposed modifications were indeed sufficient to get
> DVD+RW units working, but apparently not DVD-RW ones:-( Note though
> that [another] problem discussed here is not specific to DVD-RW
> recordings. It's generic bug/deficiency. Once a packet commands is
> terminated with an error condition the whole bio should be purged at
> once and not only the first chunk as it's currently implemented.
> 
> Attached patch should be considered as a "denoting" patch, not
> "final." Well, because it was verified with single application,
> growisofs of dvd+rw-tools, which uses mmap-ed, in other words 
> page-aligned, buffer(s). I mean I'm not 100% sure if hard_nr_sectors
> is appropriate even for general case of 4-byte aligned buffers...
> Then if-statement should probably be extended even to REQ_PC case...
> 
> Cheers. A.
> 8<--------8<--------8<--------8<--------8<--------8<--------8<--------
> --- ./drivers/ide/ide-cd.c.orig Tue Jun  3 12:21:56 2003
> +++ ./drivers/ide/ide-cd.c      Wed Jun  4 16:14:41 2003
> @@ -657,6 +657,9 @@
>         struct request *rq = HWGROUP(drive)->rq;
>         int nsectors = rq->hard_cur_sectors;
>  
> +       if (rq->flags&REQ_BLOCK_PC)
> +               nsectors = rq->hard_nr_sectors;	/* purge it all ... */
> +       else
>         if ((rq->flags & REQ_SENSE) && uptodate) {
>                 /*
>                  * For REQ_SENSE, "rq->buffer" points to the original failed

The *sector* values don't really work well for REQ_BLOCK_PC, it doesn't
even have to be set at all. One solution would be to add more rq members
(yuck), a nicer one is probably to make cdrom_end_request return
ide_end_request ret value, and simply make the error locations kill the
requests... It's not very nice, but should work.

-- 
Jens Axboe

Re: 2.5.69-70 ide-cd to guarantee fault-free CD/DVD burning experience?

[Andy Polyakov]

> > > ... accept ... patch which makes it possible to access the
> > > sense data returned by IDE CD/DVD units from user-land with SG_IO ioctl.
> >
> > ... [another] problem discussed here is not specific to DVD-RW
> > recordings. It's generic bug/deficiency. Once a packet commands is
> > terminated with an error condition the whole bio should be purged at
> > once and not only the first chunk as it's currently implemented.
> 
> The *sector* values don't really work well for REQ_BLOCK_PC, ...
> ... a nicer one is probably to make cdrom_end_request return
> ide_end_request ret value, and simply make the error locations kill the
> requests... It's not very nice, but should work.

Well, if *sectors has no meaning in this context, then there is hadrly
a reason to call ide_end_request either... So I figured rq can be
terminated as below. As for postponing HWIF(drive)->ide_dma_off(drive).
Legitimately terminated commands is hardly a reason for switchinf DMA
off. Most notably DVD-RW units terminate WRITE requests every time
internal buffer gets exhausted, which happens all the time...

Cheers. A.
8<--------8<--------8<--------8<--------8<--------8<--------8<--------
--- ./drivers/ide/ide-cd.c.orig	Tue Jun  3 12:21:56 2003
+++ ./drivers/ide/ide-cd.c	Thu Jun  5 22:26:56 2003
@@ -749,12 +749,17 @@
 		   by transferring the semaphore from the packet
 		   command request to the request sense request. */
 
+		rq->flags |= REQ_FAILED;
 		if ((stat & ERR_STAT) != 0) {
 			wait = rq->waiting;
 			rq->waiting = NULL;
+			if (rq->flags&REQ_BLOCK_PC) {
+				cdrom_queue_request_sense(drive, wait,
+							  rq->sense, rq);
+				return 1; /* REQ_BLOCK_PC self-cares */
+			}
 		}
 
-		rq->flags |= REQ_FAILED;
 		cdrom_end_request(drive, 0);
 
 		if ((stat & ERR_STAT) != 0)
@@ -1657,13 +1662,14 @@
 	dma = info->dma;
 	if (dma) {
 		info->dma = 0;
-		if ((dma_error = HWIF(drive)->ide_dma_end(drive))) {
-			printk("ide-cd: dma error\n");
-			HWIF(drive)->ide_dma_off(drive);
-		}
+		dma_error = HWIF(drive)->ide_dma_end(drive);
 	}
 
 	if (cdrom_decode_status(drive, 0, &stat)) {
+		if (stat&ERR_STAT)
+		{	end_that_request_chunk(rq, 0, rq->data_len);
+			goto end_request; /* purge the whole thing... */
+		}
 		end_that_request_chunk(rq, 1, rq->data_len);
 		return ide_stopped;
 	}
@@ -1672,8 +1678,11 @@
 	 * using dma, transfer is complete now
 	 */
 	if (dma) {
-		if (dma_error)
+		if (dma_error) {
+			printk("ide-cd: dma error\n");
+			HWIF(drive)->ide_dma_off(drive);
 			return DRIVER(drive)->error(drive, "dma error", stat);
+		}
 
 		end_that_request_chunk(rq, 1, rq->data_len);
 		rq->data_len = 0;