[PATCH 0/3] some ide fixes

[PATCH 0/3] some ide fixes

[Borislav Petkov]

Hi Bart,

I've updated the two ide-floppy patches from you to current pata-2.6 and
added the partial completions fix to the second one so that we don't
break bisect builds (although this is hardly unlikely since nobody uses
an ide-floppy anymore). The 3rd patch is the ide-cd fix from linux-next
which has been tested by Tetsuo and me.

 drivers/ide/ide-atapi.c  |  136 +++++++++++++++-------------------------------
 drivers/ide/ide-cd.c     |    4 --
 drivers/ide/ide-floppy.c |   26 ++--------
 drivers/ide/ide-tape.c   |   32 ++++++------
 include/linux/ide.h      |    5 --
 5 files changed, 64 insertions(+), 139 deletions(-)

[PATCH 0/3] some ide fixes

[Borislav Petkov]

Hi Bart,

I've updated the two ide-floppy patches from you to current pata-2.6 and
added the partial completions fix to the second one so that we don't
break bisect builds (although this is hardly unlikely since nobody uses
an ide-floppy anymore). The 3rd patch is the ide-cd fix from linux-next
which has been tested by Tetsuo and me.

 drivers/ide/ide-atapi.c  |  136 +++++++++++++++-------------------------------
 drivers/ide/ide-cd.c     |    4 --
 drivers/ide/ide-floppy.c |   26 ++--------
 drivers/ide/ide-tape.c   |   32 ++++++------
 include/linux/ide.h      |    5 --
 5 files changed, 64 insertions(+), 139 deletions(-)

[PATCH 1/3] ide-{floppy,tape}: fix padding for PIO transfers

[Borislav Petkov]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

* Return number of bytes left to transfer from idetape_{in,out}put_buffers()
  and number of bytes done from ide_tape_io_buffers().

* Fix padding for PIO transfers in ide_pc_intr() so read/write buffers are
  always completely processed and then the transfer is padded if necessary.

* Remove invalid error messages.

* Remove now superfluous padding from ide{_io_buffers,tape_input_buffers}().

While at it:

* Set pc->bh to NULL in idetape_input_buffers() after all bh-s are done.

* Cache !!(pc->flags & PC_FLAG_WRITING) in local variable in ide_pc_intr().

Cc: Borislav Petkov <petkovbb@gmail.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-atapi.c |   57 +++++++++++++++-------------------------------
 drivers/ide/ide-tape.c  |   32 +++++++++++++-------------
 2 files changed, 35 insertions(+), 54 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index ff6adea..314e6af 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -108,13 +108,6 @@ int ide_io_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
 		}
 	}
 
-	if (bcount) {
-		printk(KERN_ERR "%s: %d leftover bytes, %s\n", drive->name,
-			bcount, write ? "padding with zeros"
-				      : "discarding data");
-		ide_pad_transfer(drive, write, bcount);
-	}
-
 	return done;
 }
 EXPORT_SYMBOL_GPL(ide_io_buffers);
@@ -316,9 +309,10 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
 	struct request *rq = hwif->rq;
 	const struct ide_tp_ops *tp_ops = hwif->tp_ops;
 	xfer_func_t *xferfunc;
-	unsigned int timeout, temp;
+	unsigned int timeout, done;
 	u16 bcount;
 	u8 stat, ireason, dsc = 0;
+	u8 write = !!(pc->flags & PC_FLAG_WRITING);
 
 	debug_log("Enter %s - interrupt handler\n", __func__);
 
@@ -427,8 +421,7 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
 		return ide_do_reset(drive);
 	}
 
-	if (((ireason & ATAPI_IO) == ATAPI_IO) ==
-		!!(pc->flags & PC_FLAG_WRITING)) {
+	if (((ireason & ATAPI_IO) == ATAPI_IO) == write) {
 		/* Hopefully, we will never get here */
 		printk(KERN_ERR "%s: We wanted to %s, but the device wants us "
 				"to %s!\n", drive->name,
@@ -437,45 +430,33 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
 		return ide_do_reset(drive);
 	}
 
-	if (!(pc->flags & PC_FLAG_WRITING)) {
-		/* Reading - Check that we have enough space */
-		temp = pc->xferred + bcount;
-		if (temp > pc->req_xfer) {
-			if (temp > pc->buf_size) {
-				printk(KERN_ERR "%s: The device wants to send "
-						"us more data than expected - "
-						"discarding data\n",
-						drive->name);
-
-				ide_pad_transfer(drive, 0, bcount);
-				goto next_irq;
-			}
-			debug_log("The device wants to send us more data than "
-				  "expected - allowing transfer\n");
-		}
-		xferfunc = tp_ops->input_data;
-	} else
-		xferfunc = tp_ops->output_data;
+	xferfunc = write ? tp_ops->output_data : tp_ops->input_data;
 
 	if ((drive->media == ide_floppy && !pc->buf) ||
 	    (drive->media == ide_tape && pc->bh)) {
-		int done = drive->pc_io_buffers(drive, pc, bcount,
-				  !!(pc->flags & PC_FLAG_WRITING));
+		done = drive->pc_io_buffers(drive, pc, bcount, write);
 
 		/* FIXME: don't do partial completions */
 		if (drive->media == ide_floppy)
 			ide_complete_rq(drive, 0,
 					done ? done : ide_rq_bytes(rq));
-	} else
-		xferfunc(drive, NULL, pc->cur_pos, bcount);
+	} else {
+		done = min_t(unsigned int, bcount, pc->req_xfer - pc->xferred);
+		xferfunc(drive, NULL, pc->cur_pos, done);
+	}
 
 	/* Update the current position */
-	pc->xferred += bcount;
-	pc->cur_pos += bcount;
+	pc->xferred += done;
+	pc->cur_pos += done;
+
+	bcount -= done;
+
+	if (bcount)
+		ide_pad_transfer(drive, write, bcount);
+
+	debug_log("[cmd %x] transferred %d bytes, padded %d bytes\n",
+		  rq->cmd[0], done, bcount);
 
-	debug_log("[cmd %x] transferred %d bytes on that intr.\n",
-		  rq->cmd[0], bcount);
-next_irq:
 	/* And set the interrupt handler again */
 	ide_set_handler(drive, ide_pc_intr, timeout);
 	return ide_started;
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 733cb51..12a6e5d 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -297,19 +297,15 @@ static struct ide_tape_obj *ide_tape_chrdev_get(unsigned int i)
 	return tape;
 }
 
-static void idetape_input_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
+static int idetape_input_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
 				  unsigned int bcount)
 {
 	struct idetape_bh *bh = pc->bh;
 	int count;
 
 	while (bcount) {
-		if (bh == NULL) {
-			printk(KERN_ERR "ide-tape: bh == NULL in "
-				"idetape_input_buffers\n");
-			ide_pad_transfer(drive, 0, bcount);
-			return;
-		}
+		if (bh == NULL)
+			break;
 		count = min(
 			(unsigned int)(bh->b_size - atomic_read(&bh->b_count)),
 			bcount);
@@ -323,21 +319,21 @@ static void idetape_input_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
 				atomic_set(&bh->b_count, 0);
 		}
 	}
+
 	pc->bh = bh;
+
+	return bcount;
 }
 
-static void idetape_output_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
+static int idetape_output_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
 				   unsigned int bcount)
 {
 	struct idetape_bh *bh = pc->bh;
 	int count;
 
 	while (bcount) {
-		if (bh == NULL) {
-			printk(KERN_ERR "ide-tape: bh == NULL in %s\n",
-					__func__);
-			return;
-		}
+		if (bh == NULL)
+			break;
 		count = min((unsigned int)pc->b_count, (unsigned int)bcount);
 		drive->hwif->tp_ops->output_data(drive, NULL, pc->b_data, count);
 		bcount -= count;
@@ -352,6 +348,8 @@ static void idetape_output_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
 			}
 		}
 	}
+
+	return bcount;
 }
 
 static void idetape_update_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc)
@@ -563,12 +561,14 @@ static void ide_tape_handle_dsc(ide_drive_t *drive)
 static int ide_tape_io_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
 				unsigned int bcount, int write)
 {
+	unsigned int bleft;
+
 	if (write)
-		idetape_output_buffers(drive, pc, bcount);
+		bleft = idetape_output_buffers(drive, pc, bcount);
 	else
-		idetape_input_buffers(drive, pc, bcount);
+		bleft = idetape_input_buffers(drive, pc, bcount);
 
-	return bcount;
+	return bcount - bleft;
 }
 
 /*
-- 
1.6.1.3

[PATCH 1/3] ide-{floppy,tape}: fix padding for PIO transfers

[Borislav Petkov]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

* Return number of bytes left to transfer from idetape_{in,out}put_buffers()
  and number of bytes done from ide_tape_io_buffers().

* Fix padding for PIO transfers in ide_pc_intr() so read/write buffers are
  always completely processed and then the transfer is padded if necessary.

* Remove invalid error messages.

* Remove now superfluous padding from ide{_io_buffers,tape_input_buffers}().

While at it:

* Set pc->bh to NULL in idetape_input_buffers() after all bh-s are done.

* Cache !!(pc->flags & PC_FLAG_WRITING) in local variable in ide_pc_intr().

Cc: Borislav Petkov <petkovbb@gmail.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-atapi.c |   57 +++++++++++++++-------------------------------
 drivers/ide/ide-tape.c  |   32 +++++++++++++-------------
 2 files changed, 35 insertions(+), 54 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index ff6adea..314e6af 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -108,13 +108,6 @@ int ide_io_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
 		}
 	}
 
-	if (bcount) {
-		printk(KERN_ERR "%s: %d leftover bytes, %s\n", drive->name,
-			bcount, write ? "padding with zeros"
-				      : "discarding data");
-		ide_pad_transfer(drive, write, bcount);
-	}
-
 	return done;
 }
 EXPORT_SYMBOL_GPL(ide_io_buffers);
@@ -316,9 +309,10 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
 	struct request *rq = hwif->rq;
 	const struct ide_tp_ops *tp_ops = hwif->tp_ops;
 	xfer_func_t *xferfunc;
-	unsigned int timeout, temp;
+	unsigned int timeout, done;
 	u16 bcount;
 	u8 stat, ireason, dsc = 0;
+	u8 write = !!(pc->flags & PC_FLAG_WRITING);
 
 	debug_log("Enter %s - interrupt handler\n", __func__);
 
@@ -427,8 +421,7 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
 		return ide_do_reset(drive);
 	}
 
-	if (((ireason & ATAPI_IO) == ATAPI_IO) ==
-		!!(pc->flags & PC_FLAG_WRITING)) {
+	if (((ireason & ATAPI_IO) == ATAPI_IO) == write) {
 		/* Hopefully, we will never get here */
 		printk(KERN_ERR "%s: We wanted to %s, but the device wants us "
 				"to %s!\n", drive->name,
@@ -437,45 +430,33 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
 		return ide_do_reset(drive);
 	}
 
