Skip to content

Commit

Permalink
ide: replace the global ide_lock spinlock by per-hwgroup spinlocks (v2)
Browse files Browse the repository at this point in the history 
Now that (almost) all host drivers have been fixed not to abuse ide_lock
and core code usage of ide_lock has been sanitized we may safely replace
ide_lock by per-hwgroup locks.

This patch is partially based on earlier patch from Ravikiran G Thirumalai.

While at it:
- don't use deprecated HWIF() and HWGROUP() macros
- update locking documentation in ide.h

v2:
Add missing spin_lock_init(&hwgroup->lock).  (Noticed by Elias Oltmanns)

Cc: Vaibhav V. Nivargi <vaibhav.nivargi@gmail.com>
Cc: Alok N. Kataria <alokk@calsoftinc.com>
Cc: Shai Fultheim <shai@scalex86.org>
Signed-off-by: Ravikiran Thirumalai <kiran@scalex86.org>
Cc: Elias Oltmanns <eo@nebensachen.de>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
  • Loading branch information
@bzolnier
bzolnier committed Dec 29, 2008
1 parent 6ea5222 commit 2a2ca6a
Show file tree
Hide file tree
Showing 8 changed files with 100 additions and 86 deletions.
33 changes: 16 additions & 17 deletions drivers/ide/ide-io.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -907,7 +907,7 @@ static inline ide_drive_t *choose_drive (ide_hwgroup_t *hwgroup)

/*
* Issue a new request to a drive from hwgroup
* Caller must have already done spin_lock_irqsave(&ide_lock, ..);
* Caller must have already done spin_lock_irqsave(&hwgroup->lock, ..);
*
* A hwgroup is a serialized group of IDE interfaces. Usually there is
* exactly one hwif (interface) per hwgroup, but buggy controllers (eg. CMD640)
Expand All @@ -919,7 +919,7 @@ static inline ide_drive_t *choose_drive (ide_hwgroup_t *hwgroup)
* possibly along with many other devices. This is especially common in
* PCI-based systems with off-board IDE controller cards.
*
* The IDE driver uses the single global ide_lock spinlock to protect
* The IDE driver uses a per-hwgroup spinlock to protect
* access to the request queues, and to protect the hwgroup->busy flag.
*
* The first thread into the driver for a particular hwgroup sets the
Expand All @@ -935,7 +935,7 @@ static inline ide_drive_t *choose_drive (ide_hwgroup_t *hwgroup)
* will start the next request from the queue. If no more work remains,
* the driver will clear the hwgroup->busy flag and exit.
*
* The ide_lock (spinlock) is used to protect all access to the
* The per-hwgroup spinlock is used to protect all access to the
* hwgroup->busy flag, but is otherwise not needed for most processing in
* the driver. This makes the driver much more friendlier to shared IRQs
* than previous designs, while remaining 100% (?) SMP safe and capable.
Expand All @@ -948,7 +948,7 @@ static void ide_do_request (ide_hwgroup_t *hwgroup, int masked_irq)
ide_startstop_t startstop;
int loops = 0;

/* caller must own ide_lock */
/* caller must own hwgroup->lock */
BUG_ON(!irqs_disabled());

