* [PATCH 26/28] blk_end_request: changing ide-cd (take 3) @ 2007-11-30 23:34 Kiyoshi Ueda 2007-12-01 22:42 ` Bartlomiej Zolnierkiewicz 0 siblings, 1 reply; 5+ messages in thread From: Kiyoshi Ueda @ 2007-11-30 23:34 UTC (permalink / raw) To: jens.axboe, bharrosh Cc: linux-kernel, linux-scsi, linux-ide, dm-devel, j-nomura, k-ueda 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 <k-ueda@ct.jp.nec.com> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com> --- 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) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 26/28] blk_end_request: changing ide-cd (take 3) 2007-11-30 23:34 [PATCH 26/28] blk_end_request: changing ide-cd (take 3) Kiyoshi Ueda @ 2007-12-01 22:42 ` Bartlomiej Zolnierkiewicz 2007-12-03 15:55 ` Sergei Shtylyov 2007-12-03 22:52 ` Kiyoshi Ueda 0 siblings, 2 replies; 5+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2007-12-01 22:42 UTC (permalink / raw) To: Kiyoshi Ueda Cc: jens.axboe, bharrosh, linux-kernel, linux-scsi, linux-ide, dm-devel, j-nomura 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 <k-ueda@ct.jp.nec.com> > Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com> [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: jens.axboe@oracle.com Cc: bharrosh@panasas.com Cc: Kiyoshi Ueda <k-ueda@ct.jp.nec.com Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com> Cc: Tejun Heo <htejun@gmail.com> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> --- 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); ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 26/28] blk_end_request: changing ide-cd (take 3) 2007-12-01 22:42 ` Bartlomiej Zolnierkiewicz @ 2007-12-03 15:55 ` Sergei Shtylyov 2007-12-03 22:52 ` Kiyoshi Ueda 1 sibling, 0 replies; 5+ messages in thread From: Sergei Shtylyov @ 2007-12-03 15:55 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Kiyoshi Ueda, jens.axboe, bharrosh, linux-kernel, linux-scsi, linux-ide, dm-devel, j-nomura 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: jens.axboe@oracle.com > Cc: bharrosh@panasas.com > Cc: Kiyoshi Ueda <k-ueda@ct.jp.nec.com > Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com> > Cc: Tejun Heo <htejun@gmail.com> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com> MBR, Sergei ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 26/28] blk_end_request: changing ide-cd (take 3) 2007-12-01 22:42 ` Bartlomiej Zolnierkiewicz 2007-12-03 15:55 ` Sergei Shtylyov @ 2007-12-03 22:52 ` Kiyoshi Ueda 2007-12-04 13:46 ` Bartlomiej Zolnierkiewicz 1 sibling, 1 reply; 5+ messages in thread From: Kiyoshi Ueda @ 2007-12-03 22:52 UTC (permalink / raw) To: bzolnier Cc: jens.axboe, bharrosh, linux-kernel, linux-scsi, linux-ide, dm-devel, j-nomura, k-ueda Hi Bartlomiej, On Sat, 1 Dec 2007 23:42:51 +0100, Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> 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 <k-ueda@ct.jp.nec.com> > > Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com> > > [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: jens.axboe@oracle.com > Cc: bharrosh@panasas.com > Cc: Kiyoshi Ueda <k-ueda@ct.jp.nec.com > Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com> > Cc: Tejun Heo <htejun@gmail.com> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > --- > 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 <k-ueda@ct.jp.nec.com> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com> --- 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 26/28] blk_end_request: changing ide-cd (take 3) 2007-12-03 22:52 ` Kiyoshi Ueda @ 2007-12-04 13:46 ` Bartlomiej Zolnierkiewicz 0 siblings, 0 replies; 5+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2007-12-04 13:46 UTC (permalink / raw) To: Kiyoshi Ueda Cc: jens.axboe, bharrosh, linux-kernel, linux-scsi, linux-ide, dm-devel, j-nomura 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 <k-ueda@ct.jp.nec.com> > Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com> Looks good, thanks! Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2007-12-04 13:58 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-11-30 23:34 [PATCH 26/28] blk_end_request: changing ide-cd (take 3) Kiyoshi Ueda 2007-12-01 22:42 ` Bartlomiej Zolnierkiewicz 2007-12-03 15:55 ` Sergei Shtylyov 2007-12-03 22:52 ` Kiyoshi Ueda 2007-12-04 13:46 ` Bartlomiej Zolnierkiewicz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).