Patchwork [6/6] ide: improve handling of Power Management requests

login
register
mail settings
Submitter Bartlomiej Zolnierkiewicz
Date June 23, 2009, 9:35 p.m.
Message ID <200906232335.57161.bzolnier@gmail.com>
Download mbox | patch
Permalink /patch/29096/
State Accepted
Delegated to: David Miller
Headers show

Comments

Bartlomiej Zolnierkiewicz - June 23, 2009, 9:35 p.m.
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] ide: improve handling of Power Management requests

Make hwif->rq point to PM request during PM sequence and do not allow
any other types of requests to slip in (the old comment was never correct
as there should be no such requests generated during PM sequence).

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
This was tested in the past (with an additional testing from Borislav)
however there were block layer changes in the meantime so you may want
to give it some more testing time just to be sure.

 drivers/ide/ide-io.c |   54 ++++++++++++++++++++-------------------------------
 1 file changed, 22 insertions(+), 32 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - June 23, 2009, 11:24 p.m.
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Tue, 23 Jun 2009 23:35:56 +0200

> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Subject: [PATCH] ide: improve handling of Power Management requests
> 
> Make hwif->rq point to PM request during PM sequence and do not allow
> any other types of requests to slip in (the old comment was never correct
> as there should be no such requests generated during PM sequence).
> 
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
> This was tested in the past (with an additional testing from Borislav)
> however there were block layer changes in the meantime so you may want
> to give it some more testing time just to be sure.

In looking at this change, it occurs to me that this queue blocking
facility could also be used to solve the user SET_XFER race.

Couldn't it?
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - June 24, 2009, 1:36 a.m.
On Wednesday 24 June 2009 01:24:02 David Miller wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date: Tue, 23 Jun 2009 23:35:56 +0200
> 
> > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > Subject: [PATCH] ide: improve handling of Power Management requests
> > 
> > Make hwif->rq point to PM request during PM sequence and do not allow
> > any other types of requests to slip in (the old comment was never correct
> > as there should be no such requests generated during PM sequence).
> > 
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > ---
> > This was tested in the past (with an additional testing from Borislav)
> > however there were block layer changes in the meantime so you may want
> > to give it some more testing time just to be sure.
> 
> In looking at this change, it occurs to me that this queue blocking
> facility could also be used to solve the user SET_XFER race.
> 
> Couldn't it?

Probably.  This is software, almost everything is possible.. ;)
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - June 24, 2009, 6:52 a.m.
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Tue, 23 Jun 2009 23:35:56 +0200

> Make hwif->rq point to PM request during PM sequence and do not allow
> any other types of requests to slip in (the old comment was never correct
> as there should be no such requests generated during PM sequence).
> 
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
> This was tested in the past (with an additional testing from Borislav)
> however there were block layer changes in the meantime so you may want
> to give it some more testing time just to be sure.

This patch looks great, applied!

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

Index: b/drivers/ide/ide-io.c
===================================================================
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -466,10 +466,14 @@  void do_ide_request(struct request_queue
 
 	if (!ide_lock_port(hwif)) {
 		ide_hwif_t *prev_port;
-
-		WARN_ON_ONCE(hwif->rq);
 repeat:
 		prev_port = hwif->host->cur_port;
+
+		if (drive->dev_flags & IDE_DFLAG_BLOCKED)
+			rq = hwif->rq;
+		else
+			WARN_ON_ONCE(hwif->rq);
+
 		if (drive->dev_flags & IDE_DFLAG_SLEEPING &&
 		    time_after(drive->sleep, jiffies)) {
 			ide_unlock_port(hwif);
@@ -496,43 +500,29 @@  repeat:
 		hwif->cur_dev = drive;
 		drive->dev_flags &= ~(IDE_DFLAG_SLEEPING | IDE_DFLAG_PARKED);
 
-		spin_unlock_irq(&hwif->lock);
-		spin_lock_irq(q->queue_lock);
-		/*
-		 * we know that the queue isn't empty, but this can happen
-		 * if the q->prep_rq_fn() decides to kill a request
-		 */
-		if (!rq)
+		if (rq == NULL) {
+			spin_unlock_irq(&hwif->lock);
+			spin_lock_irq(q->queue_lock);
+			/*
+			 * we know that the queue isn't empty, but this can
+			 * happen if ->prep_rq_fn() decides to kill a request
+			 */
 			rq = blk_fetch_request(drive->queue);
+			spin_unlock_irq(q->queue_lock);
+			spin_lock_irq(&hwif->lock);
 
-		spin_unlock_irq(q->queue_lock);
-		spin_lock_irq(&hwif->lock);
-
-		if (!rq) {
-			ide_unlock_port(hwif);
-			goto out;
+			if (rq == NULL) {
+				ide_unlock_port(hwif);
+				goto out;
+			}
 		}
 
 		/*
 		 * Sanity: don't accept a request that isn't a PM request
-		 * if we are currently power managed. This is very important as
-		 * blk_stop_queue() doesn't prevent the blk_fetch_request()
-		 * above to return us whatever is in the queue. Since we call
-		 * ide_do_request() ourselves, we end up taking requests while
-		 * the queue is blocked...
-		 * 
-		 * We let requests forced at head of queue with ide-preempt
-		 * though. I hope that doesn't happen too much, hopefully not
-		 * unless the subdriver triggers such a thing in its own PM
-		 * state machine.
+		 * if we are currently power managed.
 		 */
-		if ((drive->dev_flags & IDE_DFLAG_BLOCKED) &&
-		    blk_pm_request(rq) == 0 &&
-		    (rq->cmd_flags & REQ_PREEMPT) == 0) {
-			/* there should be no pending command at this point */
-			ide_unlock_port(hwif);
-			goto plug_device;
-		}
+		BUG_ON((drive->dev_flags & IDE_DFLAG_BLOCKED) &&
+		       blk_pm_request(rq) == 0);
 
 		hwif->rq = rq;