[PATCH 0/8] ide-cd: first conversion batch

[PATCH 0/8] ide-cd: first conversion batch

[Borislav Petkov]

Hi Bart,

here's the first batch of conversion patches. Now ide_issue_pc/ide_transfer_pc
are almost completely aligned to their ide-cd counterparts
cdrom_start_packet_command/cdrom_transfer_packet_command. The only thing
missing is the setting of the irq handler through ide_set_handler but this
is not that trivial since if i'd moved ide-cd's irq handler to ide-atapi
that would suck in a bunch of other ide-cd functions which is not what we
want, (yes/no)? and I'm going to have to ponder a little bit more on that.

Those have been lightly tested with ide-cd and ide-floppy.


 drivers/ide/ide-atapi.c  |  115 ++++++++++++++++----------------------------
 drivers/ide/ide-cd.c     |  121 ++++++++++++++++++++++++++++------------------
 drivers/ide/ide-floppy.c |    2 +-
 drivers/ide/ide-tape.c   |    2 +-
 include/linux/ide.h      |    2 +-
 5 files changed, 117 insertions(+), 125 deletions(-)

[PATCH 1/8] ide-atapi: compute cmd_len based on device type in ide_transfer_pc

[Borislav Petkov]

There should be no functionality change resulting from this patch.

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

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index d412bd2..5c143b4 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -15,6 +15,8 @@
 #define debug_log(fmt, args...) do {} while (0)
 #endif
 
+#define ATAPI_MIN_CDB_BYTES	12
+
 static inline int dev_is_idecd(ide_drive_t *drive)
 {
 	return drive->media == ide_cdrom || drive->media == ide_optical;
@@ -492,6 +494,7 @@ static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
 	struct request *rq = hwif->hwgroup->rq;
 	ide_expiry_t *expiry;
 	unsigned int timeout;
+	int cmd_len;
 	ide_startstop_t startstop;
 	u8 ireason;
 
@@ -506,6 +509,14 @@ static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
 			drive->waiting_for_dma = 1;
 	}
 
+	if (dev_is_idecd(drive)) {
+		/* ATAPI commands get padded out to 12 bytes minimum */
+		cmd_len = COMMAND_SIZE(rq->cmd[0]);
+		if (cmd_len < ATAPI_MIN_CDB_BYTES)
+			cmd_len = ATAPI_MIN_CDB_BYTES;
+	} else
+		cmd_len = ATAPI_MIN_CDB_BYTES;
+
 	ireason = ide_read_ireason(drive);
 	if (drive->media == ide_tape)
 		ireason = ide_wait_ireason(drive, ireason);
@@ -513,6 +524,7 @@ static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
 	if ((ireason & ATAPI_COD) == 0 || (ireason & ATAPI_IO)) {
 		printk(KERN_ERR "%s: (IO,CoD) != (0,1) while issuing "
 				"a packet command\n", drive->name);
+
 		return ide_do_reset(drive);
 	}
 
@@ -541,7 +553,7 @@ static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
 
 	/* Send the actual packet */
 	if ((drive->atapi_flags & IDE_AFLAG_ZIP_DRIVE) == 0)
-		hwif->tp_ops->output_data(drive, NULL, rq->cmd, 12);
+		hwif->tp_ops->output_data(drive, NULL, rq->cmd, cmd_len);
 
 	return ide_started;
 }
-- 
1.6.0.4

[PATCH 2/8] ide-atapi: assign expiry and timeout based on device type

[Borislav Petkov]

There should be no functionality change resulting from this patch.

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

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 5c143b4..5295b26 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -514,9 +514,28 @@ static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
 		cmd_len = COMMAND_SIZE(rq->cmd[0]);
 		if (cmd_len < ATAPI_MIN_CDB_BYTES)
 			cmd_len = ATAPI_MIN_CDB_BYTES;
-	} else
+
+		timeout = rq->timeout;
+		expiry  = ide_cd_expiry;
+
+	} else {
 		cmd_len = ATAPI_MIN_CDB_BYTES;
 
+		/*
+		 * If necessary schedule the packet transfer to occur 'timeout'
+		 * miliseconds later in ide_delayed_transfer_pc() after the
+		 * device says it's ready for a packet.
+		 */
+		if (drive->atapi_flags & IDE_AFLAG_ZIP_DRIVE) {
+			timeout = drive->pc_delay;
+			expiry = &ide_delayed_transfer_pc;
+		} else {
+			timeout = (drive->media == ide_floppy) ? WAIT_FLOPPY_CMD
+							       : WAIT_TAPE_CMD;
+			expiry = NULL;
+		}
+	}
+
 	ireason = ide_read_ireason(drive);
 	if (drive->media == ide_tape)
 		ireason = ide_wait_ireason(drive, ireason);
@@ -528,20 +547,6 @@ static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
 		return ide_do_reset(drive);
 	}
 
-	/*
-	 * If necessary schedule the packet transfer to occur 'timeout'
-	 * miliseconds later in ide_delayed_transfer_pc() after the device
-	 * says it's ready for a packet.
-	 */
-	if (drive->atapi_flags & IDE_AFLAG_ZIP_DRIVE) {
-		timeout = drive->pc_delay;
-		expiry = &ide_delayed_transfer_pc;
-	} else {
-		timeout = (drive->media == ide_floppy) ? WAIT_FLOPPY_CMD
-						       : WAIT_TAPE_CMD;
-		expiry = NULL;
-	}
-
 	/* Set the interrupt routine */
 	ide_set_handler(drive, ide_pc_intr, timeout, expiry);
 
-- 
1.6.0.4

Re: [PATCH 2/8] ide-atapi: assign expiry and timeout based on device type

[Bartlomiej Zolnierkiewicz]

On Thursday 18 December 2008, Borislav Petkov wrote:
> There should be no functionality change resulting from this patch.
> 
> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
> ---
>  drivers/ide/ide-atapi.c |   35 ++++++++++++++++++++---------------
>  1 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
> index 5c143b4..5295b26 100644
> --- a/drivers/ide/ide-atapi.c
> +++ b/drivers/ide/ide-atapi.c
> @@ -514,9 +514,28 @@ static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
>  		cmd_len = COMMAND_SIZE(rq->cmd[0]);
>  		if (cmd_len < ATAPI_MIN_CDB_BYTES)
>  			cmd_len = ATAPI_MIN_CDB_BYTES;
> -	} else
> +
> +		timeout = rq->timeout;
> +		expiry  = ide_cd_expiry;
> +
> +	} else {
>  		cmd_len = ATAPI_MIN_CDB_BYTES;
>  
> +		/*
> +		 * If necessary schedule the packet transfer to occur 'timeout'
> +		 * miliseconds later in ide_delayed_transfer_pc() after the
> +		 * device says it's ready for a packet.
> +		 */
> +		if (drive->atapi_flags & IDE_AFLAG_ZIP_DRIVE) {
> +			timeout = drive->pc_delay;
> +			expiry = &ide_delayed_transfer_pc;
> +		} else {
> +			timeout = (drive->media == ide_floppy) ? WAIT_FLOPPY_CMD
> +							       : WAIT_TAPE_CMD;
> +			expiry = NULL;
> +		}
> +	}
> +
>  	ireason = ide_read_ireason(drive);
>  	if (drive->media == ide_tape)
>  		ireason = ide_wait_ireason(drive, ireason);
> @@ -528,20 +547,6 @@ static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
>  		return ide_do_reset(drive);
>  	}

If we add dev_is_idecd() check here in patch #1 it won't be necessary to
needlessly move the code around in this patch and it would also result in
smaller resulting code (42 bytes saved on x86-32)...

[ I updated patches 1-2 accordingly while merging them. ]

> -	/*
> -	 * If necessary schedule the packet transfer to occur 'timeout'
> -	 * miliseconds later in ide_delayed_transfer_pc() after the device
> -	 * says it's ready for a packet.
> -	 */
> -	if (drive->atapi_flags & IDE_AFLAG_ZIP_DRIVE) {
> -		timeout = drive->pc_delay;
> -		expiry = &ide_delayed_transfer_pc;
> -	} else {
> -		timeout = (drive->media == ide_floppy) ? WAIT_FLOPPY_CMD
> -						       : WAIT_TAPE_CMD;
> -		expiry = NULL;
> -	}
> -
>  	/* Set the interrupt routine */
>  	ide_set_handler(drive, ide_pc_intr, timeout, expiry);

