Re: [PATCH 5/6] ide: remove ide_execute_pkt_cmd()

From: Sergei Shtylyov
Date: Wed Feb 11 2009 - 08:22:34 EST

Next message: Ingo Molnar: "[PATCH] pci, x86, acpi: fix early_ioremap() leak"
Previous message: Frederic Weisbecker: "Re: [RFC PATCH 2/2] trace: splice support for tracing_pipe"
In reply to: Borislav Petkov: "Re: [PATCH 5/6] ide: remove ide_execute_pkt_cmd()"
Next in thread: Borislav Petkov: "Re: [PATCH 5/6] ide: remove ide_execute_pkt_cmd()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hello.

Borislav Petkov wrote:

From: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>
Subject: [PATCH] ide: remove ide_execute_pkt_cmd()

* Pass command structure to ide_execute_command() and skip
__ide_set_handler() for ATAPI protocols.

* Convert ide_issue_pc() to always use ide_execute_command()
and remove no longer needed ide_execute_pkt_cmd().

There should be no functional changes caused by this patch.

Cc: Borislav Petkov <petkovbb@xxxxxxxxx>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>
---
drivers/ide/ide-atapi.c | 15 +++++++--------
drivers/ide/ide-iops.c | 23 ++++++-----------------
drivers/ide/ide-taskfile.c | 6 ++----
include/linux/ide.h | 5 ++---
4 files changed, 17 insertions(+), 32 deletions(-)