while (!hwgroup->busy) {
Expand Down Expand Up @@ -1070,11 +1070,11 @@ static void ide_do_request (ide_hwgroup_t *hwgroup, int masked_irq)
*/
if (masked_irq != IDE_NO_IRQ && hwif->irq != masked_irq)
disable_irq_nosync(hwif->irq);
spin_unlock(&ide_lock);
spin_unlock(&hwgroup->lock);
local_irq_enable_in_hardirq();
/* allow other IRQs while we start this request */
startstop = start_request(drive, rq);
spin_lock_irq(&ide_lock);
spin_lock_irq(&hwgroup->lock);
if (masked_irq != IDE_NO_IRQ && hwif->irq != masked_irq)
enable_irq(hwif->irq);
if (startstop == ide_stopped)
Expand Down Expand Up @@ -1172,7 +1172,7 @@ void ide_timer_expiry (unsigned long data)
unsigned long flags;
unsigned long wait = -1;

spin_lock_irqsave(&ide_lock, flags);
spin_lock_irqsave(&hwgroup->lock, flags);

if (((handler = hwgroup->handler) == NULL) ||
(hwgroup->req_gen != hwgroup->req_gen_timer)) {
Expand Down Expand Up @@ -1205,7 +1205,7 @@ void ide_timer_expiry (unsigned long data)
hwgroup->timer.expires = jiffies + wait;
hwgroup->req_gen_timer = hwgroup->req_gen;
add_timer(&hwgroup->timer);
spin_unlock_irqrestore(&ide_lock, flags);
spin_unlock_irqrestore(&hwgroup->lock, flags);
return;
}
}
Expand All @@ -1215,7 +1215,7 @@ void ide_timer_expiry (unsigned long data)
* the handler() function, which means we need to
* globally mask the specific IRQ:
*/
spin_unlock(&ide_lock);
spin_unlock(&hwgroup->lock);
hwif = HWIF(drive);
/* disable_irq_nosync ?? */
disable_irq(hwif->irq);
Expand All @@ -1239,14 +1239,14 @@ void ide_timer_expiry (unsigned long data)
hwif->tp_ops->read_status(hwif));
}
drive->service_time = jiffies - drive->service_start;
spin_lock_irq(&ide_lock);
spin_lock_irq(&hwgroup->lock);
enable_irq(hwif->irq);
if (startstop == ide_stopped)
hwgroup->busy = 0;
}
}
ide_do_request(hwgroup, IDE_NO_IRQ);
spin_unlock_irqrestore(&ide_lock, flags);
spin_unlock_irqrestore(&hwgroup->lock, flags);
}

/**
Expand Down Expand Up @@ -1339,14 +1339,13 @@ irqreturn_t ide_intr (int irq, void *dev_id)
{
unsigned long flags;
ide_hwgroup_t *hwgroup = (ide_hwgroup_t *)dev_id;
ide_hwif_t *hwif;
ide_hwif_t *hwif = hwgroup->hwif;
ide_drive_t *drive;
ide_handler_t *handler;
ide_startstop_t startstop;
irqreturn_t irq_ret = IRQ_NONE;

spin_lock_irqsave(&ide_lock, flags);
hwif = hwgroup->hwif;
spin_lock_irqsave(&hwgroup->lock, flags);

if (!ide_ack_intr(hwif))
goto out;
Expand Down Expand Up @@ -1416,7 +1415,7 @@ irqreturn_t ide_intr (int irq, void *dev_id)
hwgroup->handler = NULL;
hwgroup->req_gen++;
del_timer(&hwgroup->timer);
spin_unlock(&ide_lock);
spin_unlock(&hwgroup->lock);

if (hwif->port_ops && hwif->port_ops->clear_irq)
hwif->port_ops->clear_irq(drive);
Expand All @@ -1427,7 +1426,7 @@ irqreturn_t ide_intr (int irq, void *dev_id)
/* service this interrupt, may set handler for next interrupt */
startstop = handler(drive);