[PATCH 3/8] ide-atapi: split drive-specific functionality in ide_issue_pc

[Borislav Petkov]

There should be no functionality change resulting from this patch.

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

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 5295b26..2577782 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -565,39 +565,43 @@ static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
 
 ide_startstop_t ide_issue_pc(ide_drive_t *drive, unsigned int timeout)
 {
-	struct ide_atapi_pc *pc = drive->pc;
+	struct ide_atapi_pc *pc;
 	ide_hwif_t *hwif = drive->hwif;
 	ide_expiry_t *expiry = NULL;
 	u32 tf_flags;
 	u16 bcount;
 
-	/* We haven't transferred any data yet */
-	pc->xferred = 0;
-	pc->cur_pos = pc->buf;
-
 	if (dev_is_idecd(drive)) {
 		tf_flags = IDE_TFLAG_OUT_NSECT | IDE_TFLAG_OUT_LBAL;
 		bcount = ide_cd_get_xferlen(hwif->hwgroup->rq);
 		expiry = ide_cd_expiry;
+
+		if (drive->dma)
+			drive->dma = !hwif->dma_ops->dma_setup(drive);
 	} else {
+		pc = drive->pc;
+
+		/* We haven't transferred any data yet */
+		pc->xferred = 0;
+		pc->cur_pos = pc->buf;
+
 		tf_flags = IDE_TFLAG_OUT_DEVICE;
 		bcount = ((drive->media == ide_tape) ?
 				pc->req_xfer :
 				min(pc->req_xfer, 63 * 1024));
-	}
 
-	if (pc->flags & PC_FLAG_DMA_ERROR) {
-		pc->flags &= ~PC_FLAG_DMA_ERROR;
-		ide_dma_off(drive);
-	}
+		if (pc->flags & PC_FLAG_DMA_ERROR) {
+			pc->flags &= ~PC_FLAG_DMA_ERROR;
+			ide_dma_off(drive);
+		}
 
-	if (((pc->flags & PC_FLAG_DMA_OK) &&
-		(drive->dev_flags & IDE_DFLAG_USING_DMA)) ||
-	    drive->dma)
-		drive->dma = !hwif->dma_ops->dma_setup(drive);
+		if ((pc->flags & PC_FLAG_DMA_OK) &&
+		     (drive->dev_flags & IDE_DFLAG_USING_DMA))
+			drive->dma = !hwif->dma_ops->dma_setup(drive);
 
-	if (!drive->dma)
-		pc->flags &= ~PC_FLAG_DMA_OK;
+		if (!drive->dma)
+			pc->flags &= ~PC_FLAG_DMA_OK;
+	}
 
 	ide_pktcmd_tf_load(drive, tf_flags, bcount, drive->dma);
 
-- 
1.6.0.4

[PATCH 4/8] ide-cd: remove xferlen arg to cdrom_start_packet_command

[Borislav Petkov]

There should be no functionality change resulting from this patch.

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

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 105e4d8..34981f5 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -520,10 +520,13 @@ end_request:
  * will be called immediately after the drive is prepared for the transfer.
  */
 static ide_startstop_t cdrom_start_packet_command(ide_drive_t *drive,
-						  int xferlen,
 						  ide_handler_t *handler)
 {
 	ide_hwif_t *hwif = drive->hwif;
+	struct request *rq = hwif->hwgroup->rq;
+	int xferlen;
+
+	xferlen = ide_cd_get_xferlen(rq);
 
 	ide_debug_log(IDE_DBG_PC, "Call %s, xferlen: %d\n", __func__, xferlen);
 
@@ -1175,15 +1178,12 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
 					sector_t block)
 {
 	ide_handler_t *fn;
-	int xferlen;
 
 	ide_debug_log(IDE_DBG_RQ, "Call %s, rq->cmd[0]: 0x%x, "
 		      "rq->cmd_type: 0x%x, block: %llu\n",
 		      __func__, rq->cmd[0], rq->cmd_type,
 		      (unsigned long long)block);
 
-	xferlen = ide_cd_get_xferlen(rq);
-
 	if (blk_fs_request(rq)) {
 		fn = cdrom_start_rw_cont;
 
@@ -1210,7 +1210,7 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
 		return ide_stopped;
 	}
 
-	return cdrom_start_packet_command(drive, xferlen, fn);
+	return cdrom_start_packet_command(drive, fn);
 }
 
 /*
-- 
1.6.0.4

[PATCH 5/8] ide-cd: remove handler wrappers

[Borislav Petkov]

Remove cdrom_do_newpc_cont and cdrom_start_rw_cont wrappers and pass
cdrom_transfer_packet_command to ide_execute_command directly.

There should be no functionality change resulting from this patch.

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

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 34981f5..f3c7cc3 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -511,48 +511,7 @@ end_request:
 	return 1;
 }
 
-/*
- * Set up the device registers for transferring a packet command on DEV,
- * expecting to later transfer XFERLEN bytes.  HANDLER is the routine
- * which actually transfers the command to the drive.  If this is a
- * drq_interrupt device, this routine will arrange for HANDLER to be
- * called when the interrupt from the drive arrives.  Otherwise, HANDLER
- * will be called immediately after the drive is prepared for the transfer.
- */
-static ide_startstop_t cdrom_start_packet_command(ide_drive_t *drive,
-						  ide_handler_t *handler)
-{
-	ide_hwif_t *hwif = drive->hwif;
-	struct request *rq = hwif->hwgroup->rq;
-	int xferlen;
-
-	xferlen = ide_cd_get_xferlen(rq);
-
-	ide_debug_log(IDE_DBG_PC, "Call %s, xferlen: %d\n", __func__, xferlen);
-
-	/* FIXME: for Virtual DMA we must check harder */
-	if (drive->dma)
-		drive->dma = !hwif->dma_ops->dma_setup(drive);
-
-	/* set up the controller registers */
-	ide_pktcmd_tf_load(drive, IDE_TFLAG_OUT_NSECT | IDE_TFLAG_OUT_LBAL,
-			   xferlen, drive->dma);
-
-	if (drive->atapi_flags & IDE_AFLAG_DRQ_INTERRUPT) {
-		/* waiting for CDB interrupt, not DMA yet. */
-		if (drive->dma)
-			drive->waiting_for_dma = 0;
-
-		/* packet command */
-		ide_execute_command(drive, ATA_CMD_PACKET, handler,
-				    ATAPI_WAIT_PC, ide_cd_expiry);
-		return ide_started;
-	} else {
-		ide_execute_pkt_cmd(drive);
-
-		return (*handler) (drive);
-	}
-}
+static ide_startstop_t cdrom_newpc_intr(ide_drive_t *);
 
 /*
  * Send a packet command to DRIVE described by CMD_BUF and CMD_LEN. The device
@@ -561,11 +520,10 @@ static ide_startstop_t cdrom_start_packet_command(ide_drive_t *drive,
  * there's data ready.
  */
 #define ATAPI_MIN_CDB_BYTES 12