Index: b/drivers/ide/ide-atapi.c
===================================================================
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -481,6 +481,7 @@ static void ide_init_packet_cmd(struct i
cmd->protocol = dma ? ATAPI_PROT_DMA : ATAPI_PROT_PIO;
cmd->tf_flags |= IDE_TFLAG_OUT_LBAH | IDE_TFLAG_OUT_LBAM |
IDE_TFLAG_OUT_FEATURE | tf_flags;
+ cmd->tf.command = ATA_CMD_PACKET;
cmd->tf.feature = dma; /* Use PIO/DMA */
cmd->tf.lbam = bcount & 0xff;
cmd->tf.lbah = (bcount >> 8) & 0xff;
@@ -626,6 +627,7 @@ ide_startstop_t ide_issue_pc(ide_drive_t
unsigned int timeout;
u32 tf_flags;
u16 bcount;
+ u8 drq_int = !!(drive->atapi_flags & IDE_AFLAG_DRQ_INTERRUPT);

How about we finally add those check macros in block layer fashion like
blk_pc_request et al and do

#define drv_can_drq_interrupt(drive) ((drive)->atapi_flags & IDE_AFLAG_DRQ_INTERRUPT)

I suppose it's for the devices that interrupt on packet DRQ? Then it's hardly a good name because it's not like this is some optional capability.

...

or similar instead of wasting stack space?

It doesn't necessarily waste stack space. Haven't you heard about compiler putting local vairables into registers?

It'll also read better in the if() check:

if (drv_can_irq_interrupt(drive)) { ...

It's faster to checj a local variable than to dereference drv several times -- unless gcc optimizes that away (by creating an implicit local variable :-).

If it's ok with you, I'll whip up patch(es) later.

if (dev_is_idecd(drive)) {
tf_flags = IDE_TFLAG_OUT_NSECT | IDE_TFLAG_OUT_LBAL;
@@ -675,17 +677,14 @@ ide_startstop_t ide_issue_pc(ide_drive_t

Don't leave uncommented fragment behind please.

MBR, Sergei

(void)do_rw_taskfile(drive, cmd);
- /* Issue the packet command */
- if (drive->atapi_flags & IDE_AFLAG_DRQ_INTERRUPT) {
+ if (drq_int) {
if (drive->dma)
drive->waiting_for_dma = 0;
hwif->expiry = expiry;
- ide_execute_command(drive, ATA_CMD_PACKET, ide_transfer_pc,
- timeout);
- return ide_started;
- } else {
- ide_execute_pkt_cmd(drive);
- return ide_transfer_pc(drive);
}
+
+ ide_execute_command(drive, cmd, ide_transfer_pc, timeout);
+
+ return drq_int ? ide_started : ide_transfer_pc(drive);
}
EXPORT_SYMBOL_GPL(ide_issue_pc);
Index: b/drivers/ide/ide-iops.c
===================================================================
--- a/drivers/ide/ide-iops.c
+++ b/drivers/ide/ide-iops.c
@@ -451,7 +451,7 @@ EXPORT_SYMBOL(ide_set_handler);
/**
* ide_execute_command - execute an IDE command
* @drive: IDE drive to issue the command against
- * @command: command byte to write
+ * @cmd: command
* @handler: handler for next phase
* @timeout: timeout for command
*
@@ -461,15 +461,16 @@ EXPORT_SYMBOL(ide_set_handler);
* should go via this function or do equivalent locking.
*/
-void ide_execute_command(ide_drive_t *drive, u8 cmd, ide_handler_t *handler,
- unsigned timeout)
+void ide_execute_command(ide_drive_t *drive, struct ide_cmd *cmd,
+ ide_handler_t *handler, unsigned timeout)
{
ide_hwif_t *hwif = drive->hwif;
unsigned long flags;
spin_lock_irqsave(&hwif->lock, flags);
- __ide_set_handler(drive, handler, timeout);
- hwif->tp_ops->exec_command(hwif, cmd);
+ if (cmd->protocol != ATAPI_PROT_DMA && cmd->protocol != ATAPI_PROT_PIO)
+ __ide_set_handler(drive, handler, timeout);
+ hwif->tp_ops->exec_command(hwif, cmd->tf.command);
/*
* Drive takes 400nS to respond, we must avoid the IRQ being
* serviced before that.
@@ -480,18 +481,6 @@ void ide_execute_command(ide_drive_t *dr
spin_unlock_irqrestore(&hwif->lock, flags);
}
-void ide_execute_pkt_cmd(ide_drive_t *drive)
-{
- ide_hwif_t *hwif = drive->hwif;
- unsigned long flags;
-
- spin_lock_irqsave(&hwif->lock, flags);
- hwif->tp_ops->exec_command(hwif, ATA_CMD_PACKET);
- ndelay(400);
- spin_unlock_irqrestore(&hwif->lock, flags);
-}
-EXPORT_SYMBOL_GPL(ide_execute_pkt_cmd);
-
/*
* ide_wait_not_busy() waits for the currently selected device on the hwif
* to report a non-busy status, see comments in ide_probe_port().
Index: b/drivers/ide/ide-taskfile.c
===================================================================
--- a/drivers/ide/ide-taskfile.c
+++ b/drivers/ide/ide-taskfile.c
@@ -97,8 +97,7 @@ ide_startstop_t do_rw_taskfile(ide_drive
case ATA_PROT_NODATA:
if (handler == NULL)
handler = task_no_data_intr;
- ide_execute_command(drive, tf->command, handler,
- WAIT_WORSTCASE);
+ ide_execute_command(drive, cmd, handler, WAIT_WORSTCASE);
return ide_started;
case ATA_PROT_DMA:
if ((drive->dev_flags & IDE_DFLAG_USING_DMA) == 0 ||
@@ -106,8 +105,7 @@ ide_startstop_t do_rw_taskfile(ide_drive
dma_ops->dma_setup(drive, cmd))
return ide_stopped;
hwif->expiry = dma_ops->dma_expiry;
- ide_execute_command(drive, tf->command, ide_dma_intr,
- 2 * WAIT_CMD);
+ ide_execute_command(drive, cmd, ide_dma_intr, 2 * WAIT_CMD);
dma_ops->dma_start(drive);
default:
return ide_started;
Index: b/include/linux/ide.h
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -1135,9 +1135,8 @@ void ide_kill_rq(ide_drive_t *, struct r
void __ide_set_handler(ide_drive_t *, ide_handler_t *, unsigned int);
void ide_set_handler(ide_drive_t *, ide_handler_t *, unsigned int);
-void ide_execute_command(ide_drive_t *, u8, ide_handler_t *, unsigned int);
-
-void ide_execute_pkt_cmd(ide_drive_t *);
+void ide_execute_command(ide_drive_t *, struct ide_cmd *, ide_handler_t *,
+ unsigned int);
void ide_pad_transfer(ide_drive_t *, int, int);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Ingo Molnar: "[PATCH] pci, x86, acpi: fix early_ioremap() leak"
Previous message: Frederic Weisbecker: "Re: [RFC PATCH 2/2] trace: splice support for tracing_pipe"
In reply to: Borislav Petkov: "Re: [PATCH 5/6] ide: remove ide_execute_pkt_cmd()"
Next in thread: Borislav Petkov: "Re: [PATCH 5/6] ide: remove ide_execute_pkt_cmd()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]