Regarding drivers/ide/ide-cd.c in 2.5.72

PATCH Andy Polyakov 2 participants 6 messages Jun 17, 2003 – Jul 12, 2003

Share & tools

Andy Polyakov

author

2003-06-17 21:40 UTC #0 lore raw

Hi,

I have brought this issue once already in 2.5.70 context (see uppermost post in http://marc.theaimsgroup.com/?t=105410790500005&r=1&w=2), but it apparently slipped through. So I've decided to bring it up again, this time describing [hopefully] better how does this *generic* problem with ide-cd.c manifests itself. In the nutshell the problem is that [as it is now] every failed SG_IO request is replayed second time without data transfer. E.g. if WRITE(10) SCSI command fails with sense code X, ide-cd immediately resends the command block descriptor one more time, this time without programming for any associated I/O payload, which [normally] results in *different* sense code. And the catch is that user-land gets this second sense code instead of the real/original one. Suggested patch overcomes this problem by immediately purging the failed SG_IO request from the request queue.

P.S. Non-essential "rant." One can wonder how come the originally discussed patch, the one which made to 2.5.71, was sufficient for DVD+RW units, but not DVD-RW. Are commands sent to DVD-RW unit so to say bound to fail? In a sense yes! Majority of DVD-RW units terminate WRITE requests with "LONG WRITE IN PROGRESS" sense code wherever internal buffer gets full. The idea behind this LUN behaviour is to minimize IDE-bus monopolization periods. The sense code is commonly treated as "estimate how long does it take to purge say 1/2 of internal buffer, hibernate for that while and retry the failed WRITE command." Needless to say that if original sense code is effectively discarded and replaced by another code [apparently by "INVALID ADDRESS FOR WRITE"], it makes it impossible to treat this essentially *non-error* condition appropriately For whatever reason DVD+RW units don't terminate WRITE requests in same manner and IDE-bus remains monopilized till there is buffer space available to meet the outstanding WRITE request. Well, there're commands which get replayed even in DVD+RW case, those are data-less commands, most notably TEST UNIT READY, which are "safe" to replay.

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;

Jens Axboe

2003-06-19 12:09 UTC #1 lore raw

Quoted 15 lines · Andy Polyakov

Hi,
I have brought this issue once already in 2.5.70 context (see uppermost post in http://marc.theaimsgroup.com/?t=105410790500005&r=1&w=2), but it apparently slipped through. So I've decided to bring it up again, this time describing [hopefully] better how does this *generic* problem with ide-cd.c manifests itself. In the nutshell the problem is that [as it is now] every failed SG_IO request is replayed second time without data transfer. E.g. if WRITE(10) SCSI command fails with sense code X, ide-cd immediately resends the command block descriptor one more time, this time without programming for any associated I/O payload, which [normally] results in *different* sense code. And the catch is that user-land gets this second sense code instead of the real/original one. Suggested patch overcomes this problem by immediately purging the failed SG_IO request from the request queue.

Patch looks fine, care to resend actually trying to follow the style in the file in question?

Signature · 1 line

Jens Axboe

Andy Polyakov

author

2003-06-19 13:57 UTC #2 lore raw

Quoted 8 lines

Quoted 5 lines
... In the nutshell the problem is that [as it is
now] every failed SG_IO request is replayed second time without data transfer. ... Suggested patch
overcomes this problem by immediately purging the failed SG_IO request from the request queue.
Patch looks fine, care to resend actually trying to follow the style in the file in question?

Revised to my best ability for follow the coding style of file in question. 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) != 0) {
+				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) != 0) {
+			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;

Jens Axboe

2003-06-19 13:58 UTC #3 lore raw

Quoted 11 lines · Andy Polyakov

Quoted 8 lines
Quoted 5 lines
... In the nutshell the problem is that [as it is
now] every failed SG_IO request is replayed second time without data transfer. ... Suggested patch
overcomes this problem by immediately purging the failed SG_IO request from the request queue.
Patch looks fine, care to resend actually trying to follow the style in the file in question?
Revised to my best ability for follow the coding style of file in question. A:-)

Much better, thanks :)

Signature · 1 line

Jens Axboe

Andy Polyakov

author

2003-07-11 13:12 UTC #4 lore raw

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
... In the nutshell the problem is that [as it is
now] every failed SG_IO request is replayed second time without data transfer. ... Suggested patch
overcomes this problem by immediately purging the failed SG_IO request from the request queue.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Patch looks fine, care to resend actually trying to follow the style in the file in question?
Revised to my best ability for follow the coding style of file in question. A:-)
Much better, thanks :)

And it slipped through again:-( Sharpen up! BTW, the proposed patch fixes even http://marc.theaimsgroup.com/?t=105634805100001&r=1&w=2,
where induced kernel crash occurs upon packet replay. Applicable to up to 2.5.75. 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) != 0) {
+				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) != 0) {
+			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;

Jens Axboe

2003-07-12 11:37 UTC #5 lore raw

Quoted 9 lines · Andy Polyakov

Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines
... In the nutshell the problem is that [as it is
now] every failed SG_IO request is replayed second time without data transfer. ... Suggested patch
overcomes this problem by immediately purging the failed SG_IO request from the request queue.
Patch looks fine, care to resend actually trying to follow the style in the file in question?

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
Revised to my best ability for follow the coding style of file in question. A:-)
Much better, thanks :)
And it slipped through again:-( Sharpen up! BTW, the proposed patch

Calm down, I pass the fixes on and sometimes the just fall through the cracks (and not always just my cracks).

Quoted 3 lines

fixes even http://marc.theaimsgroup.com/?t=105634805100001&r=1&w=2,
where induced kernel crash occurs upon packet replay. Applicable to up to 2.5.75. A.

It looks reasonable, I'm wondering whether the ->ide_dma_off() isn't too harsh in the first place. But we can look at that later.

Signature · 1 line

Jens Axboe