-static ide_startstop_t cdrom_transfer_packet_command(ide_drive_t *drive,
-					  struct request *rq,
-					  ide_handler_t *handler)
+static ide_startstop_t cdrom_transfer_packet_command(ide_drive_t *drive)
 {
 	ide_hwif_t *hwif = drive->hwif;
+	struct request *rq = hwif->hwgroup->rq;
 	int cmd_len;
 	ide_startstop_t startstop;
 
@@ -592,7 +550,7 @@ static ide_startstop_t cdrom_transfer_packet_command(ide_drive_t *drive,
 	}
 
 	/* arm the interrupt handler */
-	ide_set_handler(drive, handler, rq->timeout, ide_cd_expiry);
+	ide_set_handler(drive, cdrom_newpc_intr, rq->timeout, ide_cd_expiry);
 
 	/* ATAPI commands get padded out to 12 bytes minimum */
 	cmd_len = COMMAND_SIZE(rq->cmd[0]);
@@ -610,6 +568,49 @@ static ide_startstop_t cdrom_transfer_packet_command(ide_drive_t *drive,
 }
 
 /*
+ * Set up the device registers for transferring a packet command on DEV,
+ * expecting to later transfer XFERLEN bytes.  HANDLER is the routine
+ * which actually transfers the command to the drive.  If this is a
+ * drq_interrupt device, this routine will arrange for HANDLER to be
+ * called when the interrupt from the drive arrives.  Otherwise, HANDLER
+ * will be called immediately after the drive is prepared for the transfer.
+ */
+static ide_startstop_t cdrom_start_packet_command(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	struct request *rq = hwif->hwgroup->rq;
+	int xferlen;
+
+	xferlen = ide_cd_get_xferlen(rq);
+
+	ide_debug_log(IDE_DBG_PC, "Call %s, xferlen: %d\n", __func__, xferlen);
+
+	/* FIXME: for Virtual DMA we must check harder */
+	if (drive->dma)
+		drive->dma = !hwif->dma_ops->dma_setup(drive);
+
+	/* set up the controller registers */
+	ide_pktcmd_tf_load(drive, IDE_TFLAG_OUT_NSECT | IDE_TFLAG_OUT_LBAL,
+			   xferlen, drive->dma);
+
+	if (drive->atapi_flags & IDE_AFLAG_DRQ_INTERRUPT) {
+		/* waiting for CDB interrupt, not DMA yet. */
+		if (drive->dma)
+			drive->waiting_for_dma = 0;
+
+		/* packet command */
+		ide_execute_command(drive, ATA_CMD_PACKET,
+				    cdrom_transfer_packet_command,
+				    ATAPI_WAIT_PC, ide_cd_expiry);
+		return ide_started;
+	} else {
+		ide_execute_pkt_cmd(drive);
+
+		return cdrom_transfer_packet_command(drive);
+	}
+}
+
+/*
  * 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.
@@ -680,8 +681,6 @@ static int ide_cd_check_transfer_size(ide_drive_t *drive, int len)
 	return 1;
 }
 
-static ide_startstop_t cdrom_newpc_intr(ide_drive_t *);
-
 static ide_startstop_t ide_cd_prepare_rw_request(ide_drive_t *drive,
 						 struct request *rq)
 {
@@ -724,20 +723,6 @@ static ide_startstop_t ide_cd_prepare_rw_request(ide_drive_t *drive,
 }
 
 /*
- * 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 = drive->hwif->hwgroup->rq;
-
-	/* send the command to the drive and return */
-	return cdrom_transfer_packet_command(drive, rq, cdrom_newpc_intr);
-}
-
-/*
  * Fix up a possibly partially-processed request so that we can start it over
  * entirely, or even put it back on the request queue.
  */
@@ -1126,13 +1111,6 @@ static ide_startstop_t cdrom_start_rw(ide_drive_t *drive, struct request *rq)
 	return ide_started;
 }
 
