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 <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Elias Oltmanns <[email protected]>
Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
---
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 *);
> - 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
[ 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
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