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

PATCH Andy Polyakov 2 participants 11 messages May 28, 2003 – Jun 5, 2003

Share & tools

Andy Polyakov

author

2003-05-28 07:27 UTC #0 lore raw

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 renamed from drivers/ide/ide-cd.c.orig +9 -3

@@ -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))

Jens Axboe

2003-05-28 07:35 UTC #1 lore raw

Quoted 6 lines · 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:-)

;-))

Quoted 9 lines

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.

Quoted 4 lines

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.

Quoted 8 lines

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?

Quoted 6 lines

Cheers. A. 8<--------8<--------8<--------8<--------8<--------8<--------8<--------

drivers/ide/ide-cd.c line 674

Quoted 9 lines

drivers/ide/ide-cd.c renamed from drivers/ide/ide-cd.c.orig +2 -1

@@ -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.

drivers/ide/ide-cd.c line 730

Quoted 9 lines

drivers/ide/ide-cd.c renamed from drivers/ide/ide-cd.c.orig +1 -1

@@ -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.

Quoted 4 lines

drivers/ide/ide-cd.c renamed from drivers/ide/ide-cd.c.orig +0 -0

@@ -1471,8 +1472,13 @@
 		/* Keep count of how much data we've moved. */
 		rq->data += thislen;
 		rq->data_len -= thislen;

drivers/ide/ide-cd.c line 1484

Quoted 10 lines

drivers/ide/ide-cd.c renamed from drivers/ide/ide-cd.c.orig +5 -0

+#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?

drivers/ide/ide-cd.c line 1621

Quoted 9 lines

drivers/ide/ide-cd.c renamed from drivers/ide/ide-cd.c.orig +1 -1

@@ -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.

Signature · 1 line

Jens Axboe

Jens Axboe

2003-05-28 07:39 UTC #2 lore raw

Quoted 11 lines · Jens Axboe

drivers/ide/ide-cd.c line 1621
Quoted 9 lines
drivers/ide/ide-cd.c renamed from drivers/ide/ide-cd.c.orig +1 -1
@@ -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;

Signature · 1 line

Jens Axboe

Andy Polyakov

author

2003-05-28 16:29 UTC #3 lore raw

In reply to Jens Axboe

Quoted 5 lines

Quoted 5 lines
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.

Quoted 1 line

Quoted 1 line
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...

Quoted 8 lines

drivers/ide/ide-cd.c renamed from drivers/ide/ide-cd.c.orig +2 -0

@@ -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

Quoted 8 lines

drivers/ide/ide-cd.c line 1484
Quoted 6 lines
drivers/ide/ide-cd.c renamed from drivers/ide/ide-cd.c.orig +3 -0
+             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...

Quoted 9 lines

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.

Jens Axboe

2003-05-28 16:50 UTC #4 lore raw

Quoted 10 lines · Andy Polyakov

Quoted 5 lines
Quoted 5 lines
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!

Quoted 6 lines

Quoted 1 line
Quoted 1 line
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.

Quoted 3 lines

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.

Quoted 3 lines

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...

Quoted 17 lines

Quoted 8 lines

(quoted diff) +2 -0

@@ -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

Quoted 8 lines

Review line 1484

Quoted 6 lines

(quoted diff) +3 -0

+             if (rq->flags & REQ_SENSE)
+                     rq->sense_len+=thislen;
+#endif
      } else {
 confused:
              printk ("%s: cdrom_pc_intr: The drive "

Hmm confused, care to expand?

Quoted 12 lines

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.

Quoted 10 lines

Quoted 9 lines
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;

Quoted 7 lines

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.

Quoted 4 lines

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 =====

drivers/ide/ide-cd.c +10 -5

@@ -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);

Signature · 1 line

Jens Axboe

Andy Polyakov

author

2003-05-29 13:37 UTC #5 lore raw

Quoted 10 lines

Quoted 4 lines
... 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.

Quoted 6 lines

Quoted 3 lines
... 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.

Jens Axboe

2003-05-29 17:20 UTC #6 lore raw

Quoted 17 lines · Andy Polyakov

Quoted 10 lines
Quoted 4 lines
... 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.
Quoted 6 lines
Quoted 3 lines
... 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.

Quoted 7 lines

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.

Signature · 1 line

Jens Axboe

Jens Axboe

2003-05-28 07:36 UTC #7 lore raw

In reply to Andy Polyakov

Quoted 4 lines · 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.

Looks straight forward, applied.

Signature · 1 line

Jens Axboe

Andy Polyakov

author

2003-06-04 19:29 UTC #8 lore raw

In reply to Andy Polyakov

Quoted 2 lines

... 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 Wed Jun 4 16:14:41 2003 renamed from drivers/ide/ide-cd.c.orig Tue Jun 3 12:21:56 2003 +3 -0

@@ -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

Jens Axboe

2003-06-04 19:42 UTC #9 lore raw

Quoted 22 lines · Andy Polyakov

Quoted 2 lines
... 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 Wed Jun 4 16:14:41 2003 line 665

Quoted 9 lines

drivers/ide/ide-cd.c Wed Jun 4 16:14:41 2003 renamed from drivers/ide/ide-cd.c.orig Tue Jun 3 12:21:56 2003 +3 -0

@@ -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.

Signature · 1 line

Jens Axboe

Andy Polyakov

author

2003-06-05 20:52 UTC #10 lore raw

Quoted 8 lines

Quoted 7 lines
Quoted 2 lines
... 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.

Quoted 4 lines

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 renamed from drivers/ide/ide-cd.c.orig +15 -6

@@ -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;