spin_lock_irq(&ide_lock);
spin_lock_irq(&hwgroup->lock);
/*
* Note that handler() may have set things up for another
* interrupt to occur soon, but it cannot happen until
Expand All @@ -1448,7 +1447,7 @@ irqreturn_t ide_intr (int irq, void *dev_id)
out_handled:
irq_ret = IRQ_HANDLED;
out:
spin_unlock_irqrestore(&ide_lock, flags);
spin_unlock_irqrestore(&hwgroup->lock, flags);
return irq_ret;
}

Expand Down
48 changes: 23 additions & 25 deletions drivers/ide/ide-iops.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -835,10 +835,12 @@ static void __ide_set_handler (ide_drive_t *drive, ide_handler_t *handler,
void ide_set_handler (ide_drive_t *drive, ide_handler_t *handler,
unsigned int timeout, ide_expiry_t *expiry)
{
ide_hwgroup_t *hwgroup = drive->hwif->hwgroup;
unsigned long flags;
spin_lock_irqsave(&ide_lock, flags);

spin_lock_irqsave(&hwgroup->lock, flags);
__ide_set_handler(drive, handler, timeout, expiry);
spin_unlock_irqrestore(&ide_lock, flags);
spin_unlock_irqrestore(&hwgroup->lock, flags);
}

EXPORT_SYMBOL(ide_set_handler);
Expand All @@ -860,10 +862,11 @@ EXPORT_SYMBOL(ide_set_handler);
void ide_execute_command(ide_drive_t *drive, u8 cmd, ide_handler_t *handler,
unsigned timeout, ide_expiry_t *expiry)
{
ide_hwif_t *hwif = drive->hwif;
ide_hwgroup_t *hwgroup = hwif->hwgroup;
unsigned long flags;
ide_hwif_t *hwif = HWIF(drive);

spin_lock_irqsave(&ide_lock, flags);
spin_lock_irqsave(&hwgroup->lock, flags);
__ide_set_handler(drive, handler, timeout, expiry);
hwif->tp_ops->exec_command(hwif, cmd);
/*
Expand All @@ -873,19 +876,20 @@ void ide_execute_command(ide_drive_t *drive, u8 cmd, ide_handler_t *handler,
* FIXME: we could skip this delay with care on non shared devices
*/
ndelay(400);
spin_unlock_irqrestore(&ide_lock, flags);
spin_unlock_irqrestore(&hwgroup->lock, flags);
}
EXPORT_SYMBOL(ide_execute_command);

void ide_execute_pkt_cmd(ide_drive_t *drive)
{
ide_hwif_t *hwif = drive->hwif;
ide_hwgroup_t *hwgroup = hwif->hwgroup;
unsigned long flags;

spin_lock_irqsave(&ide_lock, flags);
spin_lock_irqsave(&hwgroup->lock, flags);
hwif->tp_ops->exec_command(hwif, ATA_CMD_PACKET);
ndelay(400);
spin_unlock_irqrestore(&ide_lock, flags);
spin_unlock_irqrestore(&hwgroup->lock, flags);
}
EXPORT_SYMBOL_GPL(ide_execute_pkt_cmd);

Expand Down Expand Up @@ -1076,22 +1080,16 @@ static void pre_reset(ide_drive_t *drive)
*/
static ide_startstop_t do_reset1 (ide_drive_t *drive, int do_not_try_atapi)
{
unsigned int unit;
unsigned long flags, timeout;
ide_hwif_t *hwif;
ide_hwgroup_t *hwgroup;
struct ide_io_ports *io_ports;
const struct ide_tp_ops *tp_ops;
ide_hwif_t *hwif = drive->hwif;
ide_hwgroup_t *hwgroup = hwif->hwgroup;
struct ide_io_ports *io_ports = &hwif->io_ports;
const struct ide_tp_ops *tp_ops = hwif->tp_ops;
const struct ide_port_ops *port_ops;
unsigned long flags, timeout;
unsigned int unit;
DEFINE_WAIT(wait);

spin_lock_irqsave(&ide_lock, flags);
hwif = HWIF(drive);
hwgroup = HWGROUP(drive);

io_ports = &hwif->io_ports;

tp_ops = hwif->tp_ops;
spin_lock_irqsave(&hwgroup->lock, flags);

/* We must not reset with running handlers */
BUG_ON(hwgroup->handler != NULL);
Expand All @@ -1106,7 +1104,7 @@ static ide_startstop_t do_reset1 (ide_drive_t *drive, int do_not_try_atapi)
hwgroup->poll_timeout = jiffies + WAIT_WORSTCASE;
hwgroup->polling = 1;
__ide_set_handler(drive, &atapi_reset_pollfunc, HZ/20, NULL);
spin_unlock_irqrestore(&ide_lock, flags);
spin_unlock_irqrestore(&hwgroup->lock, flags);
return ide_started;
}