-static ide_startstop_t cdrom_do_newpc_cont(ide_drive_t *drive)
-{
-	struct request *rq = HWGROUP(drive)->rq;
-
-	return cdrom_transfer_packet_command(drive, rq, cdrom_newpc_intr);
-}
-
 static void cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
 {
 
@@ -1177,7 +1155,6 @@ static void cdrom_do_block_pc(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_handler_t *fn;
 
 	ide_debug_log(IDE_DBG_RQ, "Call %s, rq->cmd[0]: 0x%x, "
 		      "rq->cmd_type: 0x%x, block: %llu\n",
@@ -1185,7 +1162,6 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
 		      (unsigned long long)block);
 
 	if (blk_fs_request(rq)) {
-		fn = cdrom_start_rw_cont;
 
 		if (cdrom_start_rw(drive, rq) == ide_stopped)
 			return ide_stopped;
@@ -1194,7 +1170,6 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
 			return ide_stopped;
 	} else if (blk_sense_request(rq) || blk_pc_request(rq) ||
 		   rq->cmd_type == REQ_TYPE_ATA_PC) {
-		fn = cdrom_do_newpc_cont;
 
 		if (!rq->timeout)
 			rq->timeout = ATAPI_WAIT_PC;
@@ -1210,7 +1185,7 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
 		return ide_stopped;
 	}
 
-	return cdrom_start_packet_command(drive, fn);
+	return cdrom_start_packet_command(drive);
 }
 
 /*
-- 
1.6.0.4

Re: [PATCH 5/8] ide-cd: remove handler wrappers

[Bartlomiej Zolnierkiewicz]

On Thursday 18 December 2008, Borislav Petkov wrote:
> Remove cdrom_do_newpc_cont and cdrom_start_rw_cont wrappers and pass
> cdrom_transfer_packet_command to ide_execute_command directly.
> 
> There should be no functionality change resulting from this patch.
> 
> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
> ---
>  drivers/ide/ide-cd.c |  121 ++++++++++++++++++++------------------------------
>  1 files changed, 48 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> index 34981f5..f3c7cc3 100644
> --- a/drivers/ide/ide-cd.c
> +++ b/drivers/ide/ide-cd.c
> @@ -511,48 +511,7 @@ end_request:
>  	return 1;
>  }
>  
> -/*
> - * Set up the device registers for transferring a packet command on DEV,
> - * expecting to later transfer XFERLEN bytes.  HANDLER is the routine
> - * which actually transfers the command to the drive.  If this is a
> - * drq_interrupt device, this routine will arrange for HANDLER to be
> - * called when the interrupt from the drive arrives.  Otherwise, HANDLER
> - * will be called immediately after the drive is prepared for the transfer.
> - */
> -static ide_startstop_t cdrom_start_packet_command(ide_drive_t *drive,
> -						  ide_handler_t *handler)
> -{
> -	ide_hwif_t *hwif = drive->hwif;
> -	struct request *rq = hwif->hwgroup->rq;
> -	int xferlen;
> -
> -	xferlen = ide_cd_get_xferlen(rq);
> -
> -	ide_debug_log(IDE_DBG_PC, "Call %s, xferlen: %d\n", __func__, xferlen);
> -
> -	/* FIXME: for Virtual DMA we must check harder */
> -	if (drive->dma)
> -		drive->dma = !hwif->dma_ops->dma_setup(drive);
> -
> -	/* set up the controller registers */
> -	ide_pktcmd_tf_load(drive, IDE_TFLAG_OUT_NSECT | IDE_TFLAG_OUT_LBAL,
> -			   xferlen, drive->dma);
> -
> -	if (drive->atapi_flags & IDE_AFLAG_DRQ_INTERRUPT) {
> -		/* waiting for CDB interrupt, not DMA yet. */
> -		if (drive->dma)
> -			drive->waiting_for_dma = 0;
> -
> -		/* packet command */
> -		ide_execute_command(drive, ATA_CMD_PACKET, handler,
> -				    ATAPI_WAIT_PC, ide_cd_expiry);
> -		return ide_started;
> -	} else {
> -		ide_execute_pkt_cmd(drive);
> -
> -		return (*handler) (drive);
> -	}
> -}
> +static ide_startstop_t cdrom_newpc_intr(ide_drive_t *);
>  
>  /*
>   * Send a packet command to DRIVE described by CMD_BUF and CMD_LEN. The device
> @@ -561,11 +520,10 @@ static ide_startstop_t cdrom_start_packet_command(ide_drive_t *drive,
>   * there's data ready.
>   */
>  #define ATAPI_MIN_CDB_BYTES 12
> -static ide_startstop_t cdrom_transfer_packet_command(ide_drive_t *drive,
> -					  struct request *rq,
> -					  ide_handler_t *handler)
> +static ide_startstop_t cdrom_transfer_packet_command(ide_drive_t *drive)
>  {
>  	ide_hwif_t *hwif = drive->hwif;
> +	struct request *rq = hwif->hwgroup->rq;
>  	int cmd_len;
>  	ide_startstop_t startstop;
>  
> @@ -592,7 +550,7 @@ static ide_startstop_t cdrom_transfer_packet_command(ide_drive_t *drive,
>  	}
>  
>  	/* arm the interrupt handler */
> -	ide_set_handler(drive, handler, rq->timeout, ide_cd_expiry);
> +	ide_set_handler(drive, cdrom_newpc_intr, rq->timeout, ide_cd_expiry);
>  
>  	/* ATAPI commands get padded out to 12 bytes minimum */
>  	cmd_len = COMMAND_SIZE(rq->cmd[0]);
> @@ -610,6 +568,49 @@ static ide_startstop_t cdrom_transfer_packet_command(ide_drive_t *drive,
>  }
>  
>  /*
> + * Set up the device registers for transferring a packet command on DEV,
> + * expecting to later transfer XFERLEN bytes.  HANDLER is the routine
> + * which actually transfers the command to the drive.  If this is a
> + * drq_interrupt device, this routine will arrange for HANDLER to be
> + * called when the interrupt from the drive arrives.  Otherwise, HANDLER
> + * will be called immediately after the drive is prepared for the transfer.
> + */
> +static ide_startstop_t cdrom_start_packet_command(ide_drive_t *drive)
> +{
> +	ide_hwif_t *hwif = drive->hwif;
> +	struct request *rq = hwif->hwgroup->rq;
> +	int xferlen;
> +
> +	xferlen = ide_cd_get_xferlen(rq);
> +
> +	ide_debug_log(IDE_DBG_PC, "Call %s, xferlen: %d\n", __func__, xferlen);
> +
> +	/* FIXME: for Virtual DMA we must check harder */
> +	if (drive->dma)
> +		drive->dma = !hwif->dma_ops->dma_setup(drive);
> +
> +	/* set up the controller registers */
> +	ide_pktcmd_tf_load(drive, IDE_TFLAG_OUT_NSECT | IDE_TFLAG_OUT_LBAL,
> +			   xferlen, drive->dma);
> +
> +	if (drive->atapi_flags & IDE_AFLAG_DRQ_INTERRUPT) {
> +		/* waiting for CDB interrupt, not DMA yet. */
> +		if (drive->dma)
> +			drive->waiting_for_dma = 0;
> +
> +		/* packet command */
> +		ide_execute_command(drive, ATA_CMD_PACKET,
> +				    cdrom_transfer_packet_command,
> +				    ATAPI_WAIT_PC, ide_cd_expiry);
> +		return ide_started;
> +	} else {
> +		ide_execute_pkt_cmd(drive);
> +
> +		return cdrom_transfer_packet_command(drive);
> +	}
> +}
> +
> +/*
>   * 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.
> @@ -680,8 +681,6 @@ static int ide_cd_check_transfer_size(ide_drive_t *drive, int len)
>  	return 1;
>  }
>  
> -static ide_startstop_t cdrom_newpc_intr(ide_drive_t *);
> -
>  static ide_startstop_t ide_cd_prepare_rw_request(ide_drive_t *drive,
>  						 struct request *rq)
>  {
> @@ -724,20 +723,6 @@ static ide_startstop_t ide_cd_prepare_rw_request(ide_drive_t *drive,
>  }
>  
>  /*
> - * 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 = drive->hwif->hwgroup->rq;
> -
> -	/* send the command to the drive and return */
> -	return cdrom_transfer_packet_command(drive, rq, cdrom_newpc_intr);
> -}
> -
> -/*
>   * Fix up a possibly partially-processed request so that we can start it over
>   * entirely, or even put it back on the request queue.
>   */
> @@ -1126,13 +1111,6 @@ static ide_startstop_t cdrom_start_rw(ide_drive_t *drive, struct request *rq)
>  	return ide_started;
>  }
>  
> -static ide_startstop_t cdrom_do_newpc_cont(ide_drive_t *drive)
> -{
> -	struct request *rq = HWGROUP(drive)->rq;
> -
> -	return cdrom_transfer_packet_command(drive, rq, cdrom_newpc_intr);
> -}
> -
>  static void cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
>  {
>  
> @@ -1177,7 +1155,6 @@ static void cdrom_do_block_pc(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_handler_t *fn;
>  
>  	ide_debug_log(IDE_DBG_RQ, "Call %s, rq->cmd[0]: 0x%x, "
>  		      "rq->cmd_type: 0x%x, block: %llu\n",
> @@ -1185,7 +1162,6 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
>  		      (unsigned long long)block);
>  
>  	if (blk_fs_request(rq)) {
> -		fn = cdrom_start_rw_cont;
>  
>  		if (cdrom_start_rw(drive, rq) == ide_stopped)
>  			return ide_stopped;
> @@ -1194,7 +1170,6 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
>  			return ide_stopped;
>  	} else if (blk_sense_request(rq) || blk_pc_request(rq) ||
>  		   rq->cmd_type == REQ_TYPE_ATA_PC) {
> -		fn = cdrom_do_newpc_cont;
>  
>  		if (!rq->timeout)
>  			rq->timeout = ATAPI_WAIT_PC;
> @@ -1210,7 +1185,7 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
>  		return ide_stopped;
>  	}
>  
> -	return cdrom_start_packet_command(drive, fn);
> +	return cdrom_start_packet_command(drive);
>  }

There is no need to move cdrom_start_packet_command() around (especially
since it goes away in patch #9/8) and not doing so allows to see changes
to the function immediately.  Fixed in the merged patch version:

From: Borislav Petkov <petkovbb@googlemail.com>
Subject: [PATCH 5/8] ide-cd: remove handler wrappers

Remove cdrom_do_newpc_cont and cdrom_start_rw_cont wrappers and pass
cdrom_transfer_packet_command to ide_execute_command directly.

There should be no functionality change resulting from this patch.

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
[bart: don't move cdrom_start_packet_command() around, remove newlines]
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-cd.c |   49 +++++++++++--------------------------------------
 1 file changed, 11 insertions(+), 38 deletions(-)

Index: b/drivers/ide/ide-cd.c
===================================================================
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -511,6 +511,9 @@ end_request:
 	return 1;
 }
 
+static ide_startstop_t cdrom_transfer_packet_command(ide_drive_t *);
+static ide_startstop_t cdrom_newpc_intr(ide_drive_t *);
+
 /*
  * Set up the device registers for transferring a packet command on DEV,
  * expecting to later transfer XFERLEN bytes.  HANDLER is the routine
@@ -519,8 +522,7 @@ end_request:
  * called when the interrupt from the drive arrives.  Otherwise, HANDLER
  * will be called immediately after the drive is prepared for the transfer.
  */
-static ide_startstop_t cdrom_start_packet_command(ide_drive_t *drive,
-						  ide_handler_t *handler)
+static ide_startstop_t cdrom_start_packet_command(ide_drive_t *drive)
 {
 	ide_hwif_t *hwif = drive->hwif;
 	struct request *rq = hwif->hwgroup->rq;
@@ -544,13 +546,14 @@ static ide_startstop_t cdrom_start_packe
 			drive->waiting_for_dma = 0;
 
 		/* packet command */
-		ide_execute_command(drive, ATA_CMD_PACKET, handler,
+		ide_execute_command(drive, ATA_CMD_PACKET,
+				    cdrom_transfer_packet_command,
 				    ATAPI_WAIT_PC, ide_cd_expiry);
 		return ide_started;
 	} else {
 		ide_execute_pkt_cmd(drive);
 
-		return (*handler) (drive);
+		return cdrom_transfer_packet_command(drive);
 	}
 }
 
@@ -561,11 +564,10 @@ static ide_startstop_t cdrom_start_packe
  * there's data ready.
  */
 #define ATAPI_MIN_CDB_BYTES 12
-static ide_startstop_t cdrom_transfer_packet_command(ide_drive_t *drive,
-					  struct request *rq,
-					  ide_handler_t *handler)
+static ide_startstop_t cdrom_transfer_packet_command(ide_drive_t *drive)
 {
 	ide_hwif_t *hwif = drive->hwif;
+	struct request *rq = hwif->hwgroup->rq;
 	int cmd_len;
 	ide_startstop_t startstop;
 
@@ -592,7 +594,7 @@ static ide_startstop_t cdrom_transfer_pa
 	}
 
 	/* arm the interrupt handler */
-	ide_set_handler(drive, handler, rq->timeout, ide_cd_expiry);
+	ide_set_handler(drive, cdrom_newpc_intr, rq->timeout, ide_cd_expiry);
 
 	/* ATAPI commands get padded out to 12 bytes minimum */
 	cmd_len = COMMAND_SIZE(rq->cmd[0]);
@@ -680,8 +682,6 @@ static int ide_cd_check_transfer_size(id
 	return 1;
 }
 
-static ide_startstop_t cdrom_newpc_intr(ide_drive_t *);
-
 static ide_startstop_t ide_cd_prepare_rw_request(ide_drive_t *drive,
 						 struct request *rq)
 {
@@ -724,20 +724,6 @@ static ide_startstop_t ide_cd_prepare_rw
 }
 
 /*
- * 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 = drive->hwif->hwgroup->rq;
-
-	/* send the command to the drive and return */
-	return cdrom_transfer_packet_command(drive, rq, cdrom_newpc_intr);
-}
-
-/*
  * Fix up a possibly partially-processed request so that we can start it over
  * entirely, or even put it back on the request queue.
  */
@@ -1126,13 +1112,6 @@ static ide_startstop_t cdrom_start_rw(id
 	return ide_started;
 }
 
-static ide_startstop_t cdrom_do_newpc_cont(ide_drive_t *drive)
-{
-	struct request *rq = HWGROUP(drive)->rq;
-
-	return cdrom_transfer_packet_command(drive, rq, cdrom_newpc_intr);
-}
-
 static void cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
 {
 
@@ -1177,16 +1156,12 @@ static void cdrom_do_block_pc(ide_drive_
 static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
 					sector_t block)
 {
-	ide_handler_t *fn;
-
 	ide_debug_log(IDE_DBG_RQ, "Call %s, rq->cmd[0]: 0x%x, "
 		      "rq->cmd_type: 0x%x, block: %llu\n",
 		      __func__, rq->cmd[0], rq->cmd_type,
 		      (unsigned long long)block);
 
 	if (blk_fs_request(rq)) {
-		fn = cdrom_start_rw_cont;
-
 		if (cdrom_start_rw(drive, rq) == ide_stopped)
 			return ide_stopped;
 
@@ -1194,8 +1169,6 @@ static ide_startstop_t ide_cd_do_request
 			return ide_stopped;
 	} else if (blk_sense_request(rq) || blk_pc_request(rq) ||
 		   rq->cmd_type == REQ_TYPE_ATA_PC) {
-		fn = cdrom_do_newpc_cont;
-
 		if (!rq->timeout)
 			rq->timeout = ATAPI_WAIT_PC;
 
@@ -1210,7 +1183,7 @@ static ide_startstop_t ide_cd_do_request
 		return ide_stopped;
 	}
 
-	return cdrom_start_packet_command(drive, fn);
+	return cdrom_start_packet_command(drive);
 }
 
 /*
\0

[PATCH 6/8] ide-atapi: remove timeout arg to ide_issue_pc

[Borislav Petkov]

There should be no functionality change resulting from this patch.

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-atapi.c  |    7 ++++++-
 drivers/ide/ide-floppy.c |    2 +-
 drivers/ide/ide-tape.c   |    2 +-
 include/linux/ide.h      |    2 +-
 4 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 2577782..dbed09c 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -563,11 +563,12 @@ static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
 	return ide_started;
 }
 
-ide_startstop_t ide_issue_pc(ide_drive_t *drive, unsigned int timeout)
+ide_startstop_t ide_issue_pc(ide_drive_t *drive)
 {
 	struct ide_atapi_pc *pc;
 	ide_hwif_t *hwif = drive->hwif;
 	ide_expiry_t *expiry = NULL;
+	unsigned int timeout;
 	u32 tf_flags;
 	u16 bcount;
 
@@ -575,6 +576,7 @@ ide_startstop_t ide_issue_pc(ide_drive_t *drive, unsigned int timeout)
 		tf_flags = IDE_TFLAG_OUT_NSECT | IDE_TFLAG_OUT_LBAL;
 		bcount = ide_cd_get_xferlen(hwif->hwgroup->rq);
 		expiry = ide_cd_expiry;
+		timeout = ATAPI_WAIT_PC;
 
 		if (drive->dma)
 			drive->dma = !hwif->dma_ops->dma_setup(drive);
@@ -601,6 +603,9 @@ ide_startstop_t ide_issue_pc(ide_drive_t *drive, unsigned int timeout)
 
 		if (!drive->dma)
 			pc->flags &= ~PC_FLAG_DMA_OK;
+
+		timeout = (drive->media == ide_floppy) ? WAIT_FLOPPY_CMD
+						       : WAIT_TAPE_CMD;
 	}
 
 	ide_pktcmd_tf_load(drive, tf_flags, bcount, drive->dma);
diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index fdec729..0a48e2d 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -197,7 +197,7 @@ static ide_startstop_t idefloppy_issue_pc(ide_drive_t *drive,
 
 	pc->retries++;
 
-	return ide_issue_pc(drive, WAIT_FLOPPY_CMD);
+	return ide_issue_pc(drive);
 }
 
 void ide_floppy_create_read_capacity_cmd(struct ide_atapi_pc *pc)
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index ac9e29a..5d2aa22 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -694,7 +694,7 @@ static ide_startstop_t idetape_issue_pc(ide_drive_t *drive,
 
 	pc->retries++;
 
-	return ide_issue_pc(drive, WAIT_TAPE_CMD);
+	return ide_issue_pc(drive);
 }
 
 /* A mode sense command is used to "sense" tape parameters. */
diff --git a/include/linux/ide.h b/include/linux/ide.h
index ad57a44..db5ef8a 100644
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -1250,7 +1250,7 @@ int ide_cd_expiry(ide_drive_t *);
 
 int ide_cd_get_xferlen(struct request *);
 
-ide_startstop_t ide_issue_pc(ide_drive_t *, unsigned int);
+ide_startstop_t ide_issue_pc(ide_drive_t *);
 
 ide_startstop_t do_rw_taskfile(ide_drive_t *, ide_task_t *);
 
-- 
1.6.0.4

[PATCH 7/8] ide-atapi: put the rest of non-ide-cd code into the else-clause of ide_transfer_pc

[Borislav Petkov]

There should be no functionality change resulting from this patch.

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

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index dbed09c..f3dea66 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -534,17 +534,17 @@ static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
 							       : WAIT_TAPE_CMD;
 			expiry = NULL;
 		}
-	}
 
