This patch converts ide-cd (cdrom_newpc_intr()) to use blk_end_request().
ide-cd (cdrom_newpc_intr()) has some tricky behaviors below which
need to use blk_end_request_callback().
Needs to:
1. call post_transform_command() to modify request contents
2. wait completing request until DRQ_STAT is cleared
after end_that_request_first() and before end_that_request_last().
As for the second one, ide-cd will wait for the interrupt from device.
So blk_end_request_callback() has to return without completing request
even if no leftover in the request.
ide-cd uses a dummy callback function, which just returns value '1',
to tell blk_end_request_callback() about that.
Signed-off-by: Kiyoshi Ueda <[email protected]>
Signed-off-by: Jun'ichi Nomura <[email protected]>
---
drivers/ide/ide-cd.c | 78 +++++++++++++++++++++++++++++++++++++++------------
1 files changed, 61 insertions(+), 17 deletions(-)
Index: 2.6.24-rc3-mm2/drivers/ide/ide-cd.c
===================================================================
--- 2.6.24-rc3-mm2.orig/drivers/ide/ide-cd.c
+++ 2.6.24-rc3-mm2/drivers/ide/ide-cd.c
@@ -1669,6 +1669,37 @@ static void post_transform_command(struc
}
}
+/*
+ * Called from blk_end_request_callback() after the data of the request
+ * is completed and before the request is completed.
+ */
+static int cdrom_newpc_intr_dma_cb(struct request *rq)
+{
+ ide_drive_t *drive = rq->q->queuedata;
+ spinlock_t *ide_lock = rq->q->queue_lock;
+ unsigned long flags = 0UL;
+
+ rq->data_len = 0;
+ post_transform_command(rq);
+
+ spin_lock_irqsave(ide_lock, flags);
+ HWGROUP(drive)->rq = NULL;
+ spin_unlock_irqrestore(ide_lock, flags);
+
+ return 0;
+}
+
+/*
+ * Called from blk_end_request_callback() after the data of the request
+ * is completed and before the request is completed.
+ * By returning value '1', blk_end_request_callback() returns immediately
+ * without completing the request.
+ */
+static int cdrom_newpc_intr_dummy_cb(struct request *rq)
+{
+ return 1;
+}
+
typedef void (xfer_func_t)(ide_drive_t *, void *, u32);
/*
@@ -1707,9 +1738,16 @@ static ide_startstop_t cdrom_newpc_intr(
return ide_error(drive, "dma error", stat);
}
- end_that_request_chunk(rq, 1, rq->data_len);
- rq->data_len = 0;
- goto end_request;
+ /*
+ * post_transform_command() needs to be called after
+ * the data of the request is completed, since it may
+ * modify the data area of the request.
+ * So use the callback special feature of blk_end_request().
+ */
+ if (blk_end_request_callback(rq, 1, rq->data_len,
+ cdrom_newpc_intr_dma_cb))
+ BUG();
+ return ide_stopped;
}
/*
@@ -1727,8 +1765,18 @@ static ide_startstop_t cdrom_newpc_intr(
/*
* If DRQ is clear, the command has completed.
*/
- if ((stat & DRQ_STAT) == 0)
- goto end_request;
+ if ((stat & DRQ_STAT) == 0) {
+ if (!rq->data_len)
+ post_transform_command(rq);
+
+ spin_lock_irqsave(&ide_lock, flags);
+ if (__blk_end_request(rq, 1, 0))
+ BUG();
+ HWGROUP(drive)->rq = NULL;
+ spin_unlock_irqrestore(&ide_lock, flags);
+
+ return ide_stopped;
+ }
/*
* check which way to transfer data
@@ -1781,7 +1829,14 @@ static ide_startstop_t cdrom_newpc_intr(
rq->data_len -= blen;
if (rq->bio)
- end_that_request_chunk(rq, 1, blen);
+ /*
+ * The request can't be completed until DRQ is cleared.
+ * So complete the data, but don't complete the request
+ * using the dummy function for the callback feature
+ * of blk_end_request().
+ */
+ blk_end_request_callback(rq, 1, blen,
+ cdrom_newpc_intr_dummy_cb);
else
rq->data += blen;
}
@@ -1802,17 +1857,6 @@ static ide_startstop_t cdrom_newpc_intr(
ide_set_handler(drive, cdrom_newpc_intr, rq->timeout, NULL);
return ide_started;
-
-end_request:
- if (!rq->data_len)
- post_transform_command(rq);
-
- spin_lock_irqsave(&ide_lock, flags);
- blkdev_dequeue_request(rq);
- end_that_request_last(rq, 1);
- HWGROUP(drive)->rq = NULL;
- spin_unlock_irqrestore(&ide_lock, flags);
- return ide_stopped;
}
static ide_startstop_t cdrom_write_intr(ide_drive_t *drive)
Hi,
On Saturday 01 December 2007, Kiyoshi Ueda wrote:
> This patch converts ide-cd (cdrom_newpc_intr()) to use blk_end_request().
>
> ide-cd (cdrom_newpc_intr()) has some tricky behaviors below which
> need to use blk_end_request_callback().
> Needs to:
> 1. call post_transform_command() to modify request contents
Seems like post_transform_command() call can be removed (patch below).
> 2. wait completing request until DRQ_STAT is cleared
Would be great if somebody convert cdrom_newpc_intr() to use scatterlists
also for PIO transfers (ide_pio_sector() in ide-taskfile.c should serve
as a good starting base to see how to do PIO transfers using scatterlists)
so we could get rid of partial request completions in cdrom_newpc_intr()
and just fully complete request when the transfer is done. Shouldn't be
difficult but I guess that we can live with blk_end_request_callback() for
the time being...
> after end_that_request_first() and before end_that_request_last().
>
> As for the second one, ide-cd will wait for the interrupt from device.
> So blk_end_request_callback() has to return without completing request
> even if no leftover in the request.
> ide-cd uses a dummy callback function, which just returns value '1',
> to tell blk_end_request_callback() about that.
>
> Signed-off-by: Kiyoshi Ueda <[email protected]>
> Signed-off-by: Jun'ichi Nomura <[email protected]>
[PATCH] ide-cd: remove dead post_transform_command()
post_transform_command() call in cdrom_newpc_intr() has no effect because
it is done after the request has already been fully completed (rq->bio and
rq->data are always NULL). It was verified to be true regardless whether
INQUIRY command is using DMA or PIO to transfer data (by using modified
Tejun Heo's test-shortsg.c utility and adding a few printk()-s to ide-cd).
This was uncovered thanks to the "blk_end_request: full I/O completion
handler (take 3)" patch series from Kiyoshi Ueda.
Cc: [email protected]
Cc: [email protected]
Cc: Kiyoshi Ueda <[email protected]
Cc: Jun'ichi Nomura <[email protected]>
Cc: Tejun Heo <[email protected]>
Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
---
Kiyoshi: please rebase your patch on top of this one (I'll send
it to Linus in the next IDE update), should make your patch a bit
simpler.
Tejun: you had really good timing with posting test-shortsg.c
(it saved me some time coding user-space SG_IO tester), thanks!
drivers/ide/ide-cd.c | 28 ----------------------------
1 file changed, 28 deletions(-)
Index: b/drivers/ide/ide-cd.c
===================================================================
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -1650,31 +1650,6 @@ static int cdrom_write_check_ireason(ide
return 1;
}
-static void post_transform_command(struct request *req)
-{
- u8 *c = req->cmd;
- char *ibuf;
-
- if (!blk_pc_request(req))
- return;
-
- if (req->bio)
- ibuf = bio_data(req->bio);
- else
- ibuf = req->data;
-
- if (!ibuf)
- return;
-
- /*
- * set ansi-revision and response data as atapi
- */
- if (c[0] == GPCMD_INQUIRY) {
- ibuf[2] |= 2;
- ibuf[3] = (ibuf[3] & 0xf0) | 2;
- }
-}
-
typedef void (xfer_func_t)(ide_drive_t *, void *, u32);
/*
@@ -1810,9 +1785,6 @@ static ide_startstop_t cdrom_newpc_intr(
return ide_started;
end_request:
- if (!rq->data_len)
- post_transform_command(rq);
-
spin_lock_irqsave(&ide_lock, flags);
blkdev_dequeue_request(rq);
end_that_request_last(rq, 1);
Bartlomiej Zolnierkiewicz wrote:
> [PATCH] ide-cd: remove dead post_transform_command()
> post_transform_command() call in cdrom_newpc_intr() has no effect because
> it is done after the request has already been fully completed (rq->bio and
> rq->data are always NULL). It was verified to be true regardless whether
> INQUIRY command is using DMA or PIO to transfer data (by using modified
> Tejun Heo's test-shortsg.c utility and adding a few printk()-s to ide-cd).
> This was uncovered thanks to the "blk_end_request: full I/O completion
> handler (take 3)" patch series from Kiyoshi Ueda.
> Cc: [email protected]
> Cc: [email protected]
> Cc: Kiyoshi Ueda <[email protected]
> Cc: Jun'ichi Nomura <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
Acked-by: Sergei Shtylyov <[email protected]>
MBR, Sergei
Hi Bartlomiej,
On Sat, 1 Dec 2007 23:42:51 +0100, Bartlomiej Zolnierkiewicz <[email protected]> wrote:
> On Saturday 01 December 2007, Kiyoshi Ueda wrote:
> > This patch converts ide-cd (cdrom_newpc_intr()) to use blk_end_request().
> >
> > ide-cd (cdrom_newpc_intr()) has some tricky behaviors below which
> > need to use blk_end_request_callback().
> > Needs to:
> > 1. call post_transform_command() to modify request contents
>
> Seems like post_transform_command() call can be removed (patch below).
>
> > 2. wait completing request until DRQ_STAT is cleared
>
> Would be great if somebody convert cdrom_newpc_intr() to use scatterlists
> also for PIO transfers (ide_pio_sector() in ide-taskfile.c should serve
> as a good starting base to see how to do PIO transfers using scatterlists)
> so we could get rid of partial request completions in cdrom_newpc_intr()
> and just fully complete request when the transfer is done. Shouldn't be
> difficult but I guess that we can live with blk_end_request_callback() for
> the time being...
>
> > after end_that_request_first() and before end_that_request_last().
> >
> > As for the second one, ide-cd will wait for the interrupt from device.
> > So blk_end_request_callback() has to return without completing request
> > even if no leftover in the request.
> > ide-cd uses a dummy callback function, which just returns value '1',
> > to tell blk_end_request_callback() about that.
> >
> > Signed-off-by: Kiyoshi Ueda <[email protected]>
> > Signed-off-by: Jun'ichi Nomura <[email protected]>
>
> [PATCH] ide-cd: remove dead post_transform_command()
>
> post_transform_command() call in cdrom_newpc_intr() has no effect because
> it is done after the request has already been fully completed (rq->bio and
> rq->data are always NULL). It was verified to be true regardless whether
> INQUIRY command is using DMA or PIO to transfer data (by using modified
> Tejun Heo's test-shortsg.c utility and adding a few printk()-s to ide-cd).
>
> This was uncovered thanks to the "blk_end_request: full I/O completion
> handler (take 3)" patch series from Kiyoshi Ueda.
>
> Cc: [email protected]
> Cc: [email protected]
> Cc: Kiyoshi Ueda <[email protected]
> Cc: Jun'ichi Nomura <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
> ---
> Kiyoshi: please rebase your patch on top of this one (I'll send
> it to Linus in the next IDE update), should make your patch a bit
> simpler.
>
> Tejun: you had really good timing with posting test-shortsg.c
> (it saved me some time coding user-space SG_IO tester), thanks!
>
> drivers/ide/ide-cd.c | 28 ----------------------------
> 1 file changed, 28 deletions(-)
>
> Index: b/drivers/ide/ide-cd.c
> ===================================================================
> --- a/drivers/ide/ide-cd.c
> +++ b/drivers/ide/ide-cd.c
> @@ -1650,31 +1650,6 @@ static int cdrom_write_check_ireason(ide
> return 1;
> }
>
> -static void post_transform_command(struct request *req)
> -{
> - u8 *c = req->cmd;
> - char *ibuf;
> -
> - if (!blk_pc_request(req))
> - return;
> -
> - if (req->bio)
> - ibuf = bio_data(req->bio);
> - else
> - ibuf = req->data;
> -
> - if (!ibuf)
> - return;
> -
> - /*
> - * set ansi-revision and response data as atapi
> - */
> - if (c[0] == GPCMD_INQUIRY) {
> - ibuf[2] |= 2;
> - ibuf[3] = (ibuf[3] & 0xf0) | 2;
> - }
> -}
> -
> typedef void (xfer_func_t)(ide_drive_t *, void *, u32);
>
> /*
> @@ -1810,9 +1785,6 @@ static ide_startstop_t cdrom_newpc_intr(
> return ide_started;
>
> end_request:
> - if (!rq->data_len)
> - post_transform_command(rq);
> -
> spin_lock_irqsave(&ide_lock, flags);
> blkdev_dequeue_request(rq);
> end_that_request_last(rq, 1);
Thank you for the comments.
I rebased my patch on top of 2.6.24-rc3-mm2 + the patch to remove
post_transform_command().
As a result, one callback function for DMA mode has been removed.
What do you think about the patch below?
Subject: [PATCH 26/28] blk_end_request: changing ide-cd (take 3)
This patch converts ide-cd (cdrom_newpc_intr()) to use blk_end_request
interfaces.
In PIO mode, ide-cd (cdrom_newpc_intr()) needs to defer
end_that_request_last() until the device clears DRQ_STAT and raises
an interrupt after end_that_request_first().
So blk_end_request() has to return without completing request
even if no leftover in the request.
ide-cd uses blk_end_request_callback() and a dummy callback function,
which just returns value '1', to tell blk_end_request_callback()
about that.
Signed-off-by: Kiyoshi Ueda <[email protected]>
Signed-off-by: Jun'ichi Nomura <[email protected]>
---
drivers/ide/ide-cd.c | 49 +++++++++++++++++++++++++++++++++++--------------
1 files changed, 35 insertions(+), 14 deletions(-)
Index: 2.6.24-rc3-mm2/drivers/ide/ide-cd.c
===================================================================
--- 2.6.24-rc3-mm2.orig/drivers/ide/ide-cd.c
+++ 2.6.24-rc3-mm2/drivers/ide/ide-cd.c
@@ -1644,6 +1644,17 @@ static int cdrom_write_check_ireason(ide
return 1;
}
+/*
+ * Called from blk_end_request_callback() after the data of the request
+ * is completed and before the request is completed.
+ * By returning value '1', blk_end_request_callback() returns immediately
+ * without completing the request.
+ */
+static int cdrom_newpc_intr_dummy_cb(struct request *rq)
+{
+ return 1;
+}
+
typedef void (xfer_func_t)(ide_drive_t *, void *, u32);
/*
@@ -1682,9 +1693,13 @@ static ide_startstop_t cdrom_newpc_intr(
return ide_error(drive, "dma error", stat);
}
- end_that_request_chunk(rq, 1, rq->data_len);
- rq->data_len = 0;
- goto end_request;
+ spin_lock_irqsave(&ide_lock, flags);
+ if (__blk_end_request(rq, 1, rq->data_len))
+ BUG();
+ HWGROUP(drive)->rq = NULL;
+ spin_unlock_irqrestore(&ide_lock, flags);
+
+ return ide_stopped;
}
/*
@@ -1702,8 +1717,15 @@ static ide_startstop_t cdrom_newpc_intr(
/*
* If DRQ is clear, the command has completed.
*/
- if ((stat & DRQ_STAT) == 0)
- goto end_request;
+ if ((stat & DRQ_STAT) == 0) {
+ spin_lock_irqsave(&ide_lock, flags);
+ if (__blk_end_request(rq, 1, 0))
+ BUG();
+ HWGROUP(drive)->rq = NULL;
+ spin_unlock_irqrestore(&ide_lock, flags);
+
+ return ide_stopped;
+ }
/*
* check which way to transfer data
@@ -1756,7 +1778,14 @@ static ide_startstop_t cdrom_newpc_intr(
rq->data_len -= blen;
if (rq->bio)
- end_that_request_chunk(rq, 1, blen);
+ /*
+ * The request can't be completed until DRQ is cleared.
+ * So complete the data, but don't complete the request
+ * using the dummy function for the callback feature
+ * of blk_end_request_callback().
+ */
+ blk_end_request_callback(rq, 1, blen,
+ cdrom_newpc_intr_dummy_cb);
else
rq->data += blen;
}
@@ -1777,14 +1806,6 @@ static ide_startstop_t cdrom_newpc_intr(
ide_set_handler(drive, cdrom_newpc_intr, rq->timeout, NULL);
return ide_started;
-
-end_request:
- spin_lock_irqsave(&ide_lock, flags);
- blkdev_dequeue_request(rq);
- end_that_request_last(rq, 1);
- HWGROUP(drive)->rq = NULL;
- spin_unlock_irqrestore(&ide_lock, flags);
- return ide_stopped;
}
static ide_startstop_t cdrom_write_intr(ide_drive_t *drive)
Thanks,
Kiyoshi Ueda
Hi,
On Monday 03 December 2007, Kiyoshi Ueda wrote:
[...]
> Thank you for the comments.
> I rebased my patch on top of 2.6.24-rc3-mm2 + the patch to remove
> post_transform_command().
>
> As a result, one callback function for DMA mode has been removed.
> What do you think about the patch below?
>
>
>
> Subject: [PATCH 26/28] blk_end_request: changing ide-cd (take 3)
>
>
> This patch converts ide-cd (cdrom_newpc_intr()) to use blk_end_request
> interfaces.
>
> In PIO mode, ide-cd (cdrom_newpc_intr()) needs to defer
> end_that_request_last() until the device clears DRQ_STAT and raises
> an interrupt after end_that_request_first().
> So blk_end_request() has to return without completing request
> even if no leftover in the request.
>
> ide-cd uses blk_end_request_callback() and a dummy callback function,
> which just returns value '1', to tell blk_end_request_callback()
> about that.
>
> Signed-off-by: Kiyoshi Ueda <[email protected]>
> Signed-off-by: Jun'ichi Nomura <[email protected]>
Looks good, thanks!
Acked-by: Bartlomiej Zolnierkiewicz <[email protected]>