Hi Bart,

here are some generic ide conversion patches. The first 12 are what i thought
you suggested :) concerning prepping the ide-cd code for the generic layer. The
remaining 6 are what i've done so far wrt removing ide_atapi_pc from the ide
drivers. It is obvious that this is not trivial and I basically tiptoe around the
landmines in the IRQ handler and request issue paths :). This raises also
several questions:

1. ide-cd uses the cdrom_info struct for e.g. dma status notification, the other
ide drivers use it per packet command in pc->flags. Well the last are kinda too
much to carry for _each_ command and i'm thinking maybe put all that in the
ide_drive_t and make it generic enough to fit all drivers. This way
pc->callback() can also be harboured there. One concern might be when a flag is
strictly per packet command which could be put somewhere else (maybe in the
struct request since it already has members when it is used as a packet
command).

2. Can all that pc->xferred, pc->b_count, pc->errors and pc->cur_pos accounting
be safely mapped to a rq? I see some discrepancies like is pc->buf_size ==
rq->data_len, what about pc->req_xfer? I'll have a more detailed look at those
when i have more spare time later.

3. (I'm sure there are more :))
--

[ message continues ]

This is done in the request issue path anyway

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

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index d998471..92a8a36 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -517,14 +517,9 @@ static ide_startstop_t cdrom_start_packet_command(ide_drive_t *drive,
 						  int xferlen,
 						  ide_handler_t *handler)
 {
-	ide_startstop_t startstop;
 	struct cdrom_info *info = drive->driver_data;
 	ide_hwif_t *hwif = drive->hwif;
 
-	/* wait for the controller to be idle */
-	if (ide_wait_stat(&startstop, drive, 0, BUSY_STAT, WAIT_READY))
-		return startstop;
-
 	/* FIXME: for Virtual DMA we must check harder */
 	if (info->dma)
 		info->dma = !hwif->dma_ops->dma_setup(drive);
-- 
1.5.5.1

--

[ message continues ]

... into cdrom_handle_failed_fs_req(). There's a slight change in internal
functionality in the case when we have sense NOT_READY and the request is WRITE: the
original function cdrom_decode_status returned 1 in this case, which means "request
was ended." We accomplish the same by returning 0 instead, which falls through in the
if..else block and the surrounding cdrom_decode_status() returns 1 at the end instead
of jumping to the end_request label as is in all the other cases, so no change
in obvious functionality.

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

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 16c4ce9..e63eea2 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -314,12 +314,115 @@ static int cdrom_handle_failed_pc_req(ide_drive_t *drive, struct request *rq,
 	return 1;
 }
 
-
 /*
- * Returns:
- * 0: if the request should be continued.
- * 1: if the request was ended.
+ * Handle errors from READ and WRITE requests. Do a request sense analysis when
+ * we have sense data. We need this in order to perform end of media processing.
  */
+static int cdrom_handle_failed_fs_req(ide_drive_t *drive, struct request *rq,
+				      int sense_key, int stat)
+{
+	int err = 0;
+
+	if (blk_noretry_request(rq))
+		err = 1;
+
+	switch (sense_key) {
+	case NOT_READY:
+		/* tray open */
+		if (rq_data_dir(rq) == READ) {
+			cdrom_saw_media_change(drive);
+
+			/* fail the request */
+			printk(KERN_ERR "%s: tray open\n", drive->name);
+			err = 1;
+		} else {
+			struct cdrom_info *info = drive->driver_data;
+
+			/*
+			 * Allow the drive 5 seconds to recover, some devices
+			 * will return this error while flushing data from
+			 * cache.
+			 */
+			if (!rq->errors)
+				info->write_timeout = jiffies +
+						      ATAPI_WAIT_WRITE_BUSY;
+			rq->errors = 1;
+
+			if ...
[ message continues ]

There should be no functionality change resulting from this patch.

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

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 21c4510..40dab2b 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -778,7 +778,7 @@ static ide_startstop_t cdrom_start_seek_continuation(ide_drive_t *drive)
 	return cdrom_transfer_packet_command(drive, rq, &cdrom_seek_intr);
 }
 
-static ide_startstop_t cdrom_start_seek(ide_drive_t *drive, unsigned int block)
+static ide_startstop_t cdrom_start_seek(ide_drive_t *drive)
 {
 	struct cdrom_info *info = drive->driver_data;
 
@@ -1237,7 +1237,7 @@ static ide_startstop_t ide_do_rw_cdrom(ide_drive_t *drive, struct request *rq,
 		    IDE_LARGE_SEEK(info->last_block, block,
 				   IDECD_SEEK_THRESHOLD) &&
 		    drive->dsc_overlap)
-			action = cdrom_start_seek(drive, block);
+			action = cdrom_start_seek(drive);
 		else
 			action = cdrom_start_rw(drive, rq);
 		info->last_block = block;
-- 
1.5.5.1

--

[ message continues ]

... into cdrom_handle_failed_pc_req().

There should be no functionality change resulting from this patch.

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

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 2f0c9d4..16c4ce9 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -273,6 +273,48 @@ static void ide_dump_status_no_sense(ide_drive_t *drive, const char *msg, u8 st)
 	ide_dump_status(drive, msg, st);
 }
 
+/* All other functions, except for READ. */
+static int cdrom_handle_failed_pc_req(ide_drive_t *drive, struct request *rq,
+				      int sense_key, int stat)
+{
+	/*
+	 * if we have an error, pass back CHECK_CONDITION as the scsi status
+	 * byte
+	 */
+	if (blk_pc_request(rq) && !rq->errors)
+		rq->errors = SAM_STAT_CHECK_CONDITION;
+
+	/* check for tray open */
+	if (sense_key == NOT_READY) {
+		cdrom_saw_media_change(drive);
+	} else if (sense_key == UNIT_ATTENTION) {
+		/* check for media change */
+		cdrom_saw_media_change(drive);
+		return 0;
+	} else if (sense_key == ILLEGAL_REQUEST &&
+			rq->cmd[0] == GPCMD_START_STOP_UNIT) {
+
+		/*
+		 * Don't print error message for this condition - SFF8090i
+		 * indicates that 5/24/00 is the correct response to a request
+		 * to close the tray if the drive doesn't have that capability.
+		 * cdrom_log_sense() knows this!
+		 */
+	} else if (!(rq->cmd_flags & REQ_QUIET)) {
+		/* otherwise, print an error */
+		ide_dump_status(drive, "packet command error", stat);
+	}
+
+	rq->cmd_flags |= REQ_FAILED;
+
+	/* instead of playing games with moving completions around, remove
+	 * failed request completely and end it when the request sense has
+	 * completed.
+	 */
+	return 1;
+}
+
+
 /*
  * Returns:
  * 0: if the request should be continued.
@@ -314,44 +356,11 @@ static int cdrom_decode_status(ide_drive_t *drive, int good_stat, int ...
[ message continues ]

I actually think that we should try to unite pc/fs error handling as much
as possible as it shouldn't really matter if i.e. READ command came through
fs or pc request - the error handling w.r.t. hardware should be the same
(at the moment it is not always the case - the most blatant example of this
disrepancy is handling of NOT_READY sense key for WRITE commands).

When I was suggesting factoring out error handling I rather meant moving
out _everything_ after OK_STAT() (sorry for the confusion).  On the second
thought we may do it in even simpler way by moving:

...
	/* check for errors */
	stat = ide_read_status(drive);

	if (stat_ret)
		*stat_ret = stat;

	if (OK_STAT(stat, good_stat, BAD_R_STAT))
		return 0;
...

to cdrom_decode_status() users and passing as an argument 'stat' instead
of 'good_stat' and 'stat_ret'.

Therefore I skipped this patch (and also patch #4) for now.
--

[ message continues ]

Hi Bart,

here's a first attempt at that. These are only RFC of nature, please take a look
when there's time. They've been lightly tested but I'll do more later since this
path is not hit that often and a special test case will be required.

---
From: Borislav Petkov <petkovbb@gmail.com>
Date: Sun, 10 Aug 2008 18:54:03 +0200
Subject: [PATCH 1/2] ide-cd: move error handling outside of cdrom_decode_status()

There should be no functionality change resulting from this patch.

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

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index f675cee..f2c12be 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -284,20 +284,11 @@ static void ide_dump_status_no_sense(ide_drive_t *drive, const char *msg, u8 st)
  * 0: if the request should be continued.
  * 1: if the request was ended.
  */
-static int cdrom_decode_status(ide_drive_t *drive, int good_stat, int *stat_ret)
+static int cdrom_decode_status(ide_drive_t *drive, int stat)
 {
 	ide_hwif_t *hwif = drive->hwif;
 	struct request *rq = hwif->hwgroup->rq;
-	int stat, err, sense_key;
-
-	/* check for errors */
-	stat = hwif->tp_ops->read_status(hwif);
-
-	if (stat_ret)
-		*stat_ret = stat;
-
-	if (OK_STAT(stat, good_stat, BAD_R_STAT))
-		return 0;
+	int err, sense_key;
 
 	/* get the IDE error register */
 	err = ide_read_error(drive);
@@ -563,7 +554,7 @@ static ide_startstop_t cdrom_transfer_packet_command(ide_drive_t *drive,
 					  ide_handler_t *handler)
 {
 	ide_hwif_t *hwif = drive->hwif;
-	int cmd_len;
+	int cmd_len, stat;
 	struct cdrom_info *info = drive->driver_data;
 	ide_startstop_t startstop;
 
@@ -574,8 +565,11 @@ static ide_startstop_t cdrom_transfer_packet_command(ide_drive_t *drive,
 		 */
 
 		/* check for errors */
-		if (cdrom_decode_status(drive, ATA_DRQ, NULL))
-			return ide_stopped;
+		stat = ...
[ message continues ]

Yep, this one has problems as I just noticed. Reworking... 
--

[ message continues ]

There should be no functionality change resulting from this patch.

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

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 40dab2b..e9d9363 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -1211,7 +1211,7 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
 /*
  * cdrom driver request routine.
  */
-static ide_startstop_t ide_do_rw_cdrom(ide_drive_t *drive, struct request *rq,
+static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
 					sector_t block)
 {
 	ide_startstop_t action;
@@ -1949,7 +1949,7 @@ static ide_driver_t ide_cdrom_driver = {
 	.version		= IDECD_VERSION,
 	.media			= ide_cdrom,
 	.supports_dsc_overlap	= 1,
-	.do_request		= ide_do_rw_cdrom,
+	.do_request		= ide_cd_do_request,
 	.end_request		= ide_end_request,
 	.error			= __ide_error,
 	.abort			= __ide_abort,
-- 
1.5.5.1

--

[ message continues ]

There should be no functionality change resulting from this patch.

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

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index e63eea2..21c4510 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -1249,11 +1249,11 @@ static ide_startstop_t ide_do_rw_cdrom(ide_drive_t *drive, struct request *rq,
 		/* right now this can only be a reset... */
 		cdrom_end_request(drive, 1);
 		return ide_stopped;
+	} else {
+		blk_dump_rq_flags(rq, "ide-cd bad flags");
+		cdrom_end_request(drive, 0);
+		return ide_stopped;
 	}
-
-	blk_dump_rq_flags(rq, "ide-cd bad flags");
-	cdrom_end_request(drive, 0);
-	return ide_stopped;
 }
 
 
-- 
1.5.5.1

--

[ message continues ]

The pc is still embedded in rq->buffer.

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

diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index 8ce7513..1ec4951 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -464,12 +464,12 @@ static void ide_floppy_report_error(idefloppy_floppy_t *floppy,
 }
 
 static ide_startstop_t idefloppy_issue_pc(ide_drive_t *drive,
-		struct ide_atapi_pc *pc)
+					  struct request *rq)
 {
 	idefloppy_floppy_t *floppy = drive->driver_data;
+	struct ide_atapi_pc *pc = (struct ide_atapi_pc *) rq->buffer;
 
-	if (floppy->failed_pc == NULL &&
-	    pc->rq->cmd[0] != GPCMD_REQUEST_SENSE)
+	if (floppy->failed_pc == NULL && rq->cmd[0] != GPCMD_REQUEST_SENSE)
 		floppy->failed_pc = pc;
 	/* Set the current packet command */
 	floppy->pc = pc;
@@ -626,6 +626,7 @@ static ide_startstop_t idefloppy_do_request(ide_drive_t *drive,
 		}
 		pc = idefloppy_next_pc_storage(drive);
 		idefloppy_create_rw_cmd(floppy, pc, rq, block);
+		rq->buffer = (char *) pc;
 	} else if (blk_pc_request(rq))
 		pc = (struct ide_atapi_pc *) rq->buffer;
 	else {
@@ -643,7 +644,7 @@ static ide_startstop_t idefloppy_do_request(ide_drive_t *drive,
 
 	pc->rq = rq;
 
-	return idefloppy_issue_pc(drive, pc);
+	return idefloppy_issue_pc(drive, rq);
 }
 
 /*
-- 
1.5.5.1

--

[ message continues ]

This is a precaution just to make sure a new pc is clean when allocd. There
should be functional change introduced by this patch.

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

diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index 615d3a3..8ce7513 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -355,9 +355,8 @@ static void idefloppy_init_pc(struct ide_atapi_pc *pc, unsigned char *cmd)
 	if (cmd)
 		memset(cmd, 0, 12);
 
-	pc->retries = 0;
-	pc->flags = 0;
-	pc->req_xfer = 0;
+	memset(pc, 0, sizeof(*pc));
+
 	pc->buf = pc->pc_buf;
 	pc->buf_size = IDEFLOPPY_PC_BUFFER_SIZE;
 	pc->callback = ide_floppy_callback;
-- 
1.5.5.1

--

[ message continues ]

Pass pc->flags to the IRQ handler from the drivers (ide-tape/floppy/scsi) thus
making it more pc-unaware.

There should be no functionality change resulting from this.

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-atapi.c  |   25 +++++++++++++------------
 drivers/ide/ide-floppy.c |    3 ++-
 drivers/ide/ide-tape.c   |    3 ++-
 drivers/scsi/ide-scsi.c  |    2 +-
 include/linux/ide.h      |    2 +-
 5 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 3a6ae79..dfe1c55 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -19,7 +19,8 @@ ide_startstop_t ide_pc_intr(ide_drive_t *drive, struct ide_atapi_pc *pc,
 	ide_handler_t *handler, unsigned int timeout, ide_expiry_t *expiry,
 	void (*update_buffers)(ide_drive_t *, struct ide_atapi_pc *),
 	void (*retry_pc)(ide_drive_t *), void (*dsc_handle)(ide_drive_t *),
-	void (*io_buffers)(ide_drive_t *, struct ide_atapi_pc *, unsigned, int))
+	void (*io_buffers)(ide_drive_t *, struct ide_atapi_pc *, unsigned, int),
+	unsigned long *flags)
 {
 	ide_hwif_t *hwif = drive->hwif;
 	xfer_func_t *xferfunc;
@@ -35,7 +36,7 @@ ide_startstop_t ide_pc_intr(ide_drive_t *drive, struct ide_atapi_pc *pc,
 
 	debug_log("Enter %s - interrupt handler\n", __func__);
 
-	if (pc->flags & PC_FLAG_TIMEDOUT) {
+	if (*flags & PC_FLAG_TIMEDOUT) {
 		pc->callback(drive);
 		return ide_stopped;
 	}
@@ -43,14 +44,14 @@ ide_startstop_t ide_pc_intr(ide_drive_t *drive, struct ide_atapi_pc *pc,
 	/* Clear the interrupt */
 	stat = ide_read_status(drive);
 
-	if (pc->flags & PC_FLAG_DMA_IN_PROGRESS) {
+	if (*flags & PC_FLAG_DMA_IN_PROGRESS) {
 		if (hwif->dma_ops->dma_end(drive) ||
 		    (drive->media == ide_tape && !scsi && (stat & ERR_STAT))) {
 			if (drive->media == ide_floppy && !scsi)
 				printk(KERN_ERR "%s: DMA %s error\n",
 					drive->name, rq_data_dir(pc->rq)
 						     ? "write" : "read");
-			pc->flags |= ...
[ message continues ]

This moves the problem around without improving the overall situation
w.r.t. pc->flags (now ATAPI drivers are more pc-aware instead).

It also adds one more argument to the already overloaded ide_pc_intr()
(my fault, I went a bit overboard but I added a nice TODO explanation! ;).

Since there are better ideas on how to deal with pc->flags (you've even
described them yourself in the first mail :) I skipped this patch for now.
--

[ message continues ]

There should be no functionality change introduced by this patch.

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

diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index 1ec4951..8bdef60 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -576,8 +576,7 @@ static void idefloppy_create_rw_cmd(idefloppy_floppy_t *floppy,
 	int blocks = rq->nr_sectors / floppy->bs_factor;
 	int rw_dir = rq_data_dir(rq);
 
-	debug_log("create_rw10_cmd: block == %d, blocks == %d\n",
-		  block, blocks);
+	debug_log("%s: block == %d, blocks == %d\n", __func__, block, blocks);
 
 	idefloppy_init_pc(pc, NULL);
 	rq->cmd[0] = rw_dir == READ ? GPCMD_READ_10 : GPCMD_WRITE_10;
@@ -586,8 +585,10 @@ static void idefloppy_create_rw_cmd(idefloppy_floppy_t *floppy,
 
 	pc->rq = rq;
 	pc->b_count = rw_dir == READ ? 0 : rq->bio->bi_size;
-	if (rq->cmd_flags & REQ_RW)
+
+	if (rw_dir)
 		pc->flags |= PC_FLAG_WRITING;
+
 	pc->buf = NULL;
 	pc->req_xfer = pc->buf_size = blocks * floppy->block_size;
 	pc->flags |= PC_FLAG_DMA_OK;
-- 
1.5.5.1

--

[ message continues ]

There's no need for this function since it is used only once.

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

diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index ffebf83..615d3a3 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -569,13 +569,6 @@ static void idefloppy_create_start_stop_cmd(struct ide_atapi_pc *pc, int start,
 	cmd[4] = start;
 }
 
-static void idefloppy_create_test_unit_ready_cmd(struct ide_atapi_pc *pc,
-						 unsigned char *cmd)
-{
-	idefloppy_init_pc(pc, cmd);
-	cmd[0] = GPCMD_TEST_UNIT_READY;
-}
-
 static void idefloppy_create_rw_cmd(idefloppy_floppy_t *floppy,
 				    struct ide_atapi_pc *pc, struct request *rq,
 				    unsigned long sector)
@@ -1161,7 +1154,9 @@ static int idefloppy_open(struct inode *inode, struct file *filp)
 		floppy->flags &= ~IDEFLOPPY_FLAG_FORMAT_IN_PROGRESS;
 		/* Just in case */
 
-		idefloppy_create_test_unit_ready_cmd(&pc, cmd);
+		idefloppy_init_pc(&pc, cmd);
+		cmd[0] = GPCMD_TEST_UNIT_READY;
+
 		if (idefloppy_queue_pc_tail(drive, &pc, cmd)) {
 			idefloppy_create_start_stop_cmd(&pc, 1, cmd);
 			(void) idefloppy_queue_pc_tail(drive, &pc, cmd);
-- 
1.5.5.1

--

[ message continues ]

... by factoring out the rq preparation code into a separate
function called in the request routine. Bart: As a nice
side effect, this minimizes the IRQ handler execution time.

There should be no functionality change resulting from this patch.

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

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index b8c98e1..da0f28c 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -763,9 +763,8 @@ static ide_startstop_t cdrom_seek_intr(ide_drive_t *drive)
 	return ide_stopped;
 }
 
-static ide_startstop_t cdrom_start_seek_continuation(ide_drive_t *drive)
+static void ide_cd_prepare_seek_request(ide_drive_t *drive, struct request *rq)
 {
-	struct request *rq = HWGROUP(drive)->rq;
 	sector_t frame = rq->sector;
 
 	sector_div(frame, queue_hardsect_size(drive->queue) >> SECTOR_BITS);
@@ -775,6 +774,11 @@ static ide_startstop_t cdrom_start_seek_continuation(ide_drive_t *drive)
 	put_unaligned(cpu_to_be32(frame), (unsigned int *) &rq->cmd[2]);
 
 	rq->timeout = ATAPI_WAIT_PC;
+}
+
+static ide_startstop_t cdrom_start_seek_continuation(ide_drive_t *drive)
+{
+	struct request *rq = HWGROUP(drive)->rq;
 	return cdrom_transfer_packet_command(drive, rq, &cdrom_seek_intr);
 }
 
@@ -1223,10 +1227,14 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
 		    IDE_LARGE_SEEK(info->last_block, block,
 			    IDECD_SEEK_THRESHOLD) &&
 		    drive->dsc_overlap) {
+
 			xferlen = 0;
 			fn = cdrom_start_seek_continuation;
 			info->dma = 0;
 			info->start_seek = jiffies;
+
+			ide_cd_prepare_seek_request(drive, rq);
+
 		} else {
 			xferlen = 32768;
 			fn = cdrom_start_rw_cont;
-- 
1.5.5.1

--

[ message continues ]

call cdrom_start_packet_command() only from the do_request()
routine. As a nice side effect, this improves code readability a bit.

There should be no functionality change resulting from this patch.

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

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index e9d9363..abf9af2 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -778,14 +778,12 @@ static ide_startstop_t cdrom_start_seek_continuation(ide_drive_t *drive)
 	return cdrom_transfer_packet_command(drive, rq, &cdrom_seek_intr);
 }
 
-static ide_startstop_t cdrom_start_seek(ide_drive_t *drive)
+static void cdrom_start_seek(ide_drive_t *drive)
 {
 	struct cdrom_info *info = drive->driver_data;
 
 	info->dma = 0;
 	info->start_seek = jiffies;
-	return cdrom_start_packet_command(drive, 0,
-					  cdrom_start_seek_continuation);
 }
 
 /*
@@ -1160,8 +1158,7 @@ static ide_startstop_t cdrom_start_rw(ide_drive_t *drive, struct request *rq)
 	if (write)
 		cd->devinfo.media_written = 1;
 
-	/* start sending the read/write request to the drive */
-	return cdrom_start_packet_command(drive, 32768, cdrom_start_rw_cont);
+	return ide_started;
 }
 
 static ide_startstop_t cdrom_do_newpc_cont(ide_drive_t *drive)
@@ -1174,7 +1171,7 @@ static ide_startstop_t cdrom_do_newpc_cont(ide_drive_t *drive)
 	return cdrom_transfer_packet_command(drive, rq, cdrom_newpc_intr);
 }
 
-static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
+static void cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
 {
 	struct cdrom_info *info = drive->driver_data;
 
@@ -1202,10 +1199,6 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
 		if ((rq->data_len & 15) || (addr & mask))
 			info->dma = 0;
 	}
-
-	/* start sending the command to the drive */
-	return ...
[ message continues ]

Use the generic ide_pad_transfer() helper instead

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

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 92a8a36..2f0c9d4 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -599,28 +599,6 @@ static ide_startstop_t cdrom_transfer_packet_command(ide_drive_t *drive,
 }
 
 /*
- * Block read functions.
- */
-static void ide_cd_pad_transfer(ide_drive_t *drive, xfer_func_t *xf, int len)
-{
-	while (len > 0) {
-		int dum = 0;
-		xf(drive, NULL, &dum, sizeof(dum));
-		len -= sizeof(dum);
-	}
-}
-
-static void ide_cd_drain_data(ide_drive_t *drive, int nsects)
-{
-	while (nsects > 0) {
-		static char dum[SECTOR_SIZE];
-
-		drive->hwif->input_data(drive, NULL, dum, sizeof(dum));
-		nsects--;
-	}
-}
-
-/*
  * Check the contents of the interrupt reason register from the cdrom
  * and attempt to recover if there are problems.  Returns  0 if everything's
  * ok; nonzero if the request has been terminated.
@@ -635,15 +613,12 @@ static int ide_cd_check_ireason(ide_drive_t *drive, struct request *rq,
 	if (ireason == (!rw << 1))
 		return 0;
 	else if (ireason == (rw << 1)) {
-		ide_hwif_t *hwif = drive->hwif;
-		xfer_func_t *xf;
 
 		/* whoops... */
 		printk(KERN_ERR "%s: %s: wrong transfer direction!\n",
 				drive->name, __func__);
 
-		xf = rw ? hwif->output_data : hwif->input_data;
-		ide_cd_pad_transfer(drive, xf, len);
+		ide_pad_transfer(drive, rw, len);
 	} else  if (rw == 0 && ireason == 1) {
 		/*
 		 * Some drives (ASUS) seem to tell us that status info is
@@ -1006,7 +981,7 @@ static ide_startstop_t cdrom_newpc_intr(ide_drive_t *drive)
 					   - bio_cur_sectors(rq->bio),
 					   thislen >> 9);
 			if (nskip > 0) {
-				ide_cd_drain_data(drive, nskip);
+				ide_pad_transfer(drive, write, nskip);
 				rq->current_nr_sectors -= nskip;
 				thislen -= (nskip << ...
[ message continues ]

applied w/ ide_cd_drain_data() -> ide_pad_transfer() conversion fixup


ide_cd_drain_data() took number for _sectors_ as an argument

ditto
--

[ message continues ]

-- 
Regards/Gruß,
    Boris.
--

[ message continues ]

... by factoring out the rq preparation code into a separate
function called in the request routine. Bart: As a nice
side effect, this minimizes the IRQ handler execution time.

There should be no functionality change resulting from this patch.

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

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index da0f28c..a5715b1 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -691,16 +691,9 @@ static int ide_cd_check_transfer_size(ide_drive_t *drive, int len)
 
 static ide_startstop_t cdrom_newpc_intr(ide_drive_t *);
 
-/*
- * Routine to send a read/write packet command to the drive. This is usually
- * called directly from cdrom_start_{read,write}(). However, for drq_interrupt
- * devices, it is called from an interrupt when the drive is ready to accept
- * the command.
- */
-static ide_startstop_t cdrom_start_rw_cont(ide_drive_t *drive)
+static ide_startstop_t ide_cd_prepare_rw_request(ide_drive_t *drive,
+						 struct request *rq)
 {
-	struct request *rq = HWGROUP(drive)->rq;
-
 	if (rq_data_dir(rq) == READ) {
 		unsigned short sectors_per_frame =
 			queue_hardsect_size(drive->queue) >> SECTOR_BITS;
@@ -737,6 +730,19 @@ static ide_startstop_t cdrom_start_rw_cont(ide_drive_t *drive)
 	/* set up the command */
 	rq->timeout = ATAPI_WAIT_PC;
 
+	return ide_started;
+}
+
+/*
+ * Routine to send a read/write packet command to the drive. This is usually
+ * called directly from cdrom_start_{read,write}(). However, for drq_interrupt
+ * devices, it is called from an interrupt when the drive is ready to accept
+ * the command.
+ */
+static ide_startstop_t cdrom_start_rw_cont(ide_drive_t *drive)
+{
+	struct request *rq = HWGROUP(drive)->rq;
+
 	/* send the command to the drive and return */
 	return cdrom_transfer_packet_command(drive, rq, cdrom_newpc_intr);
 }
@@ -1238,10 +1244,15 @@ ...
[ message continues ]

This is the first one in the series attempting to phase out ide_atapi_pc and use
block request structs. Also, the pc request type is now REQ_TYPE_BLOCK_PC. As a result,
idefloppy_blockpc_cmd becomes unused and can go.

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

---
 drivers/ide/ide-atapi.c  |   22 ++++-
 drivers/ide/ide-floppy.c |  207 +++++++++++++++++++++++-----------------------
 2 files changed, 120 insertions(+), 109 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 2802031..3a6ae79 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -26,6 +26,12 @@ ide_startstop_t ide_pc_intr(ide_drive_t *drive, struct ide_atapi_pc *pc,
 	unsigned int temp;
 	u16 bcount;
 	u8 stat, ireason, scsi = drive->scsi;
+	unsigned char *cmd;
+
+	if (drive->media == ide_floppy)
+		cmd = pc->rq->cmd;
+	else
+		cmd = pc->c;
 
 	debug_log("Enter %s - interrupt handler\n", __func__);
 
@@ -63,7 +69,7 @@ ide_startstop_t ide_pc_intr(ide_drive_t *drive, struct ide_atapi_pc *pc,
 		local_irq_enable_in_hardirq();
 
 		if (drive->media == ide_tape && !scsi &&
-		    (stat & ERR_STAT) && pc->c[0] == REQUEST_SENSE)
+		    (stat & ERR_STAT) && cmd[0] == REQUEST_SENSE)
 			stat &= ~ERR_STAT;
 		if ((stat & ERR_STAT) || (pc->flags & PC_FLAG_DMA_ERROR)) {
 			/* Error detected */
@@ -75,13 +81,13 @@ ide_startstop_t ide_pc_intr(ide_drive_t *drive, struct ide_atapi_pc *pc,
 					goto cmd_finished;
 			}
 
-			if (pc->c[0] == REQUEST_SENSE) {
+			if (cmd[0] == REQUEST_SENSE) {
 				printk(KERN_ERR "%s: I/O error in request sense"
 						" command\n", drive->name);
 				return ide_do_reset(drive);
 			}
 
-			debug_log("[cmd %x]: check condition\n", pc->c[0]);
+			debug_log("[cmd %x]: check condition\n", cmd[0]);
 
 			/* Retry operation */
 			retry_pc(drive);
@@ -175,7 +181,7 @@ cmd_finished:
 	pc->cur_pos += bcount;
 
 	debug_log("[cmd %x] transferred %d bytes on that intr.\n",
-		  pc->c[0], bcount);
+		  cmd[0], bcount);
 
 ...
[ message continues ]

Do not attack too many dragons at once or they will slay you... ;)

IOW this patch mixes too many changes (some _really_ non-trivial)
in one go which results in rather bad outcome...


REQUEST SENSE command is switched from REQ_TYPE_SPECIAL to REQ_TYPE_BLOCK_PC
which should belong to a separate patch and is premature IMHO (the other
ATAPI drivers seem to still use REQ_TYPE_SPECIAL so probably it makes sense

This actually seems to break REQUEST SENSE handling



Also the above changes seem to break handling of blk_pc_request() requests
(i.e. SG_IO ioctl) because they will be no longer initialized properly.

OTOH the main change (pc->c to rq->cmd) looks OK (only minor complaint is
that 'u8' is both shorter and more readable than 'unsigned short')...

[ I had to also skip patches #16-17 because of skipping this patch. ]
--

[ message continues ]

yeah, this was more of a RFC patch for me not intended in any way for submission
but instead to get your comments on it. Atapi-land is full of surprises and you
have to be _really_ careful when changing stuff around. I got bitten by that
couple times when preparing those patches. Anyways, don't take those seriously,
they were simply a warmup :).

Thanks,
Boris.
--

[ message continues ]

I'm not worried at all - I'm sure that if you keep fighting these dragons
one by one they won't stand a chance... :)
--

[ message continues ]

Bart: As a nice side effect, this minimizes the IRQ handler execution time.

There should be no functionality change resulting from this patch.

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

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index a5715b1..0cd23d4 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -1167,9 +1167,6 @@ static ide_startstop_t cdrom_do_newpc_cont(ide_drive_t *drive)
 {
 	struct request *rq = HWGROUP(drive)->rq;
 
-	if (!rq->timeout)
-		rq->timeout = ATAPI_WAIT_PC;
-
 	return cdrom_transfer_packet_command(drive, rq, cdrom_newpc_intr);
 }
 
@@ -1253,11 +1250,18 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
 		}
 		info->last_block = block;
 
-	} else if (blk_sense_request(rq) || blk_pc_request(rq) ||
+	} else if (blk_sense_request(rq) ||
+		   blk_pc_request(rq) ||
 		   rq->cmd_type == REQ_TYPE_ATA_PC) {
+
 		xferlen = rq->data_len;
 		fn = cdrom_do_newpc_cont;
+
+		if (!rq->timeout)
+			rq->timeout = ATAPI_WAIT_PC;
+
 		cdrom_do_block_pc(drive, rq);
+
 	} else if (blk_special_request(rq)) {
 		/* right now this can only be a reset... */
 		cdrom_end_request(drive, 1);
@@ -1271,8 +1275,6 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
 	return cdrom_start_packet_command(drive, xferlen, fn);
 }
 
-
-
 /*
  * Ioctl handling.
  *
-- 
1.5.5.1

--

[ message continues ]

Do what the compiler does anyway: inline a function that is used only once. This
saves us the overhead of a function call and the function is small enough to be
embedded in the callsite anyways.

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

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index abf9af2..b8c98e1 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -778,14 +778,6 @@ static ide_startstop_t cdrom_start_seek_continuation(ide_drive_t *drive)
 	return cdrom_transfer_packet_command(drive, rq, &cdrom_seek_intr);
 }
 
-static void cdrom_start_seek(ide_drive_t *drive)
-{
-	struct cdrom_info *info = drive->driver_data;
-
-	info->dma = 0;
-	info->start_seek = jiffies;
-}
-
 /*
  * Fix up a possibly partially-processed request so that we can start it over
  * entirely, or even put it back on the request queue.
@@ -1233,7 +1225,8 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
 		    drive->dsc_overlap) {
 			xferlen = 0;
 			fn = cdrom_start_seek_continuation;
-			cdrom_start_seek(drive);
+			info->dma = 0;
+			info->start_seek = jiffies;
 		} else {
 			xferlen = 32768;
 			fn = cdrom_start_rw_cont;
-- 
1.5.5.1

--

[ message continues ]

Hi,


Thanks.

I applied patches #1-2, #5-12 and #14-15.

I skipped patches #3-4, #13 and #16-18 for now

Some pc->flags describe device's properties and thus should be moved to
ide_drive_t (->dev_flags) while some other correspond to the queued command
and moving them to ide_drive_t would be a bad idea IMO.

For ATA commands I was planning to put taskfile flags into rq->special field
(well, I actually implemented it in draft patch and it looks OK) so maybe we

If you ask if they can be mapped 'directly' then the answer is: "probably no"
but if the question is whether it is possible to do it after some changes then
the answer is: "probably yes". :)

[ However it may be necessary to convert ATAPI drivers to use scatterlists
  instead of open-coded ->bio walking for PIO transfers first. ]

Thanks,
Bart
--

[ message continues ]

On Sat, Jun 14, 2008 at 07:29:00PM +0200, Bartlomiej Zolnierkiewicz wrote:


With "the last" i meant pc->flags and not "per packet" so i completely agree:

Yep, i was looking for a field to put all those per-command flags into so

Looking into it right now...

Thanks.

-- 
Regards/Gruß,
    Boris.
--

[ message continues ]