Skip to contentSkip to conversation

ide-cd: first conversion batch

PATCH In review Borislav Petkov 2 participants 19 messages Dec 18, 2008 – Dec 29, 2008 libata

lore
Share & tools
lore

Borislav Petkov

author
#0 lore raw

cover ide-cd: first conversion batch

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(-)
author
#1 lore raw

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

There should be no functionality change resulting from this patch.

Signed-off-by Borislav Petkov <[email protected]>
---
 drivers/ide/ide-atapi.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)
drivers/ide/ide-atapi.c +13 -1
index d412bd2..5c143b4 100644
@@ -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;
 }
Signature · 1 line
1.6.0.4
author
#2 lore raw

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

In reply to Borislav Petkov

There should be no functionality change resulting from this patch.

Signed-off-by Borislav Petkov <[email protected]>
---
 drivers/ide/ide-atapi.c |   35 ++++++++++++++++++++---------------
 1 files changed, 20 insertions(+), 15 deletions(-)
drivers/ide/ide-atapi.c +20 -15
index 5c143b4..5295b26 100644
@@ -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);
Signature · 1 line
1.6.0.4
#3 lore raw

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

Quoted 42 lines · Borislav Petkov

There should be no functionality change resulting from this patch.

Signed-off-by Borislav Petkov <[email protected]>
---
 drivers/ide/ide-atapi.c |   35 ++++++++++++++++++++---------------
 1 files changed, 20 insertions(+), 15 deletions(-)
drivers/ide/ide-atapi.c +20 -1
index 5c143b4..5295b26 100644
@@ -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);

drivers/ide/ide-atapi.c static ide_startstop_t ide_transfer_pc(ide_drive_t *drive) · line 548

Quoted 2 lines
drivers/ide/ide-atapi.c +0 -0
@@ -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. ]

Quoted 16 lines
drivers/ide/ide-atapi.c +0 -14
-	/*
-	 * 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);
author
#4 lore raw

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

In reply to Borislav Petkov

There should be no functionality change resulting from this patch.

Signed-off-by Borislav Petkov <[email protected]>
---
 drivers/ide/ide-atapi.c |   36 ++++++++++++++++++++----------------
 1 files changed, 20 insertions(+), 16 deletions(-)
drivers/ide/ide-atapi.c +20 -16
index 5295b26..2577782 100644
@@ -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);
Signature · 1 line
1.6.0.4
author
#5 lore raw

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

In reply to Borislav Petkov

There should be no functionality change resulting from this patch.

Signed-off-by Borislav Petkov <[email protected]>
---
 drivers/ide/ide-cd.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
drivers/ide/ide-cd.c +5 -5
index 105e4d8..34981f5 100644
@@ -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);
 }
 
 /*
Signature · 1 line
1.6.0.4
author
#6 lore raw

PATCH 5/8 ide-cd: remove handler wrappers

In reply to 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 <[email protected]>
---
 drivers/ide/ide-cd.c |  121 ++++++++++++++++++++------------------------------
 1 files changed, 48 insertions(+), 73 deletions(-)
drivers/ide/ide-cd.c +48 -73
index 34981f5..f3c7cc3 100644
@@ -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);
 }
 
 /*
Signature · 1 line
1.6.0.4
#7 lore raw

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

Quoted 206 lines · 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 <[email protected]>
---
 drivers/ide/ide-cd.c |  121 ++++++++++++++++++++------------------------------
 1 files changed, 48 insertions(+), 73 deletions(-)
drivers/ide/ide-cd.c +47 -72
index 34981f5..f3c7cc3 100644
@@ -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;

drivers/ide/ide-cd.c static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq, · line 1189

Quoted 6 lines
drivers/ide/ide-cd.c +1 -1
@@ -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 <[email protected]>
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 <[email protected]>
[bart: don't move cdrom_start_packet_command() around, remove newlines]
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/ide-cd.c |   49 +++++++++++--------------------------------------
 1 file changed, 11 insertions(+), 38 deletions(-)
drivers/ide/ide-cd.c +11 -38
Index: 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);
 }
 
 /*

author
#8 lore raw

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

In reply to Borislav Petkov

There should be no functionality change resulting from this patch.

Signed-off-by Borislav Petkov <[email protected]>
---
 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(-)
drivers/ide/ide-atapi.c +6 -1
index 2577782..dbed09c 100644
@@ -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);
drivers/ide/ide-floppy.c +1 -1
index fdec729..0a48e2d 100644
@@ -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)
drivers/ide/ide-tape.c +1 -1
index ac9e29a..5d2aa22 100644
@@ -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. */
include/linux/ide.h +1 -1
index ad57a44..db5ef8a 100644
@@ -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 *);
Signature · 1 line
1.6.0.4
author
#9 lore raw

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

