bug in use-ide_pio_bytes patch?

bug in use-ide_pio_bytes patch?

[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);
	 }

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.

-- 
tejun

Re: bug in use-ide_pio_bytes patch?

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday 23 March 2009, Tejun Heo wrote:
> 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?

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

Thanks,
Bart

Re: bug in use-ide_pio_bytes patch?

[Tejun Heo]

Hi,

Bartlomiej Zolnierkiewicz wrote:
>> 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.

-- 
tejun

Re: bug in use-ide_pio_bytes patch?

[Bartlomiej Zolnierkiewicz]

On Monday 23 March 2009, Tejun Heo wrote:
> Hi,
> 
> Bartlomiej Zolnierkiewicz wrote:
> >> 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 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

Re: bug in use-ide_pio_bytes patch?

[Borislav Petkov]

On Mon, Mar 23, 2009 at 12:43:50PM +0100, Bartlomiej Zolnierkiewicz wrote:

> 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 <petkovbb@gmail.com>
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 <tj@kernel.org>
Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-atapi.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 11a680c..6d2f97f 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -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;
 	}
 
-- 
1.6.2.1

-- 
Regards/Gruss,
    Boris.

Re: bug in use-ide_pio_bytes patch?

[Bartlomiej Zolnierkiewicz]

On Wednesday 25 March 2009, Borislav Petkov wrote:
> On Mon, Mar 23, 2009 at 12:43:50PM +0100, Bartlomiej Zolnierkiewicz wrote:
> 
> > 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 <petkovbb@gmail.com>
> 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 <tj@kernel.org>
> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>

applied, thanks!