linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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).