-	if (!(pc->flags & PC_FLAG_WRITING)) {
-		/* Reading - Check that we have enough space */
-		temp = pc->xferred + bcount;
-		if (temp > pc->req_xfer) {
-			if (temp > pc->buf_size) {
-				printk(KERN_ERR "%s: The device wants to send "
-						"us more data than expected - "
-						"discarding data\n",
-						drive->name);
-
-				ide_pad_transfer(drive, 0, bcount);
-				goto next_irq;
-			}
-			debug_log("The device wants to send us more data than "
-				  "expected - allowing transfer\n");
-		}
-		xferfunc = tp_ops->input_data;
-	} else
-		xferfunc = tp_ops->output_data;
+	xferfunc = write ? tp_ops->output_data : tp_ops->input_data;
 
 	if ((drive->media == ide_floppy && !pc->buf) ||
 	    (drive->media == ide_tape && pc->bh)) {
-		int done = drive->pc_io_buffers(drive, pc, bcount,
-				  !!(pc->flags & PC_FLAG_WRITING));
+		done = drive->pc_io_buffers(drive, pc, bcount, write);
 
 		/* FIXME: don't do partial completions */
 		if (drive->media == ide_floppy)
 			ide_complete_rq(drive, 0,
 					done ? done : ide_rq_bytes(rq));
-	} else
-		xferfunc(drive, NULL, pc->cur_pos, bcount);
+	} else {
+		done = min_t(unsigned int, bcount, pc->req_xfer - pc->xferred);
+		xferfunc(drive, NULL, pc->cur_pos, done);
+	}
 
 	/* Update the current position */
-	pc->xferred += bcount;
-	pc->cur_pos += bcount;
+	pc->xferred += done;
+	pc->cur_pos += done;
+
+	bcount -= done;
+
+	if (bcount)
+		ide_pad_transfer(drive, write, bcount);
+
+	debug_log("[cmd %x] transferred %d bytes, padded %d bytes\n",
+		  rq->cmd[0], done, bcount);
 
-	debug_log("[cmd %x] transferred %d bytes on that intr.\n",
-		  rq->cmd[0], bcount);
-next_irq:
 	/* And set the interrupt handler again */
 	ide_set_handler(drive, ide_pc_intr, timeout);
 	return ide_started;
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 733cb51..12a6e5d 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -297,19 +297,15 @@ static struct ide_tape_obj *ide_tape_chrdev_get(unsigned int i)
 	return tape;
 }
 
-static void idetape_input_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
+static int idetape_input_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
 				  unsigned int bcount)
 {
 	struct idetape_bh *bh = pc->bh;
 	int count;
 
 	while (bcount) {
-		if (bh == NULL) {
-			printk(KERN_ERR "ide-tape: bh == NULL in "
-				"idetape_input_buffers\n");
-			ide_pad_transfer(drive, 0, bcount);
-			return;
-		}
+		if (bh == NULL)
+			break;
 		count = min(
 			(unsigned int)(bh->b_size - atomic_read(&bh->b_count)),
 			bcount);
@@ -323,21 +319,21 @@ static void idetape_input_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
 				atomic_set(&bh->b_count, 0);
 		}
 	}
+
 	pc->bh = bh;
+
+	return bcount;
 }
 
-static void idetape_output_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
+static int idetape_output_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
 				   unsigned int bcount)
 {
 	struct idetape_bh *bh = pc->bh;
 	int count;
 
 	while (bcount) {
-		if (bh == NULL) {
-			printk(KERN_ERR "ide-tape: bh == NULL in %s\n",
-					__func__);
-			return;
-		}
+		if (bh == NULL)
+			break;
 		count = min((unsigned int)pc->b_count, (unsigned int)bcount);
 		drive->hwif->tp_ops->output_data(drive, NULL, pc->b_data, count);
 		bcount -= count;
@@ -352,6 +348,8 @@ static void idetape_output_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
 			}
 		}
 	}
+
+	return bcount;
 }
 
 static void idetape_update_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc)
@@ -563,12 +561,14 @@ static void ide_tape_handle_dsc(ide_drive_t *drive)
 static int ide_tape_io_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
 				unsigned int bcount, int write)
 {
+	unsigned int bleft;
+
 	if (write)
-		idetape_output_buffers(drive, pc, bcount);
+		bleft = idetape_output_buffers(drive, pc, bcount);
 	else
-		idetape_input_buffers(drive, pc, bcount);
+		bleft = idetape_input_buffers(drive, pc, bcount);
 
-	return bcount;
+	return bcount - bleft;
 }
 
 /*
-- 
1.6.1.3

[PATCH 2/3] ide-floppy: use ide_pio_bytes()

[Borislav Petkov]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

* Fix ide_init_sg_cmd() setup for non-fs requests.

* Convert ide_pc_intr() to use ide_pio_bytes() for floppy media.

* Remove no longer needed ide_io_buffers() and sg/sg_cnt fields
  from struct ide_atapi_pc.

* Remove partial completions; kill idefloppy_update_buffers(), as a
  result.

* Add some more debugging statements.

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-atapi.c  |   68 +++++++++++-----------------------------------
 drivers/ide/ide-floppy.c |   24 +++-------------
 include/linux/ide.h      |    5 ---
 3 files changed, 21 insertions(+), 76 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 314e6af..a5596a6 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -69,49 +69,6 @@ int ide_check_atapi_device(ide_drive_t *drive, const char *s)
 }
 EXPORT_SYMBOL_GPL(ide_check_atapi_device);
 
-/* PIO data transfer routine using the scatter gather table. */
-int ide_io_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
-		    unsigned int bcount, int write)
-{
-	ide_hwif_t *hwif = drive->hwif;
-	const struct ide_tp_ops *tp_ops = hwif->tp_ops;
-	xfer_func_t *xf = write ? tp_ops->output_data : tp_ops->input_data;
-	struct scatterlist *sg = pc->sg;
-	char *buf;
-	int count, done = 0;
-
-	while (bcount) {
-		count = min(sg->length - pc->b_count, bcount);
-
-		if (PageHighMem(sg_page(sg))) {
-			unsigned long flags;
-
-			local_irq_save(flags);
-			buf = kmap_atomic(sg_page(sg), KM_IRQ0) + sg->offset;
-			xf(drive, NULL, buf + pc->b_count, count);
-			kunmap_atomic(buf - sg->offset, KM_IRQ0);
-			local_irq_restore(flags);
-		} else {
-			buf = sg_virt(sg);
-			xf(drive, NULL, buf + pc->b_count, count);
-		}
-
-		bcount -= count;
-		pc->b_count += count;
-		done += count;
-
-		if (pc->b_count == sg->length) {
-			if (!--pc->sg_cnt)
-				break;
-			pc->sg = sg = sg_next(sg);
-			pc->b_count = 0;
-		}
-	}
-
-	return done;
-}
-EXPORT_SYMBOL_GPL(ide_io_buffers);
-
 void ide_init_pc(struct ide_atapi_pc *pc)
 {
 	memset(pc, 0, sizeof(*pc));
@@ -339,6 +296,9 @@ 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);
 	}
@@ -394,12 +354,19 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
 			rq->errors = 0;
 			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;
 			}
-			ide_complete_rq(drive, uptodate ? 0 : -EIO,
-					ide_rq_bytes(rq));
+
+			if (drive->media == ide_tape)
+				done = ide_rq_bytes(rq); /* FIXME */
+			else
+				done = blk_rq_bytes(rq);
+
+			ide_complete_rq(drive, uptodate ? 0 : -EIO, done);
 		}
 
 		return ide_stopped;
@@ -432,14 +399,11 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
 
 	xferfunc = write ? tp_ops->output_data : tp_ops->input_data;
 