In reply to Borislav Petkov

There should be no functionality change resulting from this patch.

Signed-off-by Borislav Petkov <[email protected]>
---
 drivers/ide/ide-atapi.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)
drivers/ide/ide-atapi.c +8 -8
index dbed09c..f3dea66 100644
@@ -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 */
Signature · 1 line
1.6.0.4
author
#10 lore raw

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

In reply to Borislav Petkov

There should be no functionality change resulting from this patch.

Signed-off-by Borislav Petkov <[email protected]>
---
 drivers/ide/ide-atapi.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)
drivers/ide/ide-atapi.c +11 -4
index f3dea66..5b92e0a 100644
@@ -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 */
Signature · 1 line
1.6.0.4
#11 lore raw
In reply to Borislav Petkov

Hi,

Quoted 5 lines · 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

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

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

Quoted 4 lines

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

author
#12 lore raw
Quoted 6 lines · Bartlomiej Zolnierkiewicz

Hi,

Quoted 2 lines · Borislav Petkov

Hi Bart,

Quoted 7 lines
Quoted 3 lines

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 :)

Quoted 8 lines
Quoted 4 lines

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... ?

Signature · 2 lines
Regards/Gruss,
    Boris.
author
#13 lore raw
In reply to Bartlomiej Zolnierkiewicz

Hi,

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

[.. ]

Quoted 3 lines

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 <[email protected]>
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 <[email protected]>
---
 drivers/ide/ide-atapi.c |    6 ++-
 drivers/ide/ide-cd.c    |  102 +----------------------------------------------
 include/linux/ide.h     |    2 +
 3 files changed, 9 insertions(+), 101 deletions(-)
drivers/ide/ide-atapi.c +5 -1
index 5b92e0a..0e96da5 100644
@@ -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;
drivers/ide/ide-cd.c +2 -100

120 lines not on this page · raw message

include/linux/ide.h +2 -0
index db5ef8a..2fa5539 100644
@@ -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;
Signature · 6 lines
1.6.0.4


-- 
Regards/Gruss,
    Boris.
#14 lore raw

Hi,

Quoted 4 lines · Borislav Petkov

Hi,

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

Quoted 8 lines

[.. ]

Quoted 3 lines

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. ]

Quoted 4 lines

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!

author
#15 lore raw

Hi Bart,

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

[.. ]

Quoted 5 lines
Quoted 4 lines

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

Quoted 7 lines

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 <[email protected]>
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 <[email protected]>
---
 drivers/ide/ide-cd.c |   21 +++++++--------------
 1 files changed, 7 insertions(+), 14 deletions(-)
drivers/ide/ide-cd.c +7 -14
index 6c7dd8f..1c1ba43 100644
@@ -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 */
Signature · 6 lines
1.6.0.4


-- 
Regards/Gruss,
    Boris.
#16 lore raw
Quoted 13 lines · Borislav Petkov

Hi Bart,

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

[.. ]

Quoted 8 lines
Quoted 4 lines

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).

Quoted 8 lines
Quoted 4 lines

[ 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

author
#17 lore raw
In reply to Bartlomiej Zolnierkiewicz

From: Borislav Petkov <[email protected]>
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 <[email protected]>
---
 drivers/ide/ide-cd.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
drivers/ide/ide-cd.c +3 -3
index 1c1ba43..2cb301d 100644
@@ -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;
 }
Signature · 5 lines
1.6.0.4

-- 
Regards/Gruss,
    Boris.
author
#18 lore raw
In reply to Bartlomiej Zolnierkiewicz

From: Borislav Petkov <[email protected]>
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 <[email protected]>
---
 drivers/ide/ide-atapi.c |    5 ++-
 drivers/ide/ide-cd.c    |   96 +----------------------------------------------
 include/linux/ide.h     |    2 +
 3 files changed, 8 insertions(+), 95 deletions(-)
drivers/ide/ide-atapi.c +4 -1
index d6a50d6..e96c012 100644
@@ -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)) {
drivers/ide/ide-cd.c +2 -94

114 lines not on this page · raw message

include/linux/ide.h +2 -0
index 9f6fe1f..1cb460f 100644
@@ -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;
Signature · 5 lines
1.6.0.4

-- 
Regards/Gruss,
    Boris.