Expand All @@ -1129,9 +1127,9 @@ static ide_startstop_t do_reset1 (ide_drive_t *drive, int do_not_try_atapi)
if (time_before_eq(timeout, now))
break;

spin_unlock_irqrestore(&ide_lock, flags);
spin_unlock_irqrestore(&hwgroup->lock, flags);
timeout = schedule_timeout_uninterruptible(timeout - now);
spin_lock_irqsave(&ide_lock, flags);
spin_lock_irqsave(&hwgroup->lock, flags);
} while (timeout);
finish_wait(&ide_park_wq, &wait);

Expand All @@ -1143,7 +1141,7 @@ static ide_startstop_t do_reset1 (ide_drive_t *drive, int do_not_try_atapi)
pre_reset(&hwif->drives[unit]);

if (io_ports->ctl_addr == 0) {
spin_unlock_irqrestore(&ide_lock, flags);
spin_unlock_irqrestore(&hwgroup->lock, flags);
ide_complete_drive_reset(drive, -ENXIO);
return ide_stopped;
}
Expand Down Expand Up @@ -1179,7 +1177,7 @@ static ide_startstop_t do_reset1 (ide_drive_t *drive, int do_not_try_atapi)
if (port_ops && port_ops->resetproc)
port_ops->resetproc(drive);

spin_unlock_irqrestore(&ide_lock, flags);
spin_unlock_irqrestore(&hwgroup->lock, flags);
return ide_started;
}

Expand Down
16 changes: 8 additions & 8 deletions drivers/ide/ide-park.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,16 @@ DECLARE_WAIT_QUEUE_HEAD(ide_park_wq);

static void issue_park_cmd(ide_drive_t *drive, unsigned long timeout)
{
ide_hwgroup_t *hwgroup = drive->hwif->hwgroup;
struct request_queue *q = drive->queue;
struct request *rq;
int rc;

timeout += jiffies;
spin_lock_irq(&ide_lock);
spin_lock_irq(&hwgroup->lock);
if (drive->dev_flags & IDE_DFLAG_PARKED) {
ide_hwgroup_t *hwgroup = drive->hwif->hwgroup;
int reset_timer;
int reset_timer = time_before(timeout, drive->sleep);

reset_timer = time_before(timeout, drive->sleep);
drive->sleep = timeout;
wake_up_all(&ide_park_wq);
if (reset_timer && hwgroup->sleeping &&
Expand All @@ -26,10 +25,10 @@ static void issue_park_cmd(ide_drive_t *drive, unsigned long timeout)
hwgroup->busy = 0;
blk_start_queueing(q);
}
spin_unlock_irq(&ide_lock);
spin_unlock_irq(&hwgroup->lock);
return;
}
spin_unlock_irq(&ide_lock);
spin_unlock_irq(&hwgroup->lock);

rq = blk_get_request(q, READ, __GFP_WAIT);
rq->cmd[0] = REQ_PARK_HEADS;
Expand Down Expand Up @@ -62,20 +61,21 @@ ssize_t ide_park_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
ide_drive_t *drive = to_ide_device(dev);
ide_hwgroup_t *hwgroup = drive->hwif->hwgroup;
unsigned long now;
unsigned int msecs;

if (drive->dev_flags & IDE_DFLAG_NO_UNLOAD)
return -EOPNOTSUPP;

spin_lock_irq(&ide_lock);
spin_lock_irq(&hwgroup->lock);
now = jiffies;
if (drive->dev_flags & IDE_DFLAG_PARKED &&
time_after(drive->sleep, now))
msecs = jiffies_to_msecs(drive->sleep - now);
else
msecs = 0;
spin_unlock_irq(&ide_lock);
spin_unlock_irq(&hwgroup->lock);

return snprintf(buf, 20, "%u\n", msecs);
}
Expand Down
Loading

0 comments on commit 2a2ca6a

Please sign in to comment.