Skip to contentSkip to conversation

bug in use-ide_pio_bytes patch?

DISCUSSION Applied Tejun Heo 3 participants 6 messages Mar 23, 2009 – Mar 26, 2009 libata

lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

Hello, Bartlomiej.

I've been looking at the pata-2.6 patches and use-ide_pio_bytes patch contains the following snippet.

void ide_init_pc(struct ide_atapi_pc *pc)

  {
	 memset(pc, 0, sizeof(*pc));
 @@ -351,6 +308,9 @@ static ide_startstop_t ide_pc_intr(ide_d
			 pc->xferred = pc->req_xfer;
			 if (drive->pc_update_buffers)
				 drive->pc_update_buffers(drive, pc);
 +
 +			if (drive->media == ide_floppy)
 +				ide_complete_rq(drive, 0, blk_rq_bytes(rq));
		 }
		 debug_log("%s: DMA finished\n", drive->name);
	 }

It completes ide_floppy requests in the DMA handling block; however, those requests are gonna be completed again later in the regular completion path again. I think the above part can be simply dropped?

Thanks.

Signature · 1 line
tejun
#1 lore raw

Hi,

Quoted 19 lines · Tejun Heo

Hello, Bartlomiej.

I've been looking at the pata-2.6 patches and use-ide_pio_bytes patch contains the following snippet.

void ide_init_pc(struct ide_atapi_pc *pc)

  {
	 memset(pc, 0, sizeof(*pc));
 @@ -351,6 +308,9 @@ static ide_startstop_t ide_pc_intr(ide_d
			 pc->xferred = pc->req_xfer;
			 if (drive->pc_update_buffers)
				 drive->pc_update_buffers(drive, pc);
 +
 +			if (drive->media == ide_floppy)
 +				ide_complete_rq(drive, 0, blk_rq_bytes(rq));
		 }
		 debug_log("%s: DMA finished\n", drive->name);
	 }
Quoted 3 lines

It completes ide_floppy requests in the DMA handling block; however, those requests are gonna be completed again later in the regular completion path again. I think the above part can be simply dropped?

I'm not sure I get it -- could you point me to the other completion path?

Thanks,
Bart

author
#2 lore raw

Hi,

Quoted 5 lines · Bartlomiej Zolnierkiewicz
Quoted 3 lines

It completes ide_floppy requests in the DMA handling block; however, those requests are gonna be completed again later in the regular completion path again. I think the above part can be simply dropped?

I'm not sure I get it -- could you point me to the other completion path?

static ide_startstop_t ide_pc_intr(ide_drive_t *drive) {
....

	if (pc->flags & PC_FLAG_DMA_IN_PROGRESS) {
....
		if (rc || (drive->media == ide_tape && (stat & ATA_ERR))) {
			if (drive->media == ide_floppy)
				printk(KERN_ERR "%s: DMA %s error\n",
					drive->name, rq_data_dir(pc->rq)
						     ? "write" : "read");
			pc->flags |= PC_FLAG_DMA_ERROR;
		} else {
			pc->xferred = pc->req_xfer;
			if (drive->pc_update_buffers)
				drive->pc_update_buffers(drive, pc);

			if (drive->media == ide_floppy)
1:				ide_complete_rq(drive, 0, blk_rq_bytes(rq));
		}
		debug_log("%s: DMA finished\n", drive->name);
	}

	/* No more interrupts */
	if ((stat & ATA_DRQ) == 0) {
...
		/* Command finished - Call the callback function */
		uptodate = drive->pc_callback(drive, dsc);

		if (uptodate == 0)
			drive->failed_pc = NULL;

		if (blk_special_request(rq)) {
			rq->errors = 0;
2:			ide_complete_rq(drive, 0, blk_rq_bytes(rq));
		} else {
			unsigned int done;

			if (blk_fs_request(rq) == 0 && uptodate <= 0) {
				if (rq->errors == 0)
					rq->errors = -EIO;
			}

			if (drive->media == ide_tape)
				done = ide_rq_bytes(rq); /* FIXME */
			else
				done = blk_rq_bytes(rq);
3:			ide_complete_rq(drive, uptodate ? 0 : -EIO, done);
		}

		return ide_stopped;
	}

...
}

#1 completely completes the request after which it shouldn't be accessed, but rq is accessed afterwards and #2 or #3 will be called on the rq again. Am I missing something?

Thanks.

Signature · 1 line
tejun
#3 lore raw
Quoted 61 lines · Tejun Heo

Hi,

Quoted 5 lines · Bartlomiej Zolnierkiewicz
Quoted 3 lines

It completes ide_floppy requests in the DMA handling block; however, those requests are gonna be completed again later in the regular completion path again. I think the above part can be simply dropped?

I'm not sure I get it -- could you point me to the other completion path?