-	ireason = ide_read_ireason(drive);
-	if (drive->media == ide_tape)
-		ireason = ide_wait_ireason(drive, ireason);
+		ireason = ide_read_ireason(drive);
+		if (drive->media == ide_tape)
+			ireason = ide_wait_ireason(drive, ireason);
 
-	if ((ireason & ATAPI_COD) == 0 || (ireason & ATAPI_IO)) {
-		printk(KERN_ERR "%s: (IO,CoD) != (0,1) while issuing "
-				"a packet command\n", drive->name);
+		if ((ireason & ATAPI_COD) == 0 || (ireason & ATAPI_IO)) {
+			printk(KERN_ERR "%s: (IO,CoD) != (0,1) while issuing "
+					"a packet command\n", drive->name);
 
-		return ide_do_reset(drive);
+			return ide_do_reset(drive);
+		}
 	}
 
 	/* Set the interrupt routine */
-- 
1.6.0.4

[PATCH 8/8] ide-atapi: start dma in a drive-specific way

[Borislav Petkov]

There should be no functionality change resulting from this patch.

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

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index f3dea66..5b92e0a 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -489,7 +489,7 @@ static int ide_delayed_transfer_pc(ide_drive_t *drive)
 
 static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
 {
-	struct ide_atapi_pc *pc = drive->pc;
+	struct ide_atapi_pc *uninitialized_var(pc);
 	ide_hwif_t *hwif = drive->hwif;
 	struct request *rq = hwif->hwgroup->rq;
 	ide_expiry_t *expiry;
@@ -519,6 +519,8 @@ static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
 		expiry  = ide_cd_expiry;
 
 	} else {
+		pc = drive->pc;
+
 		cmd_len = ATAPI_MIN_CDB_BYTES;
 
 		/*
@@ -551,9 +553,14 @@ static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
 	ide_set_handler(drive, ide_pc_intr, timeout, expiry);
 
 	/* Begin DMA, if necessary */
-	if (pc->flags & PC_FLAG_DMA_OK) {
-		pc->flags |= PC_FLAG_DMA_IN_PROGRESS;
-		hwif->dma_ops->dma_start(drive);
+	if (dev_is_idecd(drive)) {
+		if (drive->dma)
+			hwif->dma_ops->dma_start(drive);
+	} else {
+		if (pc->flags & PC_FLAG_DMA_OK) {
+			pc->flags |= PC_FLAG_DMA_IN_PROGRESS;
+			hwif->dma_ops->dma_start(drive);
+		}
 	}
 
 	/* Send the actual packet */
-- 
1.6.0.4

Re: [PATCH 0/8] ide-cd: first conversion batch

[Bartlomiej Zolnierkiewicz]

Hi,

On Thursday 18 December 2008, Borislav Petkov wrote:
> Hi Bart,
> 
> here's the first batch of conversion patches. Now ide_issue_pc/ide_transfer_pc
> are almost completely aligned to their ide-cd counterparts
> cdrom_start_packet_command/cdrom_transfer_packet_command. The only thing

Looks good generally but I need more time for detailed review.

[ Your super-quick action on this has caught me unprepared. ;) ]

> missing is the setting of the irq handler through ide_set_handler but this
> is not that trivial since if i'd moved ide-cd's irq handler to ide-atapi
> that would suck in a bunch of other ide-cd functions which is not what we
> want, (yes/no)? and I'm going to have to ponder a little bit more on that.

Yes, we don't want to move them over.  In worst case we can add
ide_handler_t *__old_pc_handler to ide_drive_t and set it in ide-cd
(an acceptable hack this time) but there might be better options.

Thanks,
Bart

Re: [PATCH 0/8] ide-cd: first conversion batch

[Borislav Petkov]

On Fri, Dec 19, 2008 at 09:15:40PM +0100, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Thursday 18 December 2008, Borislav Petkov wrote:
> > Hi Bart,
> > 
> > here's the first batch of conversion patches. Now ide_issue_pc/ide_transfer_pc
> > are almost completely aligned to their ide-cd counterparts
> > cdrom_start_packet_command/cdrom_transfer_packet_command. The only thing
> 
> Looks good generally but I need more time for detailed review.
> 
> [ Your super-quick action on this has caught me unprepared. ;) ]

