| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20071203.175212.52162634.k-ueda@ct.jp.nec.com>
Date: Mon, 03 Dec 2007 17:52:12 -0500 (EST)
From: Kiyoshi Ueda <k-ueda@...jp.nec.com>
To: bzolnier@...il.com
Cc: jens.axboe@...cle.com, bharrosh@...asas.com,
linux-kernel@...r.kernel.org, linux-scsi@...r.kernel.org,
linux-ide@...r.kernel.org, dm-devel@...hat.com,
j-nomura@...jp.nec.com, k-ueda@...jp.nec.com
Subject: Re: [PATCH 26/28] blk_end_request: changing ide-cd (take 3)
Hi Bartlomiej,
On Sat, 1 Dec 2007 23:42:51 +0100, Bartlomiej Zolnierkiewicz <bzolnier@...il.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@...jp.nec.com>
> > Signed-off-by: Jun'ichi Nomura <j-nomura@...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@...cle.com
> Cc: bharrosh@...asas.com
> Cc: Kiyoshi Ueda <k-ueda@...jp.nec.com
> Cc: Jun'ichi Nomura <j-nomura@...jp.nec.com>
> Cc: Tejun Heo <htejun@...il.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@...il.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@...jp.nec.com>
Signed-off-by: Jun'ichi Nomura <j-nomura@...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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists