[PATCH 2/3] ide: add ide_[un]lock_hwgroup() helpers

[PATCH 2/3] ide: add ide_[un]lock_hwgroup() helpers

[Bartlomiej Zolnierkiewicz]

Add ide_[un]lock_hwgroup() inline helpers for obtaining exclusive
access to the given hwgroup and update the core code accordingly.

[ This change besides making code saner results in more efficient
  use of ide_{get,release}_lock(). ]

Cc: Michael Schmitz <schmitz@biophys.uni-duesseldorf.de>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Elias Oltmanns <eo@nebensachen.de>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-io.c   |   32 +++++++++++---------------------
 drivers/ide/ide-park.c |    2 +-
 include/linux/ide.h    |   20 ++++++++++++++++++++
 3 files changed, 32 insertions(+), 22 deletions(-)

Index: b/drivers/ide/ide-io.c
===================================================================
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -790,10 +790,7 @@ void do_ide_request(struct request_queue
 	/* caller must own hwgroup->lock */
 	BUG_ON(!irqs_disabled());
 
-	while (!hwgroup->busy) {
-		hwgroup->busy = 1;
-		/* for atari only */
-		ide_get_lock(ide_intr, hwgroup);
+	while (!ide_lock_hwgroup(hwgroup)) {
 		drive = choose_drive(hwgroup);
 		if (drive == NULL) {
 			int sleeping = 0;
@@ -825,17 +822,10 @@ void do_ide_request(struct request_queue
 				hwgroup->sleeping = 1;
 				hwgroup->req_gen_timer = hwgroup->req_gen;
 				mod_timer(&hwgroup->timer, sleep);
-				/* we purposely leave hwgroup->busy==1
+				/* we purposely leave hwgroup locked
 				 * while sleeping */
-			} else {
-				/* Ugly, but how can we sleep for the lock
-				 * otherwise? perhaps from tq_disk?
-				 */
-
-				/* for atari only */
-				ide_release_lock();
-				hwgroup->busy = 0;
-			}
+			} else
+				ide_unlock_hwgroup(hwgroup);
 
 			/* no more work for this hwgroup (for now) */
 			goto plug_device;
@@ -865,7 +855,7 @@ void do_ide_request(struct request_queue
 		 */
 		rq = elv_next_request(drive->queue);
 		if (!rq) {
-			hwgroup->busy = 0;
+			ide_unlock_hwgroup(hwgroup);
 			break;
 		}
 
@@ -885,8 +875,8 @@ void do_ide_request(struct request_queue
 		if ((drive->dev_flags & IDE_DFLAG_BLOCKED) &&
 		    blk_pm_request(rq) == 0 &&
 		    (rq->cmd_flags & REQ_PREEMPT) == 0) {
-			/* We clear busy, there should be no pending ATA command at this point. */
-			hwgroup->busy = 0;
+			/* there should be no pending command at this point */
+			ide_unlock_hwgroup(hwgroup);
 			goto plug_device;
 		}
 
@@ -897,7 +887,7 @@ void do_ide_request(struct request_queue
 		spin_lock_irq(&hwgroup->lock);
 
 		if (startstop == ide_stopped) {
-			hwgroup->busy = 0;
+			ide_unlock_hwgroup(hwgroup);
 			if (!elv_queue_empty(orig_drive->queue))
 				blk_plug_device(orig_drive->queue);
 		}
@@ -1001,7 +991,7 @@ void ide_timer_expiry (unsigned long dat
 		 */
 		if (hwgroup->sleeping) {
 			hwgroup->sleeping = 0;
-			hwgroup->busy = 0;
+			ide_unlock_hwgroup(hwgroup);
 		}
 	} else {
 		ide_drive_t *drive = hwgroup->drive;
@@ -1056,7 +1046,7 @@ void ide_timer_expiry (unsigned long dat
 			spin_lock_irq(&hwgroup->lock);
 			enable_irq(hwif->irq);
 			if (startstop == ide_stopped) {
-				hwgroup->busy = 0;
+				ide_unlock_hwgroup(hwgroup);
 				if (!elv_queue_empty(drive->queue))
 					blk_plug_device(drive->queue);
 			}
@@ -1249,7 +1239,7 @@ irqreturn_t ide_intr (int irq, void *dev
 	drive->service_time = jiffies - drive->service_start;
 	if (startstop == ide_stopped) {
 		if (hwgroup->handler == NULL) {	/* paranoia */
-			hwgroup->busy = 0;
+			ide_unlock_hwgroup(hwgroup);
 			if (!elv_queue_empty(drive->queue))
 				blk_plug_device(drive->queue);
 		} else
Index: b/drivers/ide/ide-park.c
===================================================================
--- a/drivers/ide/ide-park.c
+++ b/drivers/ide/ide-park.c
@@ -22,7 +22,7 @@ static void issue_park_cmd(ide_drive_t *
 		if (reset_timer && hwgroup->sleeping &&
 		    del_timer(&hwgroup->timer)) {
 			hwgroup->sleeping = 0;
-			hwgroup->busy = 0;
+			ide_unlock_hwgroup(hwgroup);
 			blk_start_queueing(q);
 		}
 		spin_unlock_irq(&hwgroup->lock);
Index: b/include/linux/ide.h
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -1280,6 +1280,26 @@ extern void ide_stall_queue(ide_drive_t 
 
 extern void ide_timer_expiry(unsigned long);
 extern irqreturn_t ide_intr(int irq, void *dev_id);
+
+static inline int ide_lock_hwgroup(ide_hwgroup_t *hwgroup)
+{
+	if (hwgroup->busy)
+		return 1;
+
+	hwgroup->busy = 1;
+	/* for atari only */
+	ide_get_lock(ide_intr, hwgroup);
+
+	return 0;
+}
+
+static inline void ide_unlock_hwgroup(ide_hwgroup_t *hwgroup)
+{
+	/* for atari only */
+	ide_release_lock();
+	hwgroup->busy = 0;
+}
+
 extern void do_ide_request(struct request_queue *);
 
 void ide_init_disk(struct gendisk *, ide_drive_t *);

Re: [PATCH 2/3] ide: add ide_[un]lock_hwgroup() helpers

[Michael Schmitz]

> -	while (!hwgroup->busy) {
> -		hwgroup->busy = 1;
> -		/* for atari only */
> -		ide_get_lock(ide_intr, hwgroup);
> +	while (!ide_lock_hwgroup(hwgroup)) {

Something I've run into while working on the locking stuff: what happens if the 
above ide_lock_hwgroup(hwgroup) sleeps for long enough to trigger the request 
timer? 

I'll think about the ramifications of your patch in the context of what I tested 
WRT unlocking whenever hwgroup->busy is cleared, and get back to you. 

	Michael

Re: [PATCH 2/3] ide: add ide_[un]lock_hwgroup() helpers

[Bartlomiej Zolnierkiewicz]

[ Sorry for the late reply. ]

On Wednesday 19 November 2008, Michael Schmitz wrote:
> > -	while (!hwgroup->busy) {
> > -		hwgroup->busy = 1;
> > -		/* for atari only */
> > -		ide_get_lock(ide_intr, hwgroup);
> > +	while (!ide_lock_hwgroup(hwgroup)) {
> 
> Something I've run into while working on the locking stuff: what happens if the 
> above ide_lock_hwgroup(hwgroup) sleeps for long enough to trigger the request 
> timer? 

AFAICS this happens before the hwgroup timeout timer is armed and IDE is not
using block layer request timers yet so we should be fine here..

Thanks,
Bart

Re: [PATCH 2/3] ide: add ide_[un]lock_hwgroup() helpers

[Michael Schmitz]

Hi,

> [ Sorry for the late reply. ]

No worries - I haven't been very active either. It's home improvement season 
down under...

> > Something I've run into while working on the locking stuff: what happens if the 
> > above ide_lock_hwgroup(hwgroup) sleeps for long enough to trigger the request 
> > timer? 
> 
> AFAICS this happens before the hwgroup timeout timer is armed and IDE is not
> using block layer request timers yet so we should be fine here..

OK; I'll just let it block while SCSI requests are in flight. Thanks for 
pondering this :-)

There's another change I'll send via Geert: since in Falcon, IDE uses stdma_intr 
as interrupt handler (interrupts dispatched there to whichever driver has 
exclusive use of the ST-DMA and associated interrupt), and this handler has been 
registered prior to IDE setup it is not necessary to request the IDE interrupt 
on Falcon when probing the IDE interface. In fact, this results in the IDE 
interrupt possibly getting run while the lock is not being held by IDE. 

Other than that it all seems to work fine. 

	Michael