sorry about that, it was all in good intentions :)

> > missing is the setting of the irq handler through ide_set_handler but this
> > is not that trivial since if i'd moved ide-cd's irq handler to ide-atapi
> > that would suck in a bunch of other ide-cd functions which is not what we
> > want, (yes/no)? and I'm going to have to ponder a little bit more on that.
> 
> Yes, we don't want to move them over.  In worst case we can add
> ide_handler_t *__old_pc_handler to ide_drive_t and set it in ide-cd
> (an acceptable hack this time) but there might be better options.

Can you believe that I was just experimenting with _exactly_ the same
option :). Patches coming on tomorrow hopefully and still before the
merge window.

By the way, you've posted a bunch of ide improvements/cleanups but
they're not on kernel.org yet. What's the plan with those, do you want
to upload them first and then I redo my patches or... ?

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 0/8] ide-cd: first conversion batch

[Borislav Petkov]

Hi,

On Fri, Dec 19, 2008 at 09:15:40PM +0100, Bartlomiej Zolnierkiewicz wrote:

[.. ]

> Yes, we don't want to move them over.  In worst case we can add
> ide_handler_t *__old_pc_handler to ide_drive_t and set it in ide-cd
> (an acceptable hack this time) but there might be better options.

Ok, here's the final patch moving ide-cd to the services of
ide_(issue|transfer)_pc. The only change in functionality is that we
don't do cdrom_decode_status for DRQ_INTERRUPT devices but this is
probably not that relevant anymore since we busy-wait for DRQ to get set
through ide_wait_stat, as we talked about it before - it being a bugfix for
all atapi devices. If there's still interest for that (and I think it
could be of use to have an all-atapi function ide_decode_status() that
dumps error messages if DRQ doesn't get set based on the sense_key) I'll
add cook something up.

--
From: Borislav Petkov <petkovbb@gmail.com>
Date: Sat, 20 Dec 2008 07:06:48 +0100
Subject: [PATCH 9/9] ide-cd: convert to ide-atapi facilities

... and remove no longer needed cdrom_start_packet_command and
cdrom_transfer_packet_command.

Tested lightly with ide-cd and ide-floppy.

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-atapi.c |    6 ++-
 drivers/ide/ide-cd.c    |  102 +----------------------------------------------
 include/linux/ide.h     |    2 +
 3 files changed, 9 insertions(+), 101 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 5b92e0a..0e96da5 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -550,7 +550,10 @@ static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
 	}
 
 	/* Set the interrupt routine */
