2008-12-18 07:41:42

by Borislav Petkov

[permalink] [raw]
Subject: [PATCH 0/8] 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(-)


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


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

2008-12-19 21:19:55

by Borislav Petkov

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

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.

2008-12-20 06:29:01

by Borislav Petkov

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

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 <[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(-)

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.

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


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!

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

On Thursday 18 December 2008, Borislav Petkov wrote:
> 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(-)
>
> 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);

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

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 <[email protected]>> ---> 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 (especiallysince it goes away in patch #9/8) and not doing so allows to see changesto 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 passcdrom_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(-)
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); } /*????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2008-12-26 13:46:19

by Borislav Petkov

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

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 <[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(-)

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.

2008-12-26 13:46:49

by Borislav Petkov

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

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

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.

2008-12-26 13:54:27

by Borislav Petkov

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

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

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.

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

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

2008-12-18 07:41:28

by Borislav Petkov

[permalink] [raw]
Subject: [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(-)

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

2008-12-18 07:41:57

by Borislav Petkov

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

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

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

2008-12-18 07:42:58

by Borislav Petkov

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

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

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

2008-12-18 07:43:23

by Borislav Petkov

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

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

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

2008-12-18 07:43:39

by Borislav Petkov

[permalink] [raw]
Subject: [PATCH 7/8] ide-atapi: put the rest of non-ide-cd code into the else-clause of 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 | 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

2008-12-18 07:42:39

by Borislav Petkov

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

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

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

2008-12-18 07:42:23

by Borislav Petkov

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

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

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

2008-12-18 07:43:56

by Borislav Petkov

[permalink] [raw]
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]>
---
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