-	if ((drive->media == ide_floppy && !pc->buf) ||
-	    (drive->media == ide_tape && pc->bh)) {
+	if (drive->media == ide_floppy && pc->buf == NULL) {
+		done = min_t(unsigned int, bcount, cmd->nleft);
+		ide_pio_bytes(drive, cmd, write, done);
+	} else if (drive->media == ide_tape && pc->bh) {
 		done = drive->pc_io_buffers(drive, pc, bcount, write);
-
-		/* FIXME: don't do partial completions */
-		if (drive->media == ide_floppy)
-			ide_complete_rq(drive, 0,
-					done ? done : ide_rq_bytes(rq));
 	} else {
 		done = min_t(unsigned int, bcount, pc->req_xfer - pc->xferred);
 		xferfunc(drive, NULL, pc->cur_pos, done);
diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index 50d918a..2f8f453 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -61,16 +61,6 @@
  */
 #define IDEFLOPPY_PC_DELAY	(HZ/20)	/* default delay for ZIP 100 (50ms) */
 
-static void idefloppy_update_buffers(ide_drive_t *drive,
-				struct ide_atapi_pc *pc)
-{
-	struct request *rq = pc->rq;
-	struct bio *bio = rq->bio;
-
-	while ((bio = rq->bio) != NULL)
-		ide_complete_rq(drive, 0, ide_rq_bytes(rq));
-}
-
 static int ide_floppy_callback(ide_drive_t *drive, int dsc)
 {
 	struct ide_disk_obj *floppy = drive->driver_data;
@@ -213,7 +203,6 @@ static void idefloppy_create_rw_cmd(ide_drive_t *drive,
 	memcpy(rq->cmd, pc->c, 12);
 
 	pc->rq = rq;
-	pc->b_count = 0;
 	if (rq->cmd_flags & REQ_RW)
 		pc->flags |= PC_FLAG_WRITING;
 	pc->buf = NULL;
@@ -227,7 +216,6 @@ static void idefloppy_blockpc_cmd(struct ide_disk_obj *floppy,
 	ide_init_pc(pc);
 	memcpy(pc->c, rq->cmd, sizeof(pc->c));
 	pc->rq = rq;
-	pc->b_count = 0;
 	if (rq->data_len && rq_data_dir(rq) == WRITE)
 		pc->flags |= PC_FLAG_WRITING;
 	pc->buf = rq->data;
@@ -244,10 +232,11 @@ static ide_startstop_t ide_floppy_do_request(ide_drive_t *drive,
 					     struct request *rq, sector_t block)
 {
 	struct ide_disk_obj *floppy = drive->driver_data;
-	ide_hwif_t *hwif = drive->hwif;
 	struct ide_cmd cmd;
 	struct ide_atapi_pc *pc;
 
+	ide_debug_log(IDE_DBG_FUNC, "enter, cmd: 0x%x\n", rq->cmd[0]);
+
 	if (drive->debug_mask & IDE_DBG_RQ)
 		blk_dump_rq_flags(rq, (rq->rq_disk
 					? rq->rq_disk->disk_name
@@ -293,12 +282,9 @@ static ide_startstop_t ide_floppy_do_request(ide_drive_t *drive,
 
 	cmd.rq = rq;
 
-	ide_init_sg_cmd(&cmd, rq->nr_sectors << 9);
+	ide_init_sg_cmd(&cmd, pc->req_xfer);
 	ide_map_sg(drive, &cmd);
 
-	pc->sg = hwif->sg_table;
-	pc->sg_cnt = cmd.sg_nents;
-
 	pc->rq = rq;
 
 	return ide_floppy_issue_pc(drive, &cmd, pc);
@@ -386,6 +372,8 @@ static int ide_floppy_get_capacity(ide_drive_t *drive)
 	u8 pc_buf[256], header_len, desc_cnt;
 	int i, rc = 1, blocks, length;
 
+	ide_debug_log(IDE_DBG_FUNC, "enter");
+
 	drive->bios_cyl = 0;
 	drive->bios_head = drive->bios_sect = 0;
 	floppy->blocks = 0;
@@ -486,8 +474,6 @@ static void ide_floppy_setup(ide_drive_t *drive)
 	u16 *id = drive->id;
 
 	drive->pc_callback	 = ide_floppy_callback;
-	drive->pc_update_buffers = idefloppy_update_buffers;
-	drive->pc_io_buffers	 = ide_io_buffers;
 
 	/*
 	 * We used to check revisions here. At this point however I'm giving up.
diff --git a/include/linux/ide.h b/include/linux/ide.h
index e53f820..09bc586 100644
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -415,9 +415,6 @@ struct ide_atapi_pc {
 	struct idetape_bh *bh;
 	char *b_data;
 
-	struct scatterlist *sg;
-	unsigned int sg_cnt;
-
 	unsigned long timeout;
 };
 
@@ -1175,8 +1172,6 @@ void ide_tf_read(ide_drive_t *, struct ide_cmd *);
 void ide_input_data(ide_drive_t *, struct ide_cmd *, void *, unsigned int);
 void ide_output_data(ide_drive_t *, struct ide_cmd *, void *, unsigned int);
 
-int ide_io_buffers(ide_drive_t *, struct ide_atapi_pc *, unsigned int, int);
-
 extern void SELECT_DRIVE(ide_drive_t *);
 void SELECT_MASK(ide_drive_t *, int);
 
-- 
1.6.1.3

[PATCH 2/3] ide-floppy: use ide_pio_bytes()

[Borislav Petkov]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

* Fix ide_init_sg_cmd() setup for non-fs requests.

* Convert ide_pc_intr() to use ide_pio_bytes() for floppy media.

* Remove no longer needed ide_io_buffers() and sg/sg_cnt fields
  from struct ide_atapi_pc.

* Remove partial completions; kill idefloppy_update_buffers(), as a
  result.

* Add some more debugging statements.

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-atapi.c  |   68 +++++++++++-----------------------------------
 drivers/ide/ide-floppy.c |   24 +++-------------
 include/linux/ide.h      |    5 ---
 3 files changed, 21 insertions(+), 76 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 314e6af..a5596a6 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -69,49 +69,6 @@ int ide_check_atapi_device(ide_drive_t *drive, const char *s)
 }
 EXPORT_SYMBOL_GPL(ide_check_atapi_device);
 
-/* PIO data transfer routine using the scatter gather table. */
-int ide_io_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
-		    unsigned int bcount, int write)
-{
-	ide_hwif_t *hwif = drive->hwif;
-	const struct ide_tp_ops *tp_ops = hwif->tp_ops;
-	xfer_func_t *xf = write ? tp_ops->output_data : tp_ops->input_data;
-	struct scatterlist *sg = pc->sg;
-	char *buf;
-	int count, done = 0;
-
-	while (bcount) {
-		count = min(sg->length - pc->b_count, bcount);
-
-		if (PageHighMem(sg_page(sg))) {
-			unsigned long flags;
-
-			local_irq_save(flags);
-			buf = kmap_atomic(sg_page(sg), KM_IRQ0) + sg->offset;
-			xf(drive, NULL, buf + pc->b_count, count);
-			kunmap_atomic(buf - sg->offset, KM_IRQ0);
-			local_irq_restore(flags);
-		} else {
-			buf = sg_virt(sg);
-			xf(drive, NULL, buf + pc->b_count, count);
-		}
-
-		bcount -= count;
-		pc->b_count += count;
-		done += count;
-
-		if (pc->b_count == sg->length) {
-			if (!--pc->sg_cnt)
-				break;
-			pc->sg = sg = sg_next(sg);
-			pc->b_count = 0;
-		}
-	}
-
-	return done;
-}
-EXPORT_SYMBOL_GPL(ide_io_buffers);
-
 void ide_init_pc(struct ide_atapi_pc *pc)
 {
 	memset(pc, 0, sizeof(*pc));
@@ -339,6 +296,9 @@ 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);
 	}
@@ -394,12 +354,19 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
 			rq->errors = 0;
 			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;
 			}
-			ide_complete_rq(drive, uptodate ? 0 : -EIO,
-					ide_rq_bytes(rq));
+
+			if (drive->media == ide_tape)
+				done = ide_rq_bytes(rq); /* FIXME */
+			else
+				done = blk_rq_bytes(rq);
+
+			ide_complete_rq(drive, uptodate ? 0 : -EIO, done);
 		}
 
 		return ide_stopped;
@@ -432,14 +399,11 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
 
 	xferfunc = write ? tp_ops->output_data : tp_ops->input_data;
 
-	if ((drive->media == ide_floppy && !pc->buf) ||
-	    (drive->media == ide_tape && pc->bh)) {
+	if (drive->media == ide_floppy && pc->buf == NULL) {
+		done = min_t(unsigned int, bcount, cmd->nleft);
+		ide_pio_bytes(drive, cmd, write, done);
+	} else if (drive->media == ide_tape && pc->bh) {
 		done = drive->pc_io_buffers(drive, pc, bcount, write);
-
-		/* FIXME: don't do partial completions */
-		if (drive->media == ide_floppy)
-			ide_complete_rq(drive, 0,
-					done ? done : ide_rq_bytes(rq));
 	} else {
 		done = min_t(unsigned int, bcount, pc->req_xfer - pc->xferred);
 		xferfunc(drive, NULL, pc->cur_pos, done);
diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index 50d918a..2f8f453 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -61,16 +61,6 @@
  */
 #define IDEFLOPPY_PC_DELAY	(HZ/20)	/* default delay for ZIP 100 (50ms) */
 
-static void idefloppy_update_buffers(ide_drive_t *drive,
-				struct ide_atapi_pc *pc)
-{
-	struct request *rq = pc->rq;
-	struct bio *bio = rq->bio;
-
-	while ((bio = rq->bio) != NULL)
-		ide_complete_rq(drive, 0, ide_rq_bytes(rq));
-}
-
 static int ide_floppy_callback(ide_drive_t *drive, int dsc)
 {
 	struct ide_disk_obj *floppy = drive->driver_data;
@@ -213,7 +203,6 @@ static void idefloppy_create_rw_cmd(ide_drive_t *drive,
 	memcpy(rq->cmd, pc->c, 12);
 
 	pc->rq = rq;
-	pc->b_count = 0;
 	if (rq->cmd_flags & REQ_RW)
 		pc->flags |= PC_FLAG_WRITING;
 	pc->buf = NULL;
@@ -227,7 +216,6 @@ static void idefloppy_blockpc_cmd(struct ide_disk_obj *floppy,
 	ide_init_pc(pc);
 	memcpy(pc->c, rq->cmd, sizeof(pc->c));
 	pc->rq = rq;
-	pc->b_count = 0;
 	if (rq->data_len && rq_data_dir(rq) == WRITE)
 		pc->flags |= PC_FLAG_WRITING;
 	pc->buf = rq->data;
@@ -244,10 +232,11 @@ static ide_startstop_t ide_floppy_do_request(ide_drive_t *drive,
 					     struct request *rq, sector_t block)
 {
 	struct ide_disk_obj *floppy = drive->driver_data;
-	ide_hwif_t *hwif = drive->hwif;
 	struct ide_cmd cmd;
 	struct ide_atapi_pc *pc;
 
+	ide_debug_log(IDE_DBG_FUNC, "enter, cmd: 0x%x\n", rq->cmd[0]);
+
 	if (drive->debug_mask & IDE_DBG_RQ)
 		blk_dump_rq_flags(rq, (rq->rq_disk
 					? rq->rq_disk->disk_name
@@ -293,12 +282,9 @@ static ide_startstop_t ide_floppy_do_request(ide_drive_t *drive,
 
 	cmd.rq = rq;
 
-	ide_init_sg_cmd(&cmd, rq->nr_sectors << 9);
+	ide_init_sg_cmd(&cmd, pc->req_xfer);
 	ide_map_sg(drive, &cmd);
 
-	pc->sg = hwif->sg_table;
-	pc->sg_cnt = cmd.sg_nents;
-
 	pc->rq = rq;
 
 	return ide_floppy_issue_pc(drive, &cmd, pc);
@@ -386,6 +372,8 @@ static int ide_floppy_get_capacity(ide_drive_t *drive)
 	u8 pc_buf[256], header_len, desc_cnt;
 	int i, rc = 1, blocks, length;
 
+	ide_debug_log(IDE_DBG_FUNC, "enter");
+
 	drive->bios_cyl = 0;
 	drive->bios_head = drive->bios_sect = 0;
 	floppy->blocks = 0;
@@ -486,8 +474,6 @@ static void ide_floppy_setup(ide_drive_t *drive)
 	u16 *id = drive->id;
 
 	drive->pc_callback	 = ide_floppy_callback;
-	drive->pc_update_buffers = idefloppy_update_buffers;
-	drive->pc_io_buffers	 = ide_io_buffers;
 
 	/*
 	 * We used to check revisions here. At this point however I'm giving up.
diff --git a/include/linux/ide.h b/include/linux/ide.h
index e53f820..09bc586 100644
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -415,9 +415,6 @@ struct ide_atapi_pc {
 	struct idetape_bh *bh;
 	char *b_data;
 
-	struct scatterlist *sg;
-	unsigned int sg_cnt;
-
 	unsigned long timeout;
 };
 
@@ -1175,8 +1172,6 @@ void ide_tf_read(ide_drive_t *, struct ide_cmd *);
 void ide_input_data(ide_drive_t *, struct ide_cmd *, void *, unsigned int);
 void ide_output_data(ide_drive_t *, struct ide_cmd *, void *, unsigned int);
 
-int ide_io_buffers(ide_drive_t *, struct ide_atapi_pc *, unsigned int, int);
-
 extern void SELECT_DRIVE(ide_drive_t *);
 void SELECT_MASK(ide_drive_t *, int);
 
-- 
1.6.1.3

[PATCH 3/3] ide-{cd,floppy}: do not map all cmds to an sg

[Borislav Petkov]

... since some do not transfer any data from the drive and
rq->buffer is unset leading to an OOPS when checking VM translations
(CONFIG_DEBUG_VIRTUAL).

Tested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-atapi.c  |    9 ++++++++-
 drivers/ide/ide-cd.c     |    4 ----
 drivers/ide/ide-floppy.c |    4 ----
 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index a5596a6..ef78cbf 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -570,7 +570,7 @@ static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
 
 ide_startstop_t ide_issue_pc(ide_drive_t *drive, struct ide_cmd *cmd)
 {
-	struct ide_atapi_pc *pc;
+	struct ide_atapi_pc *uninitialized_var(pc);
 	ide_hwif_t *hwif = drive->hwif;
 	ide_expiry_t *expiry = NULL;
 	struct request *rq = hwif->rq;
@@ -587,6 +587,7 @@ ide_startstop_t ide_issue_pc(ide_drive_t *drive, struct ide_cmd *cmd)
 
 		if (drive->dma)
 			drive->dma = !ide_dma_prepare(drive, cmd);
+
 	} else {
 		pc = drive->pc;
 
@@ -614,6 +615,12 @@ ide_startstop_t ide_issue_pc(ide_drive_t *drive, struct ide_cmd *cmd)
 						       : WAIT_TAPE_CMD;
 	}
 
+	if (drive->media != ide_tape &&
+	      !drive->dma && (blk_fs_request(rq) || rq->data_len)) {
+		ide_init_sg_cmd(cmd, blk_rq_bytes(rq));
+		ide_map_sg(drive, cmd);
+	}
+
 	ide_init_packet_cmd(cmd, tf_flags, bcount, drive->dma);
 
 	(void)do_rw_taskfile(drive, cmd);
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 091d414..106cacb 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -916,10 +916,6 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
 
 	cmd.rq = rq;
 
-	ide_init_sg_cmd(&cmd,
-		blk_fs_request(rq) ? (rq->nr_sectors << 9) : rq->data_len);
-	ide_map_sg(drive, &cmd);
-
 	return ide_issue_pc(drive, &cmd);
 out_end:
 	nsectors = rq->hard_nr_sectors;
diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index 2f8f453..b91deef 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -281,10 +281,6 @@ static ide_startstop_t ide_floppy_do_request(ide_drive_t *drive,
 		cmd.tf_flags |= IDE_TFLAG_WRITE;
 
 	cmd.rq = rq;
-
-	ide_init_sg_cmd(&cmd, pc->req_xfer);
-	ide_map_sg(drive, &cmd);
-
 	pc->rq = rq;
 
 	return ide_floppy_issue_pc(drive, &cmd, pc);
-- 
1.6.1.3

[PATCH 3/3] ide-{cd,floppy}: do not map all cmds to an sg

[Borislav Petkov]

... since some do not transfer any data from the drive and
rq->buffer is unset leading to an OOPS when checking VM translations
(CONFIG_DEBUG_VIRTUAL).

Tested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-atapi.c  |    9 ++++++++-
 drivers/ide/ide-cd.c     |    4 ----
 drivers/ide/ide-floppy.c |    4 ----
 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index a5596a6..ef78cbf 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -570,7 +570,7 @@ static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
 
 ide_startstop_t ide_issue_pc(ide_drive_t *drive, struct ide_cmd *cmd)
 {
-	struct ide_atapi_pc *pc;
+	struct ide_atapi_pc *uninitialized_var(pc);
 	ide_hwif_t *hwif = drive->hwif;
 	ide_expiry_t *expiry = NULL;
 	struct request *rq = hwif->rq;
@@ -587,6 +587,7 @@ ide_startstop_t ide_issue_pc(ide_drive_t *drive, struct ide_cmd *cmd)
 
 		if (drive->dma)
 			drive->dma = !ide_dma_prepare(drive, cmd);
+
 	} else {
 		pc = drive->pc;
 
@@ -614,6 +615,12 @@ ide_startstop_t ide_issue_pc(ide_drive_t *drive, struct ide_cmd *cmd)
 						       : WAIT_TAPE_CMD;
 	}
 
+	if (drive->media != ide_tape &&
+	      !drive->dma && (blk_fs_request(rq) || rq->data_len)) {
+		ide_init_sg_cmd(cmd, blk_rq_bytes(rq));
+		ide_map_sg(drive, cmd);
+	}
+
 	ide_init_packet_cmd(cmd, tf_flags, bcount, drive->dma);
 
 	(void)do_rw_taskfile(drive, cmd);
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 091d414..106cacb 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -916,10 +916,6 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
 
 	cmd.rq = rq;
 
-	ide_init_sg_cmd(&cmd,
-		blk_fs_request(rq) ? (rq->nr_sectors << 9) : rq->data_len);
-	ide_map_sg(drive, &cmd);
-
 	return ide_issue_pc(drive, &cmd);
 out_end:
 	nsectors = rq->hard_nr_sectors;
diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index 2f8f453..b91deef 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -281,10 +281,6 @@ static ide_startstop_t ide_floppy_do_request(ide_drive_t *drive,
 		cmd.tf_flags |= IDE_TFLAG_WRITE;
 
 	cmd.rq = rq;
-
-	ide_init_sg_cmd(&cmd, pc->req_xfer);
-	ide_map_sg(drive, &cmd);
-
 	pc->rq = rq;
 
 	return ide_floppy_issue_pc(drive, &cmd, pc);
-- 
1.6.1.3

Re: [PATCH 3/3] ide-{cd,floppy}: do not map all cmds to an sg

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday 04 March 2009, Borislav Petkov wrote:
> ... since some do not transfer any data from the drive and
> rq->buffer is unset leading to an OOPS when checking VM translations
> (CONFIG_DEBUG_VIRTUAL).
> 
> Tested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>

Thanks for fixing it but I worry that the patch is not entirely correct
(why o why did you have to throw in unrelated changes...?)

also ideally there should have been two patches:
- minimal bugfix for the buggy patch
- unification/cleanup of PIO sg setup

> ---
>  drivers/ide/ide-atapi.c  |    9 ++++++++-
>  drivers/ide/ide-cd.c     |    4 ----
>  drivers/ide/ide-floppy.c |    4 ----
>  3 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
> index a5596a6..ef78cbf 100644
> --- a/drivers/ide/ide-atapi.c
> +++ b/drivers/ide/ide-atapi.c
> @@ -570,7 +570,7 @@ static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
>  
>  ide_startstop_t ide_issue_pc(ide_drive_t *drive, struct ide_cmd *cmd)
>  {
> -	struct ide_atapi_pc *pc;
> +	struct ide_atapi_pc *uninitialized_var(pc);
>  	ide_hwif_t *hwif = drive->hwif;
>  	ide_expiry_t *expiry = NULL;
>  	struct request *rq = hwif->rq;
> @@ -587,6 +587,7 @@ ide_startstop_t ide_issue_pc(ide_drive_t *drive, struct ide_cmd *cmd)
>  
>  		if (drive->dma)
>  			drive->dma = !ide_dma_prepare(drive, cmd);
> +

?

>  	} else {
>  		pc = drive->pc;
>  
> @@ -614,6 +615,12 @@ ide_startstop_t ide_issue_pc(ide_drive_t *drive, struct ide_cmd *cmd)
>  						       : WAIT_TAPE_CMD;
>  	}
>  
> +	if (drive->media != ide_tape &&
> +	      !drive->dma && (blk_fs_request(rq) || rq->data_len)) {
> +		ide_init_sg_cmd(cmd, blk_rq_bytes(rq));
> +		ide_map_sg(drive, cmd);
> +	}
> +
>  	ide_init_packet_cmd(cmd, tf_flags, bcount, drive->dma);
>  
>  	(void)do_rw_taskfile(drive, cmd);
> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> index 091d414..106cacb 100644
> --- a/drivers/ide/ide-cd.c
> +++ b/drivers/ide/ide-cd.c
> @@ -916,10 +916,6 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
>  
>  	cmd.rq = rq;
>  
> -	ide_init_sg_cmd(&cmd,
> -		blk_fs_request(rq) ? (rq->nr_sectors << 9) : rq->data_len);
> -	ide_map_sg(drive, &cmd);
> -
>  	return ide_issue_pc(drive, &cmd);
>  out_end:
>  	nsectors = rq->hard_nr_sectors;
> diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
> index 2f8f453..b91deef 100644
> --- a/drivers/ide/ide-floppy.c
> +++ b/drivers/ide/ide-floppy.c
> @@ -281,10 +281,6 @@ static ide_startstop_t ide_floppy_do_request(ide_drive_t *drive,
>  		cmd.tf_flags |= IDE_TFLAG_WRITE;
>  
>  	cmd.rq = rq;
> -
> -	ide_init_sg_cmd(&cmd, pc->req_xfer);

There was a reason for using ->req_xfer.

I don't see where rq->data_len is set for REQ_TYPE_SPECIAL requests
(ide_queue_pc_tail() and friends)?

> -	ide_map_sg(drive, &cmd);
> -
>  	pc->rq = rq;
>  
>  	return ide_floppy_issue_pc(drive, &cmd, pc);

Re: [PATCH 2/3] ide-floppy: use ide_pio_bytes()

[Bartlomiej Zolnierkiewicz]

On Wednesday 04 March 2009, Borislav Petkov wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 
> * Fix ide_init_sg_cmd() setup for non-fs requests.
> 
> * Convert ide_pc_intr() to use ide_pio_bytes() for floppy media.
> 
> * Remove no longer needed ide_io_buffers() and sg/sg_cnt fields
>   from struct ide_atapi_pc.
> 
> * Remove partial completions; kill idefloppy_update_buffers(), as a
>   result.
> 
> * Add some more debugging statements.
> 
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>

thanks, applied 1-2

Re: [PATCH 3/3] ide-{cd,floppy}: do not map all cmds to an sg

[Borislav Petkov]

Hi,

On Thu, Mar 5, 2009 at 1:12 PM, Bartlomiej Zolnierkiewicz
<bzolnier@gmail.com> wrote:
>
> Hi,
>
> On Wednesday 04 March 2009, Borislav Petkov wrote:
>> ... since some do not transfer any data from the drive and
>> rq->buffer is unset leading to an OOPS when checking VM translations
>> (CONFIG_DEBUG_VIRTUAL).
>>
>> Tested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
>
> Thanks for fixing it but I worry that the patch is not entirely correct
> (why o why did you have to throw in unrelated changes...?)

What unrelated changes? This is the ide-cd fix for when mapping a NULL
rq->buffer pointer to an sg. If you mean the [2/3] patch, I had to
remove the partial completions for ide-floppy because it was OOPSing on NULL
rq-pointer in the interrupt handler. I sent you a pretty detailed OOPS along
with backtracking to the offending code line, remember? Also, for reasons of
bisectability.

> also ideally there should have been two patches:
> - minimal bugfix for the buggy patch
> - unification/cleanup of PIO sg setup
>
>> ---
>>  drivers/ide/ide-atapi.c  |    9 ++++++++-
>>  drivers/ide/ide-cd.c     |    4 ----
>>  drivers/ide/ide-floppy.c |    4 ----
>>  3 files changed, 8 insertions(+), 9 deletions(-)
>>

..

>> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
>> index 091d414..106cacb 100644
>> --- a/drivers/ide/ide-cd.c
>> +++ b/drivers/ide/ide-cd.c
>> @@ -916,10 +916,6 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
>>
>>       cmd.rq = rq;
>>
>> -     ide_init_sg_cmd(&cmd,
>> -             blk_fs_request(rq) ? (rq->nr_sectors << 9) : rq->data_len);
>> -     ide_map_sg(drive, &cmd);
>> -
>>       return ide_issue_pc(drive, &cmd);
>>  out_end:
>>       nsectors = rq->hard_nr_sectors;
>> diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
>> index 2f8f453..b91deef 100644
>> --- a/drivers/ide/ide-floppy.c
>> +++ b/drivers/ide/ide-floppy.c
>> @@ -281,10 +281,6 @@ static ide_startstop_t ide_floppy_do_request(ide_drive_t *drive,
>>               cmd.tf_flags |= IDE_TFLAG_WRITE;
>>
>>       cmd.rq = rq;
>> -
>> -     ide_init_sg_cmd(&cmd, pc->req_xfer);
>
> There was a reason for using ->req_xfer.

Yeah, this is also one of those painful places where I cringe everytime I have
to look at. We finally have to sit down and decide which is which; sometimes
pc->req_xfer _is_ rq->data_len (idefloppy_blockpc_cmd()) and sometimes obviously
not. I'll take a look at that whole mess next and try to come up with something
more clean.

> I don't see where rq->data_len is set for REQ_TYPE_SPECIAL requests
> (ide_queue_pc_tail() and friends)?

me neither :(.

>> -     ide_map_sg(drive, &cmd);
>> -
>>       pc->rq = rq;
>>
>>       return ide_floppy_issue_pc(drive, &cmd, pc);

Man, this sucks!

-- 
Regards/Gruss,
Boris

Re: [PATCH 3/3] ide-{cd,floppy}: do not map all cmds to an sg

[Bartlomiej Zolnierkiewicz]

On Thursday 05 March 2009, Borislav Petkov wrote:
> Hi,
> 
> On Thu, Mar 5, 2009 at 1:12 PM, Bartlomiej Zolnierkiewicz
> <bzolnier@gmail.com> wrote:
> >
> > Hi,
> >
> > On Wednesday 04 March 2009, Borislav Petkov wrote:
> >> ... since some do not transfer any data from the drive and
> >> rq->buffer is unset leading to an OOPS when checking VM translations
> >> (CONFIG_DEBUG_VIRTUAL).
> >>
> >> Tested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> >> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
> >
> > Thanks for fixing it but I worry that the patch is not entirely correct
> > (why o why did you have to throw in unrelated changes...?)
> 
> What unrelated changes? This is the ide-cd fix for when mapping a NULL

Unrelated to the original issue that the patch tries to fix
(sg mapping no data commands which OOPS-es w/ DEBUG_VIRTUAL=y):
- moving ide_init_sg_cmd()+ide_map_sg() to ide_issue_pc()
- converting the code to use blk_rq_bytes()

[ Moreover if you actually cared to write down the proper changelog
  I'm pretty sure that you would ask yourself whether these changes
  really belong to a bugfix patch... ]

Why not simply do what originally has been suggested:

--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -916,9 +916,11 @@ static ide_startstop_t ide_cd_do_request
 
 	cmd.rq = rq;
 
-	ide_init_sg_cmd(&cmd,
-		blk_fs_request(rq) ? (rq->nr_sectors << 9) : rq->data_len);
-	ide_map_sg(drive, &cmd);
+	if (blk_fs_request(rq) || rq->data_len) {
+		ide_init_sg_cmd(&cmd, blk_fs_request(rq) ? (rq->nr_sectors << 9)
+							 : rq->data_len);
+		ide_map_sg(drive, &cmd);
+	}
 
 	return ide_issue_pc(drive, &cmd);
 out_end:

?

The bonus is that it can be either queued after the problematic patch
or just merged with it later (even if it needs to be applied by-hand
the change is so trivial that it is very unlikely to cause problems).

Doing cleanup is fine but not at the same time because we don't want to
make things more complex (they are pretty complex already) and thus risk
introducing new bugs.  Yes, it is tempting to do few things at once and
save some work, and it is also perfectly fine to do it sometimes but we
have to draw a line somewhere...

> rq->buffer pointer to an sg. If you mean the [2/3] patch, I had to
> remove the partial completions for ide-floppy because it was OOPSing on NULL
> rq-pointer in the interrupt handler. I sent you a pretty detailed OOPS along
> with backtracking to the offending code line, remember? Also, for reasons of
> bisectability.

I meant this patch, 2/3 is fine and was applied
(thanks for fixing it again).

> > also ideally there should have been two patches:
> > - minimal bugfix for the buggy patch
> > - unification/cleanup of PIO sg setup
> >
> >> ---
> >>  drivers/ide/ide-atapi.c  |    9 ++++++++-
> >>  drivers/ide/ide-cd.c     |    4 ----
> >>  drivers/ide/ide-floppy.c |    4 ----
> >>  3 files changed, 8 insertions(+), 9 deletions(-)
> >>
> 
> ..
> 
> >> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> >> index 091d414..106cacb 100644
> >> --- a/drivers/ide/ide-cd.c
> >> +++ b/drivers/ide/ide-cd.c
> >> @@ -916,10 +916,6 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
> >>
> >>       cmd.rq = rq;
> >>
> >> -     ide_init_sg_cmd(&cmd,
> >> -             blk_fs_request(rq) ? (rq->nr_sectors << 9) : rq->data_len);
> >> -     ide_map_sg(drive, &cmd);
> >> -
> >>       return ide_issue_pc(drive, &cmd);
> >>  out_end:
> >>       nsectors = rq->hard_nr_sectors;
> >> diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
> >> index 2f8f453..b91deef 100644
> >> --- a/drivers/ide/ide-floppy.c
> >> +++ b/drivers/ide/ide-floppy.c
> >> @@ -281,10 +281,6 @@ static ide_startstop_t ide_floppy_do_request(ide_drive_t *drive,
> >>               cmd.tf_flags |= IDE_TFLAG_WRITE;
> >>
> >>       cmd.rq = rq;
> >> -
> >> -     ide_init_sg_cmd(&cmd, pc->req_xfer);
> >
> > There was a reason for using ->req_xfer.
> 
> Yeah, this is also one of those painful places where I cringe everytime I have
> to look at. We finally have to sit down and decide which is which; sometimes
> pc->req_xfer _is_ rq->data_len (idefloppy_blockpc_cmd()) and sometimes obviously

This is exactly my point -- these things are tricky so it is very risky to do
drive-by cleanups together with fixes.

> not. I'll take a look at that whole mess next and try to come up with something
> more clean.

Ok.

> > I don't see where rq->data_len is set for REQ_TYPE_SPECIAL requests
> > (ide_queue_pc_tail() and friends)?
> 
> me neither :(.
> 
> >> -     ide_map_sg(drive, &cmd);
> >> -
> >>       pc->rq = rq;
> >>
> >>       return ide_floppy_issue_pc(drive, &cmd, pc);
> 
> Man, this sucks!

:)

Re: [PATCH 3/3] ide-{cd,floppy}: do not map all cmds to an sg

[Borislav Petkov]

Hi,

> Unrelated to the original issue that the patch tries to fix
> (sg mapping no data commands which OOPS-es w/ DEBUG_VIRTUAL=y):
> - moving ide_init_sg_cmd()+ide_map_sg() to ide_issue_pc()
> - converting the code to use blk_rq_bytes()

aah, ok, now I could get exactly what you meant :). However, (!) ide-floppy is
potentially broken in the same way for data-less cmds since we unconditionally
map an rq to an sg there _too_. That was  the reason for the other changes. And
yes, I should've been more clear in the changelog.

> [ Moreover if you actually cared to write down the proper changelog
>  I'm pretty sure that you would ask yourself whether these changes
>  really belong to a bugfix patch... ]

-- 
Regards/Gruss,
Boris

Re: [PATCH 3/3] ide-{cd,floppy}: do not map all cmds to an sg

[Bartlomiej Zolnierkiewicz]

On Thursday 05 March 2009, Borislav Petkov wrote:
> Hi,
> 
> > Unrelated to the original issue that the patch tries to fix
> > (sg mapping no data commands which OOPS-es w/ DEBUG_VIRTUAL=y):
> > - moving ide_init_sg_cmd()+ide_map_sg() to ide_issue_pc()
> > - converting the code to use blk_rq_bytes()
> 
> aah, ok, now I could get exactly what you meant :). However, (!) ide-floppy is
> potentially broken in the same way for data-less cmds since we unconditionally

Indeed, you are right but it seems that this is not a new problem.

Wait... it makes things "worse" as there should have been 3 patches:
(IOW _three_ patchpoints! ;):

- ide-cd fix for problematic patch
- ide-floppy fix for mainline
- ide-atapi cleanup for pata tree

Thanks,
Bart

Re: [PATCH 3/3] ide-{cd,floppy}: do not map all cmds to an sg

[Borislav Petkov]

On Thu, Mar 5, 2009 at 5:52 PM, Bartlomiej Zolnierkiewicz
<bzolnier@gmail.com> wrote:
> On Thursday 05 March 2009, Borislav Petkov wrote:
>> Hi,
>>
>> > Unrelated to the original issue that the patch tries to fix
>> > (sg mapping no data commands which OOPS-es w/ DEBUG_VIRTUAL=y):
>> > - moving ide_init_sg_cmd()+ide_map_sg() to ide_issue_pc()
>> > - converting the code to use blk_rq_bytes()
>>
>> aah, ok, now I could get exactly what you meant :). However, (!) ide-floppy is
>> potentially broken in the same way for data-less cmds since we unconditionally
>
> Indeed, you are right but it seems that this is not a new problem.

Sure, as I said before, nobody's using ide-floppy.

> Wait... it makes things "worse" as there should have been 3 patches:
> (IOW _three_ patchpoints! ;):
>
> - ide-cd fix for problematic patch
> - ide-floppy fix for mainline
> - ide-atapi cleanup for pata tree

Yeah, I get the hint. :).

-- 
Regards/Gruss,
Boris

Re: [PATCH 3/3] ide-{cd,floppy}: do not map all cmds to an sg

[Borislav Petkov]

On Thu, Mar 05, 2009 at 05:52:41PM +0100, Bartlomiej Zolnierkiewicz wrote:
> On Thursday 05 March 2009, Borislav Petkov wrote:
> > Hi,
> > 
> > > Unrelated to the original issue that the patch tries to fix
> > > (sg mapping no data commands which OOPS-es w/ DEBUG_VIRTUAL=y):
> > > - moving ide_init_sg_cmd()+ide_map_sg() to ide_issue_pc()
> > > - converting the code to use blk_rq_bytes()
> > 
> > aah, ok, now I could get exactly what you meant :). However, (!) ide-floppy is
> > potentially broken in the same way for data-less cmds since we unconditionally
> 
> Indeed, you are right but it seems that this is not a new problem.
> 
> Wait... it makes things "worse" as there should have been 3 patches:
> (IOW _three_ patchpoints! ;):
> 
> - ide-cd fix for problematic patch

see below

> - ide-floppy fix for mainline

ide-floppy is still royally broken when reading the partition
information - I've been staring at traces for a while yesterday and
it looks pretty hairy. What is more, it needs that same fix for
DEBUG_VIRTUAL as ide-cd since I could reproduce it yesterday. Do you
still want that for the merge window or you wanna wait until its
properly fixed?

> - ide-atapi cleanup for pata tree

postponed for now.

--
From: Borislav Petkov <petkovbb@gmail.com>
Date: Fri, 6 Mar 2009 09:35:54 +0100
Subject: [PATCH] ide-cd: do not map dataless cmds to an sg

since it oopses on the virt_to_page() translation check when
DEBUG_VIRTUAL is enabled.

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-cd.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 091d414..35729a4 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -916,9 +916,11 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
 
 	cmd.rq = rq;
 
-	ide_init_sg_cmd(&cmd,
-		blk_fs_request(rq) ? (rq->nr_sectors << 9) : rq->data_len);
-	ide_map_sg(drive, &cmd);
+	if (blk_fs_request(rq) || rq->data_len) {
+		ide_init_sg_cmd(&cmd, blk_fs_request(rq) ? (rq->nr_sectors << 9)
+							 : rq->data_len);
+		ide_map_sg(drive, &cmd);
+	}
 
 	return ide_issue_pc(drive, &cmd);
 out_end:
-- 
1.6.1.3


-- 
Regards/Gruss,
    Boris.

Re: [PATCH 3/3] ide-{cd,floppy}: do not map all cmds to an sg

[Bartlomiej Zolnierkiewicz]

On Sunday 08 March 2009, Borislav Petkov wrote:

[...]

> > Wait... it makes things "worse" as there should have been 3 patches:
> > (IOW _three_ patchpoints! ;):
> > 
> > - ide-cd fix for problematic patch
> 
> see below

Ok, I folded it into the original patch (attached).

> > - ide-floppy fix for mainline
> 
> ide-floppy is still royally broken when reading the partition
> information - I've been staring at traces for a while yesterday and
> it looks pretty hairy. What is more, it needs that same fix for
> DEBUG_VIRTUAL as ide-cd since I could reproduce it yesterday. Do you
> still want that for the merge window or you wanna wait until its
> properly fixed?

If the mainline is broken sg fix can wait but to be honest I don't see much
point in delaying it (it is an independent problem and the bugfix should be
a completely safe one-liner).

> > - ide-atapi cleanup for pata tree
> 
> postponed for now.

Ok...

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] ide-cd: use scatterlists for PIO transfers (non-fs requests) (v2)

Convert ide-cd to use scatterlists for PIO transfers and get rid of
partial completions (except on error) also for non-fs requests.

v2:
Do not map dataless commands to an sg since it oopses on the virt_to_page()
translation check when DEBUG_VIRTUAL is enabled.  (from Borislav Petkov,
reported/bisected-by Tetsuo Handa).

Cc: Borislav Petkov <petkovbb@gmail.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
replacement patch for the one in pata tree / linux-next

 drivers/ide/ide-cd.c |  100 +++++++++++++++------------------------------------
 1 file changed, 30 insertions(+), 70 deletions(-)

Index: b/drivers/ide/ide-cd.c
===================================================================
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -509,8 +509,10 @@ static int ide_cd_check_ireason(ide_driv
 	return -1;
 }
 
-static void ide_cd_request_sense_fixup(ide_drive_t *drive, struct request *rq)
+static void ide_cd_request_sense_fixup(ide_drive_t *drive, struct ide_cmd *cmd)
 {
+	struct request *rq = cmd->rq;
+
 	ide_debug_log(IDE_DBG_FUNC, "rq->cmd[0]: 0x%x", rq->cmd[0]);
 
 	/*
@@ -518,11 +520,14 @@ static void ide_cd_request_sense_fixup(i
 	 * and some drives don't send them.  Sigh.
 	 */
 	if (rq->cmd[0] == GPCMD_REQUEST_SENSE &&
-	    rq->data_len > 0 && rq->data_len <= 5)
-		while (rq->data_len > 0) {
-			*(u8 *)rq->data++ = 0;
-			--rq->data_len;
+	    cmd->nleft > 0 && cmd->nleft <= 5) {
+		unsigned int ofs = cmd->nbytes - cmd->nleft;
+
+		while (cmd->nleft > 0) {
+			*((u8 *)rq->data + ofs++) = 0;
+			cmd->nleft--;
 		}
+	}
 }
 
 int ide_cd_queue_pc(ide_drive_t *drive, const unsigned char *cmd,
@@ -613,22 +618,11 @@ static void ide_cd_error_cmd(ide_drive_t
 		ide_complete_rq(drive, 0, nr_bytes);
 }
 
-/*
- * Called from blk_end_request_callback() after the data of the request is
- * completed and before the request itself is completed. By returning value '1',
- * blk_end_request_callback() returns immediately without completing it.
- */
-static int cdrom_newpc_intr_dummy_cb(struct request *rq)
-{
-	return 1;
-}
-
 static ide_startstop_t cdrom_newpc_intr(ide_drive_t *drive)
 {
 	ide_hwif_t *hwif = drive->hwif;
 	struct ide_cmd *cmd = &hwif->cmd;
 	struct request *rq = hwif->rq;
-	xfer_func_t *xferfunc;
 	ide_expiry_t *expiry = NULL;
 	int dma_error = 0, dma, stat, thislen, uptodate = 0;
 	int write = (rq_data_dir(rq) == WRITE) ? 1 : 0, rc, nsectors;
@@ -678,7 +672,7 @@ static ide_startstop_t cdrom_newpc_intr(
 
 	ide_read_bcount_and_ireason(drive, &len, &ireason);
 
-	thislen = blk_fs_request(rq) ? len : rq->data_len;
+	thislen = blk_fs_request(rq) ? len : cmd->nleft;
 	if (thislen > len)
 		thislen = len;
 
@@ -702,9 +696,9 @@ static ide_startstop_t cdrom_newpc_intr(
 				uptodate = 0;
 			}
 		} else if (!blk_pc_request(rq)) {
-			ide_cd_request_sense_fixup(drive, rq);
+			ide_cd_request_sense_fixup(drive, cmd);
 			/* complain if we still have data left to transfer */
-			uptodate = rq->data_len ? 0 : 1;
+			uptodate = cmd->nleft ? 0 : 1;
 			if (uptodate == 0)
 				rq->cmd_flags |= REQ_FAILED;
 		}
@@ -718,35 +712,15 @@ static ide_startstop_t cdrom_newpc_intr(
 
 	cmd->last_xfer_len = 0;
 
-	if (ireason == 0) {
-		write = 1;
-		xferfunc = hwif->tp_ops->output_data;
-	} else {
-		write = 0;
-		xferfunc = hwif->tp_ops->input_data;
-	}
-
 	ide_debug_log(IDE_DBG_PC, "data transfer, rq->cmd_type: 0x%x, "
 				  "ireason: 0x%x",
 				  rq->cmd_type, ireason);
 
 	/* transfer data */
 	while (thislen > 0) {
-		u8 *ptr = blk_fs_request(rq) ? NULL : rq->data;
-		int blen = rq->data_len;
-
-		/* bio backed? */
-		if (rq->bio) {
-			if (blk_fs_request(rq)) {
-				blen = min_t(int, thislen, cmd->nleft);
-			} else {
-				ptr = bio_data(rq->bio);
-				blen = bio_iovec(rq->bio)->bv_len;
-			}
-		}
+		int blen = min_t(int, thislen, cmd->nleft);
 
-		if ((blk_fs_request(rq) && cmd->nleft == 0) ||
-		    (blk_fs_request(rq) == 0 && ptr == NULL)) {
+		if (cmd->nleft == 0) {
 			if (blk_fs_request(rq) && !write)
 				/*
 				 * If the buffers are full, pipe the rest into
@@ -763,33 +737,12 @@ static ide_startstop_t cdrom_newpc_intr(
 			break;
 		}
 
-		if (blen > thislen)
-			blen = thislen;
-
-		if (blk_fs_request(rq)) {
-			ide_pio_bytes(drive, cmd, write, blen);
-			cmd->last_xfer_len += blen;
-		} else
-			xferfunc(drive, NULL, ptr, blen);
+		ide_pio_bytes(drive, cmd, write, blen);
+		cmd->last_xfer_len += blen;
 
 		thislen -= blen;
 		len -= blen;
 
-		if (blk_fs_request(rq) == 0) {
-			rq->data_len -= blen;
-
-			/*
-			 * The request can't be completed until DRQ is cleared.
-			 * So complete the data, but don't complete the request
-			 * using the dummy function for the callback feature
-			 * of blk_end_request_callback().
-			 */
-			if (rq->bio)
-				blk_end_request_callback(rq, 0, blen,
-						 cdrom_newpc_intr_dummy_cb);
-			else
-				rq->data += blen;
-		}
 		if (sense && write == 0)
 			rq->sense_len += blen;
 	}
@@ -814,8 +767,7 @@ out_end:
 	if (blk_pc_request(rq) && rc == 0) {
 		unsigned int dlen = rq->data_len;
 
-		if (dma)
-			rq->data_len = 0;
+		rq->data_len = 0;
 
 		if (blk_end_request(rq, 0, dlen))
 			BUG();
@@ -828,13 +780,14 @@ out_end:
 		if (blk_fs_request(rq)) {
 			if (cmd->nleft == 0)
 				uptodate = 1;
-			if (uptodate == 0)
-				ide_cd_error_cmd(drive, cmd);
 		} else {
 			if (uptodate <= 0 && rq->errors == 0)
 				rq->errors = -EIO;
 		}
 
+		if (uptodate == 0)
+			ide_cd_error_cmd(drive, cmd);
+
 		/* make sure it's fully ended */
 		if (blk_pc_request(rq))
 			nsectors = (rq->data_len + 511) >> 9;
@@ -844,6 +797,12 @@ out_end:
 		if (nsectors == 0)
 			nsectors = 1;
 
+		if (blk_fs_request(rq) == 0) {
+			rq->data_len -= (cmd->nbytes - cmd->nleft);
+			if (uptodate == 0 && (cmd->tf_flags & IDE_TFLAG_WRITE))
+				rq->data_len += cmd->last_xfer_len;
+		}
+
 		ide_complete_rq(drive, uptodate ? 0 : -EIO, nsectors << 9);
 
 		if (sense && rc == 2)
@@ -971,8 +930,9 @@ static ide_startstop_t ide_cd_do_request
 
 	cmd.rq = rq;
 
-	if (blk_fs_request(rq)) {
-		ide_init_sg_cmd(&cmd, rq->nr_sectors << 9);
+	if (blk_fs_request(rq) || rq->data_len) {
+		ide_init_sg_cmd(&cmd, blk_fs_request(rq) ? (rq->nr_sectors << 9)
+							 : rq->data_len);
 		ide_map_sg(drive, &cmd);
 	}
 

Re: [PATCH 3/3] ide-{cd,floppy}: do not map all cmds to an sg

[Borislav Petkov]

Hi,

> If the mainline is broken sg fix can wait but to be honest I don't see much
> point in delaying it (it is an independent problem and the bugfix should be
> a completely safe one-liner).

--
From: Borislav Petkov <petkovbb@gmail.com>
Date: Tue, 10 Mar 2009 07:04:52 +0100
Subject: [PATCH] ide-floppy: do not map dataless cmds to an sg

since it fails the virt_to_page() translation check with DEBUG_VIRTUAL
enabled.

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-atapi.c  |   12 ++++++++++++
 drivers/ide/ide-floppy.c |    6 ++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index a5596a6..11a680c 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -90,6 +90,12 @@ static void ide_queue_pc_head(ide_drive_t *drive, struct gendisk *disk,
 	rq->cmd_flags |= REQ_PREEMPT;
 	rq->buffer = (char *)pc;
 	rq->rq_disk = disk;
+
+	if (pc->req_xfer) {
+		rq->data = pc->buf;
+		rq->data_len = pc->req_xfer;
+	}
+
 	memcpy(rq->cmd, pc->c, 12);
 	if (drive->media == ide_tape)
 		rq->cmd[13] = REQ_IDETAPE_PC1;
@@ -112,6 +118,12 @@ int ide_queue_pc_tail(ide_drive_t *drive, struct gendisk *disk,
 	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
 	rq->cmd_type = REQ_TYPE_SPECIAL;
 	rq->buffer = (char *)pc;
+
+	if (pc->req_xfer) {
+		rq->data = pc->buf;
+		rq->data_len = pc->req_xfer;
+	}
+
 	memcpy(rq->cmd, pc->c, 12);
 	if (drive->media == ide_tape)
 		rq->cmd[13] = REQ_IDETAPE_PC1;
diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index 2f8f453..2b4868d 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -282,8 +282,10 @@ static ide_startstop_t ide_floppy_do_request(ide_drive_t *drive,
 
 	cmd.rq = rq;
 
-	ide_init_sg_cmd(&cmd, pc->req_xfer);
-	ide_map_sg(drive, &cmd);
+	if (blk_fs_request(rq) || pc->req_xfer) {
+		ide_init_sg_cmd(&cmd, pc->req_xfer);
+		ide_map_sg(drive, &cmd);
+	}
 
 	pc->rq = rq;
 
-- 
1.6.2


> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Subject: [PATCH] ide-cd: use scatterlists for PIO transfers (non-fs requests) (v2)
> 
> Convert ide-cd to use scatterlists for PIO transfers and get rid of
> partial completions (except on error) also for non-fs requests.
> 
> v2:
> Do not map dataless commands to an sg since it oopses on the virt_to_page()
> translation check when DEBUG_VIRTUAL is enabled.  (from Borislav Petkov,
> reported/bisected-by Tetsuo Handa).
> 
> Cc: Borislav Petkov <petkovbb@gmail.com>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

Acked-by: Borislav Petkov <petkovbb@gmail.com>

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 3/3] ide-{cd,floppy}: do not map all cmds to an sg

[Bartlomiej Zolnierkiewicz]

On Tuesday 10 March 2009, Borislav Petkov wrote:
> Hi,
> 
> > If the mainline is broken sg fix can wait but to be honest I don't see much
> > point in delaying it (it is an independent problem and the bugfix should be
> > a completely safe one-liner).
> 
> --
> From: Borislav Petkov <petkovbb@gmail.com>
> Date: Tue, 10 Mar 2009 07:04:52 +0100
> Subject: [PATCH] ide-floppy: do not map dataless cmds to an sg
> 
> since it fails the virt_to_page() translation check with DEBUG_VIRTUAL
> enabled.
> 
> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>

I applied it with some changes:

> ---
>  drivers/ide/ide-atapi.c  |   12 ++++++++++++
>  drivers/ide/ide-floppy.c |    6 ++++--
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
> index a5596a6..11a680c 100644
> --- a/drivers/ide/ide-atapi.c
> +++ b/drivers/ide/ide-atapi.c
> @@ -90,6 +90,12 @@ static void ide_queue_pc_head(ide_drive_t *drive, struct gendisk *disk,
>  	rq->cmd_flags |= REQ_PREEMPT;
>  	rq->buffer = (char *)pc;
>  	rq->rq_disk = disk;
> +
> +	if (pc->req_xfer) {
> +		rq->data = pc->buf;
> +		rq->data_len = pc->req_xfer;
> +	}
> +
>  	memcpy(rq->cmd, pc->c, 12);
>  	if (drive->media == ide_tape)
>  		rq->cmd[13] = REQ_IDETAPE_PC1;
> @@ -112,6 +118,12 @@ int ide_queue_pc_tail(ide_drive_t *drive, struct gendisk *disk,
>  	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
>  	rq->cmd_type = REQ_TYPE_SPECIAL;
>  	rq->buffer = (char *)pc;
> +
> +	if (pc->req_xfer) {
> +		rq->data = pc->buf;
> +		rq->data_len = pc->req_xfer;
> +	}
> +
>  	memcpy(rq->cmd, pc->c, 12);
>  	if (drive->media == ide_tape)
>  		rq->cmd[13] = REQ_IDETAPE_PC1;

ide-atapi.c part doesn't seem to be needed for fixing the issue
so I removed it (IMO it would fit much better with your sg setup
cleanup patch than this one)

> diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
> index 2f8f453..2b4868d 100644
> --- a/drivers/ide/ide-floppy.c
> +++ b/drivers/ide/ide-floppy.c
> @@ -282,8 +282,10 @@ static ide_startstop_t ide_floppy_do_request(ide_drive_t *drive,
>  
>  	cmd.rq = rq;
>  
> -	ide_init_sg_cmd(&cmd, pc->req_xfer);
> -	ide_map_sg(drive, &cmd);
> +	if (blk_fs_request(rq) || pc->req_xfer) {

ditto for blk_fs_request(rq) check

> +		ide_init_sg_cmd(&cmd, pc->req_xfer);
> +		ide_map_sg(drive, &cmd);
> +	}
>  
>  	pc->rq = rq;

Thanks,
Bart

Re: [PATCH 3/3] ide-{cd,floppy}: do not map all cmds to an sg

[Borislav Petkov]

On Wed, Mar 11, 2009 at 05:34:28PM +0100, Bartlomiej Zolnierkiewicz wrote:
> On Tuesday 10 March 2009, Borislav Petkov wrote:
> > Hi,
> > 
> > > If the mainline is broken sg fix can wait but to be honest I don't see much
> > > point in delaying it (it is an independent problem and the bugfix should be
> > > a completely safe one-liner).
> > 
> > --
> > From: Borislav Petkov <petkovbb@gmail.com>
> > Date: Tue, 10 Mar 2009 07:04:52 +0100
> > Subject: [PATCH] ide-floppy: do not map dataless cmds to an sg
> > 
> > since it fails the virt_to_page() translation check with DEBUG_VIRTUAL
> > enabled.
> > 
> > Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
> 
> I applied it with some changes:
> 
> > ---
> >  drivers/ide/ide-atapi.c  |   12 ++++++++++++
> >  drivers/ide/ide-floppy.c |    6 ++++--
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
> > index a5596a6..11a680c 100644
> > --- a/drivers/ide/ide-atapi.c
> > +++ b/drivers/ide/ide-atapi.c
> > @@ -90,6 +90,12 @@ static void ide_queue_pc_head(ide_drive_t *drive, struct gendisk *disk,
> >  	rq->cmd_flags |= REQ_PREEMPT;
> >  	rq->buffer = (char *)pc;
> >  	rq->rq_disk = disk;
> > +
> > +	if (pc->req_xfer) {
> > +		rq->data = pc->buf;
> > +		rq->data_len = pc->req_xfer;
> > +	}
> > +
> >  	memcpy(rq->cmd, pc->c, 12);
> >  	if (drive->media == ide_tape)
> >  		rq->cmd[13] = REQ_IDETAPE_PC1;
> > @@ -112,6 +118,12 @@ int ide_queue_pc_tail(ide_drive_t *drive, struct gendisk *disk,
> >  	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
> >  	rq->cmd_type = REQ_TYPE_SPECIAL;
> >  	rq->buffer = (char *)pc;
> > +
> > +	if (pc->req_xfer) {
> > +		rq->data = pc->buf;
> > +		rq->data_len = pc->req_xfer;
> > +	}
> > +
> >  	memcpy(rq->cmd, pc->c, 12);
> >  	if (drive->media == ide_tape)
> >  		rq->cmd[13] = REQ_IDETAPE_PC1;
> 
> ide-atapi.c part doesn't seem to be needed for fixing the issue
> so I removed it (IMO it would fit much better with your sg setup
> cleanup patch than this one)

No, you need that part. And especially the rq->data assignment.
Take a look at ide_floppy_get_capacity() - it calls into
ide_queue_pc_tail() with pc->req_xfer == 255 resulting from the
ide_floppy_create_read_capacity_cmd(). However, the rq->data is still
NULL if you'd remove the chunk I added and you get

[    6.317953] ------------[ cut here ]------------
[    6.318011] kernel BUG at arch/x86/mm/ioremap.c:80!
[    6.318699] invalid opcode: 0000 [#1] PREEMPT SMP 
[    6.318895] last sysfs file: 
[    6.318895] Modules linked in:
[    6.318895] 
[    6.318895] Pid: 1, comm: swapper Not tainted (2.6.29-rc7 #27) P4I45PE    1.00
[    6.318895] EIP: 0060:[<c0115608>] EFLAGS: 00010213 CPU: 0
[    6.318895] EIP is at __phys_addr+0xc/0x41
[    6.318895] EAX: 00000000 EBX: c1000000 ECX: 00000001 EDX: 00000000
[    6.318895] ESI: df882000 EDI: 00000000 EBP: df82faa4 ESP: df82faa4
[    6.318895]  DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
[    6.318895] Process swapper (pid: 1, ti=df82e000 task=df830000 task.ti=df82e000)
[    6.318895] Stack:
[    6.318895]  df82fabc c01fcfe3 00000000 df882000 df82faec df82fb2c df82facc c0256b61
[    6.320031]  df9aba00 df82faec df82fb38 c0260e10 df8f6000 dfa636c0 df8f6000 df82fe0c
[    6.320031]  c01fa4e3 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    6.321008] Call Trace:
[    6.321008]  [<c01fcfe3>] ? sg_init_one+0x27/0x70
[    6.321008]  [<c0256b61>] ? ide_map_sg+0x3f/0x59
[    6.321008]  [<c0260e10>] ? ide_floppy_do_request+0x2a4/0x3a0
[    6.321008]  [<c01fa4e3>] ? delay_tsc+0x6d/0x86
[    6.321008]  [<c025f2fc>] ? ide_gd_do_request+0xa/0xd
[    6.321008]  [<c0257031>] ? do_ide_request+0x326/0x4a5
[    6.321008]  [<c032b9de>] ? _spin_lock_irqsave+0x1b/0x21
[    6.322015]  [<c012977d>] ? lock_timer_base+0x1f/0x3e
[    6.322015]  [<c032b8bf>] ? _spin_unlock_irqrestore+0x17/0x2c
[    6.322015]  [<c012984a>] ? del_timer+0x47/0x4e
[    6.322015]  [<c01ee8d8>] ? blk_start_queueing+0x18/0x23
[    6.322015]  [<c01ecb71>] ? elv_insert+0x6e/0x18b
[    6.322015]  [<c012995f>] ? mod_timer+0x21/0x27
[    6.322015]  [<c01ecd0f>] ? __elv_add_request+0x81/0x86
[    6.322015]  [<c01f0f9c>] ? blk_execute_rq_nowait+0x58/0x7e
[    6.323007]  [<c01f105c>] ? blk_execute_rq+0x9a/0xba
[    6.323007]  [<c01f0f1c>] ? blk_end_sync_rq+0x0/0x28
[    6.323007]  [<c025c31c>] ? ide_queue_pc_tail+0x5a/0x6d
[    6.323007]  [<c0260f81>] ? ide_floppy_get_capacity+0x75/0x47e
[    6.323007]  [<c01f9ac6>] ? vsnprintf+0x3a6/0x8c5
[    6.323007]  [<c0329da9>] ? schedule_timeout+0x16/0x9d
[    6.323007]  [<c01f5559>] ? idr_get_empty_slot+0x13c/0x1f0
[    6.323007]  [<c01f5559>] ? idr_get_empty_slot+0x13c/0x1f0
[    6.323007]  [<c01f56dd>] ? ida_get_new_above+0xd0/0x171
[    6.323007]  [<c032a116>] ? mutex_unlock+0x8/0xa
[    6.323007]  [<c01a0c4f>] ? sysfs_addrm_finish+0x16/0x1ca
[    6.323007]  [<c01a0977>] ? __sysfs_add_one+0x14/0x8e
[    6.323007]  [<c01a052c>] ? sysfs_add_file_mode+0x4e/0x6d
[    6.323007]  [<c026144a>] ? ide_floppy_setup+0x85/0x9b
[    6.323007]  [<c025f712>] ? ide_gd_probe+0x104/0x162
[    6.323007]  [<c0255bd1>] ? generic_ide_probe+0x1f/0x21
[    6.323007]  [<c024e743>] ? driver_probe_device+0x9c/0x12f
[    6.325013]  [<c024e822>] ? __driver_attach+0x4c/0x6b
[    6.325013]  [<c024e177>] ? bus_for_each_dev+0x3b/0x63
[    6.325013]  [<c024e5ea>] ? driver_attach+0x14/0x16
[    6.325013]  [<c024e7d6>] ? __driver_attach+0x0/0x6b
[    6.325013]  [<c024dba8>] ? bus_add_driver+0xa0/0x1bd
[    6.325013]  [<c024e9cb>] ? driver_register+0x81/0xe1
[    6.325013]  [<c048d35b>] ? ide_gd_init+0x17/0x19
[    6.325013]  [<c0101132>] ? _stext+0x4a/0x10c
[    6.325013]  [<c048d344>] ? ide_gd_init+0x0/0x19
[    6.325013]  [<c0147f0d>] ? register_irq_proc+0x7f/0x9b
[    6.325013]  [<c0147f7c>] ? init_irq_proc+0x53/0x60
[    6.325013]  [<c04772cb>] ? kernel_init+0x101/0x155
[    6.325013]  [<c04771ca>] ? kernel_init+0x0/0x155
[    6.325013]  [<c010370b>] ? kernel_thread_helper+0x7/0x10

If you don't want the changes to ide-atapi you can alternatively do
the following (checking the rq->data being NULL is simply paranoid
cautiousness on my side :)) :

diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index f7a3ea0..fc6648d 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -283,6 +283,9 @@ static ide_startstop_t ide_floppy_do_request(ide_drive_t *drive,
 	cmd.rq = rq;
 
 	if (pc->req_xfer) {
+		if (!rq->data)
+			rq->data = pc->buf;
+
 		ide_init_sg_cmd(&cmd, pc->req_xfer);
 		ide_map_sg(drive, &cmd);
 	}


> > diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
> > index 2f8f453..2b4868d 100644
> > --- a/drivers/ide/ide-floppy.c
> > +++ b/drivers/ide/ide-floppy.c
> > @@ -282,8 +282,10 @@ static ide_startstop_t ide_floppy_do_request(ide_drive_t *drive,
> >  
> >  	cmd.rq = rq;
> >  
> > -	ide_init_sg_cmd(&cmd, pc->req_xfer);
> > -	ide_map_sg(drive, &cmd);
> > +	if (blk_fs_request(rq) || pc->req_xfer) {
> 
> ditto for blk_fs_request(rq) check

Unfortunately I can't confirm that because of the other breakage. It
won't hurt if we'd left it in though.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 3/3] ide-{cd,floppy}: do not map all cmds to an sg

[Bartlomiej Zolnierkiewicz]

On Thursday 12 March 2009, Borislav Petkov wrote:
> On Wed, Mar 11, 2009 at 05:34:28PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > On Tuesday 10 March 2009, Borislav Petkov wrote:
> > > Hi,
> > > 
> > > > If the mainline is broken sg fix can wait but to be honest I don't see much
> > > > point in delaying it (it is an independent problem and the bugfix should be
> > > > a completely safe one-liner).
> > > 
> > > --
> > > From: Borislav Petkov <petkovbb@gmail.com>
> > > Date: Tue, 10 Mar 2009 07:04:52 +0100
> > > Subject: [PATCH] ide-floppy: do not map dataless cmds to an sg
> > > 
> > > since it fails the virt_to_page() translation check with DEBUG_VIRTUAL
> > > enabled.
> > > 
> > > Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
> > 
> > I applied it with some changes:
> > 
> > > ---
> > >  drivers/ide/ide-atapi.c  |   12 ++++++++++++
> > >  drivers/ide/ide-floppy.c |    6 ++++--
> > >  2 files changed, 16 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
> > > index a5596a6..11a680c 100644
> > > --- a/drivers/ide/ide-atapi.c
> > > +++ b/drivers/ide/ide-atapi.c
> > > @@ -90,6 +90,12 @@ static void ide_queue_pc_head(ide_drive_t *drive, struct gendisk *disk,
> > >  	rq->cmd_flags |= REQ_PREEMPT;
> > >  	rq->buffer = (char *)pc;
> > >  	rq->rq_disk = disk;
> > > +
> > > +	if (pc->req_xfer) {
> > > +		rq->data = pc->buf;
> > > +		rq->data_len = pc->req_xfer;
> > > +	}
> > > +
> > >  	memcpy(rq->cmd, pc->c, 12);
> > >  	if (drive->media == ide_tape)
> > >  		rq->cmd[13] = REQ_IDETAPE_PC1;
> > > @@ -112,6 +118,12 @@ int ide_queue_pc_tail(ide_drive_t *drive, struct gendisk *disk,
> > >  	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
> > >  	rq->cmd_type = REQ_TYPE_SPECIAL;
> > >  	rq->buffer = (char *)pc;
> > > +
> > > +	if (pc->req_xfer) {
> > > +		rq->data = pc->buf;
> > > +		rq->data_len = pc->req_xfer;
> > > +	}
> > > +
> > >  	memcpy(rq->cmd, pc->c, 12);
> > >  	if (drive->media == ide_tape)
> > >  		rq->cmd[13] = REQ_IDETAPE_PC1;
> > 
> > ide-atapi.c part doesn't seem to be needed for fixing the issue
> > so I removed it (IMO it would fit much better with your sg setup
> > cleanup patch than this one)
> 
> No, you need that part. And especially the rq->data assignment.
> Take a look at ide_floppy_get_capacity() - it calls into
> ide_queue_pc_tail() with pc->req_xfer == 255 resulting from the
> ide_floppy_create_read_capacity_cmd(). However, the rq->data is still
> NULL if you'd remove the chunk I added and you get

[...]

Ah I see it now but this is a separate issue from the original one
(OOPS on DEBUG_VIRTUAL=y)!  I somehow missed it (probably becasue
patch description fails to mention this important information)
and thus tried to be smart while merging the patch...

Anyway, I replaced the patch in pata tree by your original one.

Thanks,
Bart

Re: [PATCH 3/3] ide-{cd,floppy}: do not map all cmds to an sg

[Borislav Petkov]

Hi,

> Ah I see it now but this is a separate issue from the original one
> (OOPS on DEBUG_VIRTUAL=y)!

Hmm, no, this oopses on the same DEBUG_VIRTUAL check due to rq->data being NULL,
it is the same issue happening when entered from a different path:

> I somehow missed it (probably becasue
> patch description fails to mention this important information)
> and thus tried to be smart while merging the patch...

... and yes, we could add something along the lines of "Make sure ->data pointer
in sg mapping is set from all command issuing paths" to the commit message. My
bad, I'm being too economical wrt commit messages, as always :).

-- 
Regards/Gruss,
Boris