-	ide_set_handler(drive, ide_pc_intr, timeout, expiry);
+	ide_set_handler(drive,
+			(dev_is_idecd(drive) ? drive->irq_handler
+					     : ide_pc_intr),
+			timeout, expiry);
 
 	/* Begin DMA, if necessary */
 	if (dev_is_idecd(drive)) {
@@ -580,6 +583,7 @@ ide_startstop_t ide_issue_pc(ide_drive_t *drive)
 	u16 bcount;
 
 	if (dev_is_idecd(drive)) {
+
 		tf_flags = IDE_TFLAG_OUT_NSECT | IDE_TFLAG_OUT_LBAL;
 		bcount = ide_cd_get_xferlen(hwif->hwgroup->rq);
 		expiry = ide_cd_expiry;
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index f3c7cc3..b6087a2 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -511,105 +511,6 @@ end_request:
 	return 1;
 }
 
-static ide_startstop_t cdrom_newpc_intr(ide_drive_t *);
-
-/*
- * Send a packet command to DRIVE described by CMD_BUF and CMD_LEN. The device
- * registers must have already been prepared by cdrom_start_packet_command.
- * HANDLER is the interrupt handler to call when the command completes or
- * there's data ready.
- */
-#define ATAPI_MIN_CDB_BYTES 12
-static ide_startstop_t cdrom_transfer_packet_command(ide_drive_t *drive)
-{
-	ide_hwif_t *hwif = drive->hwif;
-	struct request *rq = hwif->hwgroup->rq;
-	int cmd_len;
-	ide_startstop_t startstop;
-
-	ide_debug_log(IDE_DBG_PC, "Call %s\n", __func__);
-
-	if (drive->atapi_flags & IDE_AFLAG_DRQ_INTERRUPT) {
-		/*
-		 * Here we should have been called after receiving an interrupt
-		 * from the device.  DRQ should how be set.
-		 */
-
-		/* check for errors */
-		if (cdrom_decode_status(drive, ATA_DRQ, NULL))
-			return ide_stopped;
-
-		/* ok, next interrupt will be DMA interrupt */
-		if (drive->dma)
-			drive->waiting_for_dma = 1;
-	} else {
-		/* otherwise, we must wait for DRQ to get set */
-		if (ide_wait_stat(&startstop, drive, ATA_DRQ,
-				  ATA_BUSY, WAIT_READY))
-			return startstop;
-	}
-
-	/* arm the interrupt handler */
-	ide_set_handler(drive, cdrom_newpc_intr, rq->timeout, ide_cd_expiry);
-
-	/* ATAPI commands get padded out to 12 bytes minimum */
-	cmd_len = COMMAND_SIZE(rq->cmd[0]);
-	if (cmd_len < ATAPI_MIN_CDB_BYTES)
-		cmd_len = ATAPI_MIN_CDB_BYTES;
-
-	/* send the command to the device */
-	hwif->tp_ops->output_data(drive, NULL, rq->cmd, cmd_len);
-
-	/* start the DMA if need be */
-	if (drive->dma)
-		hwif->dma_ops->dma_start(drive);
-
-	return ide_started;
-}
-
-/*
- * Set up the device registers for transferring a packet command on DEV,
- * expecting to later transfer XFERLEN bytes.  HANDLER is the routine
- * which actually transfers the command to the drive.  If this is a
- * drq_interrupt device, this routine will arrange for HANDLER to be
- * called when the interrupt from the drive arrives.  Otherwise, HANDLER
- * will be called immediately after the drive is prepared for the transfer.
- */
-static ide_startstop_t cdrom_start_packet_command(ide_drive_t *drive)
-{
-	ide_hwif_t *hwif = drive->hwif;
-	struct request *rq = hwif->hwgroup->rq;
-	int xferlen;
-
-	xferlen = ide_cd_get_xferlen(rq);
-
-	ide_debug_log(IDE_DBG_PC, "Call %s, xferlen: %d\n", __func__, xferlen);
-
-	/* FIXME: for Virtual DMA we must check harder */
-	if (drive->dma)
-		drive->dma = !hwif->dma_ops->dma_setup(drive);
-
-	/* set up the controller registers */
-	ide_pktcmd_tf_load(drive, IDE_TFLAG_OUT_NSECT | IDE_TFLAG_OUT_LBAL,
-			   xferlen, drive->dma);
-
-	if (drive->atapi_flags & IDE_AFLAG_DRQ_INTERRUPT) {
-		/* waiting for CDB interrupt, not DMA yet. */
-		if (drive->dma)
-			drive->waiting_for_dma = 0;
-
-		/* packet command */
-		ide_execute_command(drive, ATA_CMD_PACKET,
-				    cdrom_transfer_packet_command,
-				    ATAPI_WAIT_PC, ide_cd_expiry);
-		return ide_started;
-	} else {
-		ide_execute_pkt_cmd(drive);
-
-		return cdrom_transfer_packet_command(drive);
-	}
-}
-
 /*
  * Check the contents of the interrupt reason register from the cdrom
  * and attempt to recover if there are problems.  Returns  0 if everything's
@@ -1185,7 +1086,7 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
 		return ide_stopped;
 	}
 
-	return cdrom_start_packet_command(drive);
+	return ide_issue_pc(drive);
 }
 
 /*
@@ -2084,6 +1985,7 @@ static int ide_cd_probe(ide_drive_t *drive)
 	}
 
 	drive->debug_mask = debug_mask;
+	drive->irq_handler = cdrom_newpc_intr;
 
 	info = kzalloc(sizeof(struct cdrom_info), GFP_KERNEL);
 	if (info == NULL) {
diff --git a/include/linux/ide.h b/include/linux/ide.h
index db5ef8a..2fa5539 100644
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -662,6 +662,8 @@ struct ide_drive_s {
 	int  (*pc_io_buffers)(struct ide_drive_s *, struct ide_atapi_pc *,
 			      unsigned int, int);
 
+	ide_startstop_t (*irq_handler)(struct ide_drive_s *);
+
 	unsigned long atapi_flags;
 
 	struct ide_atapi_pc request_sense_pc;
-- 
1.6.0.4


-- 
Regards/Gruss,
    Boris.

Re: [PATCH 0/8] ide-cd: first conversion batch

[Bartlomiej Zolnierkiewicz]

Hi,

On Saturday 20 December 2008, Borislav Petkov wrote:
> Hi,
> 
> On Fri, Dec 19, 2008 at 09:15:40PM +0100, Bartlomiej Zolnierkiewicz wrote:
> 
> [.. ]
> 
> > Yes, we don't want to move them over.  In worst case we can add
> > ide_handler_t *__old_pc_handler to ide_drive_t and set it in ide-cd
> > (an acceptable hack this time) but there might be better options.
> 
> Ok, here's the final patch moving ide-cd to the services of
> ide_(issue|transfer)_pc. The only change in functionality is that we

I merged the patchset (with 2 very minor fixups) but I think that this
one still needs some small preparatory changes first.

[ BTW I also merged outstanding ide patches.  Not many ide-{atapi,cd}
  changes in them though.  Thus if you prefer you may as well send me
  patches based on the old tree and let me handle potential rejects. ]

> don't do cdrom_decode_status for DRQ_INTERRUPT devices but this is
> probably not that relevant anymore since we busy-wait for DRQ to get set
> through ide_wait_stat, as we talked about it before - it being a bugfix for
> all atapi devices. If there's still interest for that (and I think it

Yes, this change is OK but for bisectability reasons it would be better
to do it in pre-patch (which would fix ide-cd.c accordingly).

[ The other changes in functionality are small and acceptable for this
  patch (i.e. ide_wait_stat() prints error message now) except the change
  of the ordering between ->dma_start and ->output_data calls -- which
  also seems to deserve patch on its own. ]

Otherwise it all looks fine and Big Thanks for working on this!

Re: [PATCH 0/8] ide-cd: first conversion batch

[Borislav Petkov]

Hi Bart,

On Sun, Dec 21, 2008 at 08:05:56PM +0100, Bartlomiej Zolnierkiewicz wrote:

[.. ]
> > don't do cdrom_decode_status for DRQ_INTERRUPT devices but this is
> > probably not that relevant anymore since we busy-wait for DRQ to get set
> > through ide_wait_stat, as we talked about it before - it being a bugfix for
> > all atapi devices. If there's still interest for that (and I think it
> 
> Yes, this change is OK but for bisectability reasons it would be better
> to do it in pre-patch (which would fix ide-cd.c accordingly).
> 
> [ The other changes in functionality are small and acceptable for this
>   patch (i.e. ide_wait_stat() prints error message now) except the change
>   of the ordering between ->dma_start and ->output_data calls -- which
>   also seems to deserve patch on its own. ]

sorry for the delay but you know how it is, Christmas and all. Anyway,
here are the patches attached as you requested, just in time for the
merge window.

--
From: Borislav Petkov <petkovbb@gmail.com>
Date: Thu, 25 Dec 2008 18:12:34 +0100
Subject: [PATCH 09/11] ide-cd: wait for DRQ to get set per default

... instead of assuming it is set for accelerated DRQ type devices.

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

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 6c7dd8f..1c1ba43 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -572,24 +572,17 @@ static ide_startstop_t cdrom_transfer_packet_command(ide_drive_t *drive)
 
 	ide_debug_log(IDE_DBG_PC, "Call %s\n", __func__);
 
-	if (drive->atapi_flags & IDE_AFLAG_DRQ_INTERRUPT) {
-		/*
-		 * Here we should have been called after receiving an interrupt
-		 * from the device.  DRQ should how be set.
-		 */
-
-		/* check for errors */
-		if (cdrom_decode_status(drive, ATA_DRQ, NULL))
-			return ide_stopped;
+	/* we must wait for DRQ to get set */
+	if (ide_wait_stat(&startstop, drive, ATA_DRQ, ATA_BUSY, WAIT_READY)) {
+		printk(KERN_ERR "%s: timeout while waiting for DRQ to assert\n",
+				drive->name);
+		return startstop;
+	}
 
+	if (drive->atapi_flags & IDE_AFLAG_DRQ_INTERRUPT) {
 		/* ok, next interrupt will be DMA interrupt */
 		if (drive->dma)
 			drive->waiting_for_dma = 1;
-	} else {
-		/* otherwise, we must wait for DRQ to get set */
-		if (ide_wait_stat(&startstop, drive, ATA_DRQ,
-				  ATA_BUSY, WAIT_READY))
-			return startstop;
 	}
 
 	/* arm the interrupt handler */
-- 
1.6.0.4


-- 
Regards/Gruss,
    Boris.

Re: [PATCH 0/8] ide-cd: first conversion batch

[Bartlomiej Zolnierkiewicz]

On Friday 26 December 2008, Borislav Petkov wrote:
> Hi Bart,
> 
> On Sun, Dec 21, 2008 at 08:05:56PM +0100, Bartlomiej Zolnierkiewicz wrote:
> 
> [.. ]
> > > don't do cdrom_decode_status for DRQ_INTERRUPT devices but this is
> > > probably not that relevant anymore since we busy-wait for DRQ to get set
> > > through ide_wait_stat, as we talked about it before - it being a bugfix for
> > > all atapi devices. If there's still interest for that (and I think it
> > 
> > Yes, this change is OK but for bisectability reasons it would be better
> > to do it in pre-patch (which would fix ide-cd.c accordingly).
> > 
> > [ The other changes in functionality are small and acceptable for this
> >   patch (i.e. ide_wait_stat() prints error message now) except the change
> >   of the ordering between ->dma_start and ->output_data calls -- which
> >   also seems to deserve patch on its own. ]
> 
> sorry for the delay but you know how it is, Christmas and all. Anyway,
> here are the patches attached as you requested, just in time for the
> merge window.

np, I was busy with Christmas-fu myself...

I applied all three patches but I want them to see some time in linux-next
before push to mainline (just-in-case, they should make it into 2.6.29).

Thanks,
Bart

Re: [PATCH 0/8] ide-cd: first conversion batch

[Borislav Petkov]

From: Borislav Petkov <petkovbb@gmail.com>
Date: Thu, 25 Dec 2008 18:46:35 +0100
Subject: [PATCH 10/11] ide-cd: start DMA before sending the actual packet command

as it is done for all other IDE ATAPI devices.

There should be no functionality change resulting from this patch.

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

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 1c1ba43..2cb301d 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -593,13 +593,13 @@ static ide_startstop_t cdrom_transfer_packet_command(ide_drive_t *drive)
 	if (cmd_len < ATAPI_MIN_CDB_BYTES)
 		cmd_len = ATAPI_MIN_CDB_BYTES;
 
-	/* send the command to the device */
-	hwif->tp_ops->output_data(drive, NULL, rq->cmd, cmd_len);
-
 	/* start the DMA if need be */
 	if (drive->dma)
 		hwif->dma_ops->dma_start(drive);
 
+	/* send the command to the device */
+	hwif->tp_ops->output_data(drive, NULL, rq->cmd, cmd_len);
+
 	return ide_started;
 }
 