static ide_startstop_t ide_pc_intr(ide_drive_t *drive) {
....

	if (pc->flags & PC_FLAG_DMA_IN_PROGRESS) {
....
		if (rc || (drive->media == ide_tape && (stat & ATA_ERR))) {
			if (drive->media == ide_floppy)
				printk(KERN_ERR "%s: DMA %s error\n",
					drive->name, rq_data_dir(pc->rq)
						     ? "write" : "read");
			pc->flags |= PC_FLAG_DMA_ERROR;
		} else {
			pc->xferred = pc->req_xfer;
			if (drive->pc_update_buffers)
				drive->pc_update_buffers(drive, pc);

			if (drive->media == ide_floppy)
1:				ide_complete_rq(drive, 0, blk_rq_bytes(rq));
		}
		debug_log("%s: DMA finished\n", drive->name);
	}

	/* No more interrupts */
	if ((stat & ATA_DRQ) == 0) {
...
		/* Command finished - Call the callback function */
		uptodate = drive->pc_callback(drive, dsc);

		if (uptodate == 0)
			drive->failed_pc = NULL;

		if (blk_special_request(rq)) {
			rq->errors = 0;
2:			ide_complete_rq(drive, 0, blk_rq_bytes(rq));
		} else {
			unsigned int done;

			if (blk_fs_request(rq) == 0 && uptodate <= 0) {
				if (rq->errors == 0)
					rq->errors = -EIO;
			}

			if (drive->media == ide_tape)
				done = ide_rq_bytes(rq); /* FIXME */
			else
				done = blk_rq_bytes(rq);
3:			ide_complete_rq(drive, uptodate ? 0 : -EIO, done);
		}

		return ide_stopped;
	}
Quoted 6 lines

...
}

#1 completely completes the request after which it shouldn't be accessed, but rq is accessed afterwards and #2 or #3 will be called on the rq again. Am I missing something?

Thanks for explanation.

[ IIRC "1:" was always there but in the form of partial completions loop
inside ->pc_update_buffers callback... ]

It seems like we really should remove "1:" but it would be best to retest ide-floppy with this change. Borislav, could you add this to your queue?

Thanks,
Bart

#4 lore raw
Quoted 2 lines · Bartlomiej Zolnierkiewicz

It seems like we really should remove "1:" but it would be best to retest ide-floppy with this change. Borislav, could you add this to your queue?

Done.

---
From: Borislav Petkov <[email protected]>
Date: Wed, 25 Mar 2009 10:10:18 +0100
Subject: [PATCH] ide-floppy: do not complete rq's prematurely

... and access them afterwards. Simplify rq completing code while at it.

Spotted-by Tejun Heo <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
---
 drivers/ide/ide-atapi.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)
drivers/ide/ide-atapi.c +6 -7
index 11a680c..6d2f97f 100644
@@ -308,16 +308,14 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
 			pc->xferred = pc->req_xfer;
 			if (drive->pc_update_buffers)
 				drive->pc_update_buffers(drive, pc);
-
-			if (drive->media == ide_floppy)
-				ide_complete_rq(drive, 0, blk_rq_bytes(rq));
 		}
 		debug_log("%s: DMA finished\n", drive->name);
 	}
 
 	/* No more interrupts */
 	if ((stat & ATA_DRQ) == 0) {
-		int uptodate;
+		int uptodate, error;
+		unsigned int done;
 
 		debug_log("Packet command completed, %d bytes transferred\n",
 			  pc->xferred);

@@ -364,9 +362,9 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
 
 		if (blk_special_request(rq)) {
 			rq->errors = 0;
-			ide_complete_rq(drive, 0, blk_rq_bytes(rq));
+			done = blk_rq_bytes(rq);
+			error = 0;
 		} else {
-			unsigned int done;
 
 			if (blk_fs_request(rq) == 0 && uptodate <= 0) {
 				if (rq->errors == 0)

@@ -378,9 +376,10 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
 			else
 				done = blk_rq_bytes(rq);
 
-			ide_complete_rq(drive, uptodate ? 0 : -EIO, done);
+			error = uptodate ? 0 : -EIO;
 		}
 
+		ide_complete_rq(drive, error, done);
 		return ide_stopped;
 	}
Signature · 5 lines
1.6.2.1

-- 
Regards/Gruss,
    Boris.
Applied
#5 lore raw
Quoted 12 lines · Borislav Petkov
Quoted 2 lines · Bartlomiej Zolnierkiewicz

It seems like we really should remove "1:" but it would be best to retest ide-floppy with this change. Borislav, could you add this to your queue?

Done.

---
From: Borislav Petkov <[email protected]>
Date: Wed, 25 Mar 2009 10:10:18 +0100
Subject: [PATCH] ide-floppy: do not complete rq's prematurely

Quoted 4 lines

... and access them afterwards. Simplify rq completing code while at it.

Spotted-by Tejun Heo <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>

applied, thanks!