-- 
1.6.0.4

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 0/8] ide-cd: first conversion batch

[Borislav Petkov]

From: Borislav Petkov <petkovbb@gmail.com>
Date: Fri, 26 Dec 2008 14:39:54 +0100
Subject: [PATCH 11/11] ide-cd: convert to ide-atapi facilities

... and remove no longer needed cdrom_start_packet_command and
cdrom_transfer_packet_command.

Tested lightly with ide-cd and ide-floppy.

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-atapi.c |    5 ++-
 drivers/ide/ide-cd.c    |   96 +----------------------------------------------
 include/linux/ide.h     |    2 +
 3 files changed, 8 insertions(+), 95 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index d6a50d6..e96c012 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -549,7 +549,10 @@ static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
 	}
 
 	/* Set the interrupt routine */
-	ide_set_handler(drive, ide_pc_intr, timeout, expiry);
+	ide_set_handler(drive,
+			(dev_is_idecd(drive) ? drive->irq_handler
+					     : ide_pc_intr),
+			timeout, expiry);
 
 	/* Begin DMA, if necessary */
 	if (dev_is_idecd(drive)) {
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 2cb301d..cae6937 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -510,99 +510,6 @@ end_request:
 	return 1;
 }
 
-static ide_startstop_t cdrom_transfer_packet_command(ide_drive_t *);
-static ide_startstop_t cdrom_newpc_intr(ide_drive_t *);
-
-/*
- * Set up the device registers for transferring a packet command on DEV,
- * expecting to later transfer XFERLEN bytes.  HANDLER is the routine
- * which actually transfers the command to the drive.  If this is a
- * drq_interrupt device, this routine will arrange for HANDLER to be
- * called when the interrupt from the drive arrives.  Otherwise, HANDLER
- * will be called immediately after the drive is prepared for the transfer.
- */
-static ide_startstop_t cdrom_start_packet_command(ide_drive_t *drive)
-{
-	ide_hwif_t *hwif = drive->hwif;
-	struct request *rq = hwif->rq;
-	int xferlen;
-
-	xferlen = ide_cd_get_xferlen(rq);
-
-	ide_debug_log(IDE_DBG_PC, "Call %s, xferlen: %d\n", __func__, xferlen);
-
-	/* FIXME: for Virtual DMA we must check harder */
-	if (drive->dma)
-		drive->dma = !hwif->dma_ops->dma_setup(drive);
-
-	/* set up the controller registers */
-	ide_pktcmd_tf_load(drive, IDE_TFLAG_OUT_NSECT | IDE_TFLAG_OUT_LBAL,
-			   xferlen, drive->dma);
-
-	if (drive->atapi_flags & IDE_AFLAG_DRQ_INTERRUPT) {
-		/* waiting for CDB interrupt, not DMA yet. */
-		if (drive->dma)
-			drive->waiting_for_dma = 0;
-
-		/* packet command */
-		ide_execute_command(drive, ATA_CMD_PACKET,
-				    cdrom_transfer_packet_command,
-				    ATAPI_WAIT_PC, ide_cd_expiry);
-		return ide_started;
-	} else {
-		ide_execute_pkt_cmd(drive);
-
-		return cdrom_transfer_packet_command(drive);
-	}
-}
-
-/*
- * Send a packet command to DRIVE described by CMD_BUF and CMD_LEN. The device
- * registers must have already been prepared by cdrom_start_packet_command.
- * HANDLER is the interrupt handler to call when the command completes or
- * there's data ready.
- */
-#define ATAPI_MIN_CDB_BYTES 12
-static ide_startstop_t cdrom_transfer_packet_command(ide_drive_t *drive)
-{
-	ide_hwif_t *hwif = drive->hwif;
-	struct request *rq = hwif->rq;
-	int cmd_len;
-	ide_startstop_t startstop;
-
-	ide_debug_log(IDE_DBG_PC, "Call %s\n", __func__);
-
-	/* we must wait for DRQ to get set */
-	if (ide_wait_stat(&startstop, drive, ATA_DRQ, ATA_BUSY, WAIT_READY)) {
-		printk(KERN_ERR "%s: timeout while waiting for DRQ to assert\n",
-				drive->name);
-		return startstop;
-	}
-
-	if (drive->atapi_flags & IDE_AFLAG_DRQ_INTERRUPT) {
-		/* ok, next interrupt will be DMA interrupt */
-		if (drive->dma)
-			drive->waiting_for_dma = 1;
-	}
-
-	/* arm the interrupt handler */
-	ide_set_handler(drive, cdrom_newpc_intr, rq->timeout, ide_cd_expiry);
-
-	/* ATAPI commands get padded out to 12 bytes minimum */
-	cmd_len = COMMAND_SIZE(rq->cmd[0]);
-	if (cmd_len < ATAPI_MIN_CDB_BYTES)
-		cmd_len = ATAPI_MIN_CDB_BYTES;
-
-	/* start the DMA if need be */
-	if (drive->dma)
-		hwif->dma_ops->dma_start(drive);
-
-	/* send the command to the device */
-	hwif->tp_ops->output_data(drive, NULL, rq->cmd, cmd_len);
-
-	return ide_started;
-}
-
 /*
  * Check the contents of the interrupt reason register from the cdrom
  * and attempt to recover if there are problems.  Returns  0 if everything's
@@ -1174,7 +1081,7 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
 		return ide_stopped;
 	}
 
-	return cdrom_start_packet_command(drive);
+	return ide_issue_pc(drive);
 }
 
 /*
@@ -2072,6 +1979,7 @@ static int ide_cd_probe(ide_drive_t *drive)
 	}
 
 	drive->debug_mask = debug_mask;
+	drive->irq_handler = cdrom_newpc_intr;
 
 	info = kzalloc(sizeof(struct cdrom_info), GFP_KERNEL);
 	if (info == NULL) {
diff --git a/include/linux/ide.h b/include/linux/ide.h
index 9f6fe1f..1cb460f 100644
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -654,6 +654,8 @@ struct ide_drive_s {
 	int  (*pc_io_buffers)(struct ide_drive_s *, struct ide_atapi_pc *,
 			      unsigned int, int);
 
+	ide_startstop_t (*irq_handler)(struct ide_drive_s *);
+
 	unsigned long atapi_flags;
 
 	struct ide_atapi_pc request_sense_pc;
-- 
1.6.0.4

-- 
Regards/Gruss,
    Boris.