[patch 0/4] ide: Break ide_lock to per-hwgroup lock

[patch 0/4] ide: Break ide_lock to per-hwgroup lock

[Ravikiran G Thirumalai]

The following patchset breaks down the global ide_lock to per-hwgroup lock.
We have taken the following approach.

1. Move the hwif tuning code from probe_hwif to ideprobe_init, after
hwif_init so that hwgroups are present for all the hwifs when the tune
routines for the hwifs are invoked (patch 1)

2. Change the core ide code to use hwgroup->lock instead of ide_lock.
Deprecate ide_lock (patch 2)

3. Change the host controllers to use hwgroup->lock (patch 3)

4. Change host controller drivers to use per driver lock instead of ide_lock
where needed or hwgroup->lock on case by case basis. This can be done 
incrementally for various controllers and we will have working code between 
patches -- this is done now for piix controller only.  Eventually, 
we can change all controllers to remove ide_lock

Thanks to Bartlomiej for comments and suggestions.

Patchset follows.  Patchset tested on a smp box with a piix controller.

Thanks,
Kiran

[patch 1/4] ide: Break ide_lock -- Move hwif tuning code after hwif_init

[Ravikiran G Thirumalai]

Following patch moves the hwif tuning code from probe_hwif to ideprobe_init
after ideprobe_init calls hwif_init so that all hwif's have associated
hwgroups.  With this patch, we should always have hwgroups for hwifs during
calls the drive tune routines.


Signed-off-by: Alok N Kataria <alokk@calsoftinc.com>
Signed-off-by: Ravikiran Thirumalai <kiran@scalex86.org>

Index: linux-2.6.13/drivers/ide/ide-probe.c
===================================================================
--- linux-2.6.13.orig/drivers/ide/ide-probe.c	2005-09-06 11:22:29.000000000 -0700
+++ linux-2.6.13/drivers/ide/ide-probe.c	2005-09-06 15:12:58.000000000 -0700
@@ -852,8 +852,15 @@
 
 	if (!hwif->present) {
 		ide_hwif_release_regions(hwif);
-		return;
 	}
+}
+
+static void ide_tune_drives(ide_hwif_t * hwif)
+{
+	unsigned int unit;
+
+	if (!hwif->present)
+		return;
 
 	for (unit = 0; unit < MAX_DRIVES; ++unit) {
 		ide_drive_t *drive = &hwif->drives[unit];
@@ -1443,9 +1450,12 @@
 	for (index = 0; index < MAX_HWIFS; ++index)
 		if (probe[index])
 			probe_hwif(&ide_hwifs[index]);
-	for (index = 0; index < MAX_HWIFS; ++index)
-		if (probe[index])
+	for (index = 0; index < MAX_HWIFS; ++index) {
+		if (probe[index]) {
 			hwif_init(&ide_hwifs[index]);
+			ide_tune_drives(&ide_hwifs[index]);
+		}
+	}
 	for (index = 0; index < MAX_HWIFS; ++index) {
 		if (probe[index]) {
 			ide_hwif_t *hwif = &ide_hwifs[index];

[patch 2/4] ide: Break ide_lock -- replace ide_lock with hwgroup->lock in core ide

[Ravikiran G Thirumalai]

Patch to convert ide core code to use hwgroup lock instead of a global
ide_lock.

Signed-off-by: Vaibhav V. Nivargi <vaibhav.nivargi@gmail.com>
Signed-off-by: Alok N. Kataria <alokk@calsoftinc.com>
Signed-off-by: Ravikiran Thirumalai <kiran@scalex86.org>
Signed-off-by: Shai Fultheim <shai@scalex86.org>


Index: linux-2.6.13/drivers/ide/ide-cd.c
===================================================================
--- linux-2.6.13.orig/drivers/ide/ide-cd.c	2005-09-06 11:22:29.000000000 -0700
+++ linux-2.6.13/drivers/ide/ide-cd.c	2005-09-06 11:34:43.000000000 -0700
@@ -590,7 +590,8 @@
 
 static void cdrom_end_request (ide_drive_t *drive, int uptodate)
 {
-	struct request *rq = HWGROUP(drive)->rq;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
+	struct request *rq = hwgroup->rq;
 	int nsectors = rq->hard_cur_sectors;
 
 	if ((rq->flags & REQ_SENSE) && uptodate) {
@@ -612,10 +613,10 @@
 			/*
 			 * now end failed request
 			 */
-			spin_lock_irqsave(&ide_lock, flags);
+			spin_lock_irqsave(&hwgroup->lock, flags);
 			end_that_request_chunk(failed, 0, failed->data_len);
 			end_that_request_last(failed);
-			spin_unlock_irqrestore(&ide_lock, flags);
+			spin_unlock_irqrestore(&hwgroup->lock, flags);
 		}
 
 		cdrom_analyze_sense_data(drive, failed, sense);
@@ -636,7 +637,8 @@
    Returns 1 if the request was ended. */
 static int cdrom_decode_status(ide_drive_t *drive, int good_stat, int *stat_ret)
 {
-	struct request *rq = HWGROUP(drive)->rq;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
+	struct request *rq = hwgroup->rq;
 	int stat, err, sense_key;
 	
 	/* Check for errors. */
@@ -698,10 +700,10 @@
 		 * request sense has completed
 		 */
 		if (stat & ERR_STAT) {
-			spin_lock_irqsave(&ide_lock, flags);
+			spin_lock_irqsave(&hwgroup->lock, flags);
 			blkdev_dequeue_request(rq);
 			HWGROUP(drive)->rq = NULL;
-			spin_unlock_irqrestore(&ide_lock, flags);
+			spin_unlock_irqrestore(&hwgroup->lock, flags);
 
 			cdrom_queue_request_sense(drive, rq->sense, rq);
 		} else
@@ -741,9 +743,9 @@
 					 * take a breather relying on the
 					 * unplug timer to kick us again
 					 */
-					spin_lock_irqsave(&ide_lock, flags);
+					spin_lock_irqsave(&hwgroup->lock, flags);
 					blk_plug_device(drive->queue);
-					spin_unlock_irqrestore(&ide_lock,flags);
+					spin_unlock_irqrestore(&hwgroup->lock, flags);
 					return 1;
 				}
 			}
@@ -839,6 +841,7 @@
 	ide_startstop_t startstop;
 	struct cdrom_info *info = drive->driver_data;
 	ide_hwif_t *hwif = drive->hwif;
+	ide_hwgroup_t *hwgroup = hwif->hwgroup;
 
 	/* Wait for the controller to be idle. */
 	if (ide_wait_stat(&startstop, drive, 0, BUSY_STAT, WAIT_READY))
@@ -866,10 +869,10 @@
 		unsigned long flags;
 
 		/* packet command */
-		spin_lock_irqsave(&ide_lock, flags);
+		spin_lock_irqsave(&hwgroup->lock, flags);
 		hwif->OUTBSYNC(drive, WIN_PACKETCMD, IDE_COMMAND_REG);
 		ndelay(400);
-		spin_unlock_irqrestore(&ide_lock, flags);
+		spin_unlock_irqrestore(&hwgroup->lock, flags);
 
 		return (*handler) (drive);
 	}
@@ -1609,7 +1612,8 @@
 static ide_startstop_t cdrom_newpc_intr(ide_drive_t *drive)
 {
 	struct cdrom_info *info = drive->driver_data;
-	struct request *rq = HWGROUP(drive)->rq;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
+	struct request *rq = hwgroup->rq;
 	int dma_error, dma, stat, ireason, len, thislen;
 	u8 lowcyl, highcyl;
 	xfer_func_t *xferfunc;
@@ -1737,11 +1741,11 @@
 	if (!rq->data_len)
 		post_transform_command(rq);
 
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
 	blkdev_dequeue_request(rq);
 	end_that_request_last(rq);
 	HWGROUP(drive)->rq = NULL;
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 	return ide_stopped;
 }
 
Index: linux-2.6.13/drivers/ide/ide-disk.c
===================================================================
--- linux-2.6.13.orig/drivers/ide/ide-disk.c	2005-09-06 11:22:29.000000000 -0700
+++ linux-2.6.13/drivers/ide/ide-disk.c	2005-09-06 11:34:43.000000000 -0700
@@ -786,11 +786,12 @@
 
 static int set_nowerr(ide_drive_t *drive, int arg)
 {
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 	if (ide_spin_wait_hwgroup(drive))
 		return -EBUSY;
 	drive->nowerr = arg;
 	drive->bad_wstat = arg ? BAD_R_STAT : BAD_W_STAT;
-	spin_unlock_irq(&ide_lock);
+	spin_unlock_irq(&hwgroup->lock);
 	return 0;
 }
 
Index: linux-2.6.13/drivers/ide/ide-io.c
===================================================================
--- linux-2.6.13.orig/drivers/ide/ide-io.c	2005-09-06 11:22:29.000000000 -0700
+++ linux-2.6.13/drivers/ide/ide-io.c	2005-09-06 11:34:43.000000000 -0700
@@ -112,9 +112,10 @@
 	struct request *rq;
 	unsigned long flags;
 	int ret = 1;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
-	spin_lock_irqsave(&ide_lock, flags);
-	rq = HWGROUP(drive)->rq;
+	spin_lock_irqsave(&hwgroup->lock, flags);
+	rq = hwgroup->rq;
 
 	if (!nr_sectors)
 		nr_sectors = rq->hard_cur_sectors;
@@ -124,7 +125,7 @@
 	else
 		ret = __ide_end_request(drive, rq, uptodate, nr_sectors);
 
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 	return ret;
 }
 EXPORT_SYMBOL(ide_end_request);
@@ -233,12 +234,13 @@
 static void ide_complete_pm_request (ide_drive_t *drive, struct request *rq)
 {
 	unsigned long flags;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
 #ifdef DEBUG_PM
 	printk("%s: completing PM request, %s\n", drive->name,
 	       blk_pm_suspend_request(rq) ? "suspend" : "resume");
 #endif
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
 	if (blk_pm_suspend_request(rq)) {
 		blk_stop_queue(drive->queue);
 	} else {
@@ -248,7 +250,7 @@
 	blkdev_dequeue_request(rq);
 	HWGROUP(drive)->rq = NULL;
 	end_that_request_last(rq);
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 }
 
 /*
@@ -305,10 +307,11 @@
 	ide_hwif_t *hwif = HWIF(drive);
 	unsigned long flags;
 	struct request *rq;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
-	spin_lock_irqsave(&ide_lock, flags);
-	rq = HWGROUP(drive)->rq;
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
+	rq = hwgroup->rq;
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 
 	if (rq->flags & REQ_DRIVE_CMD) {
 		u8 *args = (u8 *) rq->buffer;
@@ -375,12 +378,12 @@
 		return;
 	}
 
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
 	blkdev_dequeue_request(rq);
-	HWGROUP(drive)->rq = NULL;
+	hwgroup->rq = NULL;
 	rq->errors = err;
 	end_that_request_last(rq);
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 }
 
 EXPORT_SYMBOL(ide_end_drive_cmd);
@@ -1062,7 +1065,7 @@
 
 /*
  * 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)
@@ -1074,7 +1077,7 @@
  * 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
@@ -1090,7 +1093,7 @@
  * 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(hwgroup->lock) 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.
@@ -1105,7 +1108,7 @@
 	/* for atari only: POSSIBLY BROKEN HERE(?) */
 	ide_get_lock(ide_intr, hwgroup);
 
-	/* caller must own ide_lock */
+	/* caller must own hwgroup->lock */
 	BUG_ON(!irqs_disabled());
 
 	while (!hwgroup->busy) {
@@ -1211,11 +1214,11 @@
 		 */
 		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();
 			/* allow other IRQs while we start this request */
 		startstop = start_request(drive, rq);
-		spin_lock_irq(&ide_lock);
+		spin_unlock_irq(&hwgroup->lock);
 		if (masked_irq != IDE_NO_IRQ && hwif->irq != masked_irq)
 			enable_irq(hwif->irq);
 		if (startstop == ide_stopped)
@@ -1309,7 +1312,7 @@
 	unsigned long	flags;
 	unsigned long	wait = -1;
 
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
 
 	if ((handler = hwgroup->handler) == NULL) {
 		/*
@@ -1340,7 +1343,7 @@
 					/* reset timer */
 					hwgroup->timer.expires  = jiffies + wait;
 					add_timer(&hwgroup->timer);
-					spin_unlock_irqrestore(&ide_lock, flags);
+					spin_unlock_irqrestore(&hwgroup->lock, flags);
 					return;
 				}
 			}
@@ -1350,7 +1353,7 @@
 			 * the handler() function, which means we need to
 			 * globally mask the specific IRQ:
 			 */
-			spin_unlock(&ide_lock);
+			spin_unlock(&hwgroup->lock);
 			hwif  = HWIF(drive);
 #if DISABLE_IRQ_NOSYNC
 			disable_irq_nosync(hwif->irq);
@@ -1377,14 +1380,14 @@
 					ide_error(drive, "irq timeout", hwif->INB(IDE_STATUS_REG));
 			}
 			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);
 }
 
 /**
@@ -1481,11 +1484,11 @@
 	ide_handler_t *handler;
 	ide_startstop_t startstop;
 
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
 	hwif = hwgroup->hwif;
 
 	if (!ide_ack_intr(hwif)) {
-		spin_unlock_irqrestore(&ide_lock, flags);
+		spin_unlock_irqrestore(&hwgroup->lock, flags);
 		return IRQ_NONE;
 	}
 
@@ -1523,7 +1526,7 @@
 			(void) hwif->INB(hwif->io_ports[IDE_STATUS_OFFSET]);
 #endif /* CONFIG_BLK_DEV_IDEPCI */
 		}
-		spin_unlock_irqrestore(&ide_lock, flags);
+		spin_unlock_irqrestore(&hwgroup->lock, flags);
 		return IRQ_NONE;
 	}
 	drive = hwgroup->drive;
@@ -1534,7 +1537,7 @@
 		 *
 		 * [Note - this can occur if the drive is hot unplugged]
 		 */
-		spin_unlock_irqrestore(&ide_lock, flags);
+		spin_unlock_irqrestore(&hwgroup->lock, flags);
 		return IRQ_HANDLED;
 	}
 	if (!drive_is_ready(drive)) {
@@ -1545,7 +1548,7 @@
 		 * their status register is up to date.  Hopefully we have
 		 * enough advance overhead that the latter isn't a problem.
 		 */
-		spin_unlock_irqrestore(&ide_lock, flags);
+		spin_unlock_irqrestore(&hwgroup->lock, flags);
 		return IRQ_NONE;
 	}
 	if (!hwgroup->busy) {
@@ -1554,13 +1557,13 @@
 	}
 	hwgroup->handler = NULL;
 	del_timer(&hwgroup->timer);
-	spin_unlock(&ide_lock);
+	spin_unlock(&hwgroup->lock);
 
 	if (drive->unmask)
 		local_irq_enable();
 	/* 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
@@ -1579,7 +1582,7 @@
 				"on exit\n", drive->name);
 		}
 	}
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 	return IRQ_HANDLED;
 }
 
@@ -1654,7 +1657,7 @@
 		rq->end_io = blk_end_sync_rq;
 	}
 
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
 	if (action == ide_preempt)
 		hwgroup->rq = NULL;
 	if (action == ide_preempt || action == ide_head_wait) {
@@ -1663,7 +1666,7 @@
 	}
 	__elv_add_request(drive->queue, rq, where, 0);
 	ide_do_request(hwgroup, IDE_NO_IRQ);
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 
 	err = 0;
 	if (must_wait) {
Index: linux-2.6.13/drivers/ide/ide-iops.c
===================================================================
--- linux-2.6.13.orig/drivers/ide/ide-iops.c	2005-09-06 11:22:29.000000000 -0700
+++ linux-2.6.13/drivers/ide/ide-iops.c	2005-09-06 11:34:43.000000000 -0700
@@ -945,9 +945,10 @@
 		      unsigned int timeout, ide_expiry_t *expiry)
 {
 	unsigned long flags;
-	spin_lock_irqsave(&ide_lock, flags);
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
+	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);
@@ -972,7 +973,7 @@
 	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 	ide_hwif_t *hwif = HWIF(drive);
 	
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
 	
 	if(hwgroup->handler)
 		BUG();
@@ -988,7 +989,7 @@
 	   devices 
 	*/
 	ndelay(400);
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 }
 
 EXPORT_SYMBOL(ide_execute_command);
@@ -1168,10 +1169,11 @@
 	ide_hwif_t *hwif;
 	ide_hwgroup_t *hwgroup;
 	
-	spin_lock_irqsave(&ide_lock, flags);
 	hwif = HWIF(drive);
 	hwgroup = HWGROUP(drive);
 
+	spin_lock_irqsave(&hwgroup->lock, flags);
+
 	/* We must not reset with running handlers */
 	if(hwgroup->handler != NULL)
 		BUG();
@@ -1186,7 +1188,7 @@
 		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;
 	}
 
@@ -1199,7 +1201,7 @@
 
 #if OK_TO_RESET_CONTROLLER
 	if (!IDE_CONTROL_REG) {
-		spin_unlock_irqrestore(&ide_lock, flags);
+		spin_unlock_irqrestore(&hwgroup->lock, flags);
 		return ide_stopped;
 	}
 
@@ -1239,7 +1241,7 @@
 	
 #endif	/* OK_TO_RESET_CONTROLLER */
 
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 	return ide_started;
 }
 
Index: linux-2.6.13/drivers/ide/ide-lib.c
===================================================================
--- linux-2.6.13.orig/drivers/ide/ide-lib.c	2005-09-06 11:22:29.000000000 -0700
+++ linux-2.6.13/drivers/ide/ide-lib.c	2005-09-06 12:00:13.000000000 -0700
@@ -450,12 +450,11 @@
 	struct request *rq;
 	u8 opcode = 0;
 	int found = 0;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
-	spin_lock(&ide_lock);
-	rq = NULL;
-	if (HWGROUP(drive))
-		rq = HWGROUP(drive)->rq;
-	spin_unlock(&ide_lock);
+	spin_lock(&hwgroup->lock);
+	rq = hwgroup->rq;
+	spin_unlock(&hwgroup->lock);
 	if (!rq)
 		return;
 	if (rq->flags & (REQ_DRIVE_CMD | REQ_DRIVE_TASK)) {
Index: linux-2.6.13/drivers/ide/ide-probe.c
===================================================================
--- linux-2.6.13.orig/drivers/ide/ide-probe.c	2005-09-06 11:34:31.000000000 -0700
+++ linux-2.6.13/drivers/ide/ide-probe.c	2005-09-06 11:34:43.000000000 -0700
@@ -985,7 +985,8 @@
 	 *	do not.
 	 */
 
-	q = blk_init_queue_node(do_ide_request, &ide_lock, hwif_to_node(hwif));
+	q = blk_init_queue_node(do_ide_request, &(HWGROUP(drive)->lock), 
+				hwif_to_node(hwif));
 	if (!q)
 		return 1;
 
@@ -1099,10 +1100,10 @@
 		 * linked list, the first entry is the hwif that owns
 		 * hwgroup->handler - do not change that.
 		 */
-		spin_lock_irq(&ide_lock);
+		spin_lock_irq(&hwgroup->lock);
 		hwif->next = hwgroup->hwif->next;
 		hwgroup->hwif->next = hwif;
-		spin_unlock_irq(&ide_lock);
+		spin_unlock_irq(&hwgroup->lock);
 	} else {
 		hwgroup = kmalloc_node(sizeof(ide_hwgroup_t), GFP_KERNEL,
 					hwif_to_node(hwif->drives[0].hwif));
@@ -1112,6 +1113,7 @@
 		hwif->hwgroup = hwgroup;
 
 		memset(hwgroup, 0, sizeof(ide_hwgroup_t));
+		spin_lock_init(&hwgroup->lock);
 		hwgroup->hwif     = hwif->next = hwif;
 		hwgroup->rq       = NULL;
 		hwgroup->handler  = NULL;
@@ -1159,7 +1161,7 @@
 			printk(KERN_ERR "ide: failed to init %s\n",drive->name);
 			continue;
 		}
-		spin_lock_irq(&ide_lock);
+		spin_lock_irq(&hwgroup->lock);
 		if (!hwgroup->drive) {
 			/* first drive for hwgroup. */
 			drive->next = drive;
@@ -1169,7 +1171,7 @@
 			drive->next = hwgroup->drive->next;
 			hwgroup->drive->next = drive;
 		}
-		spin_unlock_irq(&ide_lock);
+		spin_unlock_irq(&hwgroup->lock);
 	}
 
 #if !defined(__mc68000__) && !defined(CONFIG_APUS) && !defined(__sparc__)
@@ -1193,13 +1195,13 @@
 	up(&ide_cfg_sem);
 	return 0;
 out_unlink:
-	spin_lock_irq(&ide_lock);
 	if (hwif->next == hwif) {
 		BUG_ON(match);
 		BUG_ON(hwgroup->hwif != hwif);
 		kfree(hwgroup);
 	} else {
 		ide_hwif_t *g;
+		spin_lock_irq(&hwgroup->lock);
 		g = hwgroup->hwif;
 		while (g->next != hwif)
 			g = g->next;
@@ -1210,8 +1212,8 @@
 			hwgroup->hwif = g;
 		}
 		BUG_ON(hwgroup->hwif == hwif);
+		spin_unlock_irq(&hwgroup->lock);
 	}
-	spin_unlock_irq(&ide_lock);
 out_up:
 	up(&ide_cfg_sem);
 	return 1;
@@ -1316,8 +1318,9 @@
 static void drive_release_dev (struct device *dev)
 {
 	ide_drive_t *drive = container_of(dev, ide_drive_t, gendev);
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
-	spin_lock_irq(&ide_lock);
+	spin_lock_irq(&hwgroup->lock);
 	if (drive->devfs_name[0] != '\0') {
 		devfs_remove(drive->devfs_name);
 		drive->devfs_name[0] = '\0';
@@ -1329,11 +1332,11 @@
 	}
 	drive->present = 0;
 	/* Messed up locking ... */
-	spin_unlock_irq(&ide_lock);
+	spin_unlock_irq(&hwgroup->lock);
 	blk_cleanup_queue(drive->queue);
-	spin_lock_irq(&ide_lock);
+	spin_lock_irq(&hwgroup->lock);
 	drive->queue = NULL;
-	spin_unlock_irq(&ide_lock);
+	spin_unlock_irq(&hwgroup->lock);
 
 	up(&drive->gendev_rel_sem);
 }
Index: linux-2.6.13/drivers/ide/ide.c
===================================================================
--- linux-2.6.13.orig/drivers/ide/ide.c	2005-09-06 11:22:29.000000000 -0700
+++ linux-2.6.13/drivers/ide/ide.c	2005-09-06 11:34:43.000000000 -0700
@@ -175,7 +175,13 @@
 static int initializing;	/* set while initializing built-in drivers */
 
 DECLARE_MUTEX(ide_cfg_sem);
- __cacheline_aligned_in_smp DEFINE_SPINLOCK(ide_lock);
+
+/* 
+ * Global ide_lock is broken down to per HWGROUP lock. For controller 
+ * serialization when hwgroups are not present, use per-controller
+ * spinlocks.  See drivers/ide/pci/piix.c
+ */
+ __cacheline_aligned_in_smp DEFINE_SPINLOCK(ide_lock __deprecated);
 
 #ifdef CONFIG_BLK_DEV_IDEPCI
 static int ide_scan_direction; /* THIS was formerly 2.2.x pci=reverse */
@@ -587,10 +593,15 @@
 	BUG_ON(in_interrupt());
 	BUG_ON(irqs_disabled());
 	down(&ide_cfg_sem);
-	spin_lock_irq(&ide_lock);
 	hwif = &ide_hwifs[index];
-	if (!hwif->present)
-		goto abort;
+	hwgroup = hwif->hwgroup;
+	spin_lock_irq(&hwgroup->lock);
+	if (!hwif->present) {
+		spin_unlock_irq(&hwgroup->lock);
+		up(&ide_cfg_sem);
+		return;
+	}
+
 	for (unit = 0; unit < MAX_DRIVES; ++unit) {
 		drive = &hwif->drives[unit];
 		if (!drive->present) {
@@ -600,18 +611,17 @@
 			}
 			continue;
 		}
-		spin_unlock_irq(&ide_lock);
+		spin_unlock_irq(&hwgroup->lock);
 		device_unregister(&drive->gendev);
 		down(&drive->gendev_rel_sem);
-		spin_lock_irq(&ide_lock);
+		spin_lock_irq(&hwgroup->lock);
 	}
 	hwif->present = 0;
 
-	spin_unlock_irq(&ide_lock);
+	spin_unlock_irq(&hwgroup->lock);
 
 	destroy_proc_ide_interface(hwif);
 
-	hwgroup = hwif->hwgroup;
 	/*
 	 * free the irq if we were the only hwif using it
 	 */
@@ -624,7 +634,7 @@
 	if (irq_count == 1)
 		free_irq(hwif->irq, hwgroup);
 
-	spin_lock_irq(&ide_lock);
+	spin_lock_irq(&hwgroup->lock);
 	/*
 	 * Note that we only release the standard ports,
 	 * and do not even try to handle any extra ports
@@ -638,6 +648,7 @@
 	 */
 	if (hwif->next == hwif) {
 		BUG_ON(hwgroup->hwif != hwif);
+		spin_unlock_irq(&hwgroup->lock);
 		kfree(hwgroup);
 	} else {
 		/* There is another interface in hwgroup.
@@ -657,10 +668,9 @@
 			hwgroup->hwif = g;
 		}
 		BUG_ON(hwgroup->hwif == hwif);
+		spin_unlock_irq(&hwgroup->lock);
 	}
 
-	/* More messed up locking ... */
-	spin_unlock_irq(&ide_lock);
 	device_unregister(&hwif->gendev);
 	down(&hwif->gendev_rel_sem);
 
@@ -670,7 +680,6 @@
 	blk_unregister_region(MKDEV(hwif->major, 0), MAX_DRIVES<<PARTN_BITS);
 	kfree(hwif->sg_table);
 	unregister_blkdev(hwif->major, hwif->name);
-	spin_lock_irq(&ide_lock);
 
 	if (hwif->dma_base) {
 		(void) ide_release_dma(hwif);
@@ -692,9 +701,6 @@
 	init_hwif_default(hwif, index);
 
 	ide_hwif_restore(hwif, &tmp_hwif);
-
-abort:
-	spin_unlock_irq(&ide_lock);
 	up(&ide_cfg_sem);
 }
 
@@ -1012,9 +1018,10 @@
 {
 	int		val = -EINVAL;
 	unsigned long	flags;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
 	if ((setting->rw & SETTING_READ)) {
-		spin_lock_irqsave(&ide_lock, flags);
+		spin_lock_irqsave(&hwgroup->lock, flags);
 		switch(setting->data_type) {
 			case TYPE_BYTE:
 				val = *((u8 *) setting->data);
@@ -1027,7 +1034,7 @@
 				val = *((u32 *) setting->data);
 				break;
 		}
-		spin_unlock_irqrestore(&ide_lock, flags);
+		spin_unlock_irqrestore(&hwgroup->lock, flags);
 	}
 	return val;
 }
@@ -1037,7 +1044,7 @@
  *	@drive: drive in the group
  *
  *	Wait for an IDE device group to go non busy and then return
- *	holding the ide_lock which guards the hwgroup->busy status
+ *	holding the hwgroup->lock which guards the hwgroup->busy status
  *	and right to use it.
  */
 
@@ -1046,11 +1053,11 @@
 	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 	unsigned long timeout = jiffies + (3 * HZ);
 
-	spin_lock_irq(&ide_lock);
+	spin_lock_irq(&hwgroup->lock);
 
 	while (hwgroup->busy) {
 		unsigned long lflags;
-		spin_unlock_irq(&ide_lock);
+		spin_unlock_irq(&hwgroup->lock);
 		local_irq_set(lflags);
 		if (time_after(jiffies, timeout)) {
 			local_irq_restore(lflags);
@@ -1058,7 +1065,7 @@
 			return -EBUSY;
 		}
 		local_irq_restore(lflags);
-		spin_lock_irq(&ide_lock);
+		spin_lock_irq(&hwgroup->lock);
 	}
 	return 0;
 }
@@ -1087,6 +1094,7 @@
 {
 	int i;
 	u32 *p;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
@@ -1114,7 +1122,7 @@
 				*p = val;
 			break;
 	}
-	spin_unlock_irq(&ide_lock);
+	spin_unlock_irq(&hwgroup->lock);
 	return 0;
 }
 
@@ -1260,6 +1268,7 @@
 	ide_driver_t *drv;
 	int err = 0;
 	void __user *p = (void __user *)arg;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
 	down(&ide_setting_sem);
 	if ((setting = ide_find_setting_by_ioctl(drive, cmd)) != NULL) {
@@ -1377,18 +1386,18 @@
 			 *	spot if we miss one somehow
 			 */
 
-			spin_lock_irqsave(&ide_lock, flags);
+			spin_lock_irqsave(&hwgroup->lock, flags);
 
 			ide_abort(drive, "drive reset");
 
-			if(HWGROUP(drive)->handler)
+			if(hwgroup->handler)
 				BUG();
 				
 			/* Ensure nothing gets queued after we
 			   drop the lock. Reset will clear the busy */
 		   
-			HWGROUP(drive)->busy = 1;
-			spin_unlock_irqrestore(&ide_lock, flags);
+			hwgroup->busy = 1;
+			spin_unlock_irqrestore(&hwgroup->lock, flags);
 			(void) ide_do_reset(drive);
 
 			return 0;
@@ -1874,21 +1883,22 @@
  *	Disconnect a drive from the driver it was attached to and then
  *	clean up the various proc files and other objects attached to it.
  *
- *	Takes ide_setting_sem and ide_lock.
+ *	Takes ide_setting_sem and hwgroup->lock.
  *	Caller must hold none of the locks.
  */
 
 void ide_unregister_subdriver(ide_drive_t *drive, ide_driver_t *driver)
 {
 	unsigned long flags;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 	
 	down(&ide_setting_sem);
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
 #ifdef CONFIG_PROC_FS
 	ide_remove_proc_entries(drive->proc, driver->proc);
 #endif
 	auto_remove_settings(drive);
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 	up(&ide_setting_sem);
 }
 
Index: linux-2.6.13/drivers/scsi/ide-scsi.c
===================================================================
--- linux-2.6.13.orig/drivers/scsi/ide-scsi.c	2005-09-06 11:22:29.000000000 -0700
+++ linux-2.6.13/drivers/scsi/ide-scsi.c	2005-09-06 11:34:43.000000000 -0700
@@ -956,6 +956,7 @@
 	ide_drive_t    *drive = scsi->drive;
 	int		busy;
 	int             ret   = FAILED;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
 	/* In idescsi_eh_abort we try to gently pry our command from the ide subsystem */
 
@@ -974,7 +975,7 @@
 	if (test_bit(IDESCSI_LOG_CMD, &scsi->log))
 		printk (KERN_WARNING "ide-scsi: drive did%s become ready\n", busy?" not":"");
 
-	spin_lock_irq(&ide_lock);
+	spin_lock_irq(&hwgroup->lock);
 
 	/* If there is no pc running we're done (our interrupt took care of it) */
 	if (!scsi->pc) {
@@ -1000,7 +1001,7 @@
 	}
 
 ide_unlock:
-	spin_unlock_irq(&ide_lock);
+	spin_unlock_irq(&hwgroup->lock);
 no_drive:
 	if (test_bit(IDESCSI_LOG_CMD, &scsi->log))
 		printk (KERN_WARNING "ide-scsi: abort returns %s\n", ret == SUCCESS?"success":"failed");
@@ -1015,6 +1016,7 @@
 	ide_drive_t    *drive = scsi->drive;
 	int             ready = 0;
 	int             ret   = SUCCESS;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
 	/* In idescsi_eh_reset we forcefully remove the command from the ide subsystem and reset the device. */
 
@@ -1028,11 +1030,11 @@
 	}
 
 	spin_lock_irq(cmd->device->host->host_lock);
-	spin_lock(&ide_lock);
+	spin_lock(&hwgroup->lock);
 
-	if (!scsi->pc || (req = scsi->pc->rq) != HWGROUP(drive)->rq || !HWGROUP(drive)->handler) {
+	if (!scsi->pc || (req = scsi->pc->rq) != hwgroup->rq || !hwgroup->handler) {
 		printk (KERN_WARNING "ide-scsi: No active request in idescsi_eh_reset\n");
-		spin_unlock(&ide_lock);
+		spin_unlock(&hwgroup->lock);
 		spin_unlock_irq(cmd->device->host->host_lock);
 		return FAILED;
 	}
@@ -1052,10 +1054,10 @@
 		end_that_request_last(req);
 	}
 
-	HWGROUP(drive)->rq = NULL;
-	HWGROUP(drive)->handler = NULL;
-	HWGROUP(drive)->busy = 1;		/* will set this to zero when ide reset finished */
-	spin_unlock(&ide_lock);
+	hwgroup->rq = NULL;
+	hwgroup->handler = NULL;
+	hwgroup->busy = 1;		/* will set this to zero when ide reset finished */
+	spin_unlock(&hwgroup->lock);
 
 	ide_do_reset(drive);
 
Index: linux-2.6.13/include/linux/ide.h
===================================================================
--- linux-2.6.13.orig/include/linux/ide.h	2005-09-06 11:22:29.000000000 -0700
+++ linux-2.6.13/include/linux/ide.h	2005-09-06 11:34:43.000000000 -0700
@@ -961,6 +961,7 @@
 	int pio_clock;
 
 	unsigned char cmd_buf[4];
+	spinlock_t lock;
 } ide_hwgroup_t;
 
 /* structure attached to the request for IDE_TASK_CMDS */
@@ -1480,12 +1481,12 @@
 /*
  * Structure locking:
  *
- * ide_cfg_sem and ide_lock together protect changes to
+ * ide_cfg_sem and hwgroup->lock together protect changes to
  * ide_hwif_t->{next,hwgroup}
  * ide_drive_t->next
  *
- * ide_hwgroup_t->busy: ide_lock
- * ide_hwgroup_t->hwif: ide_lock
+ * ide_hwgroup_t->busy: hwgroup->lock
+ * ide_hwgroup_t->hwif: hwgroup->lock
  * ide_hwif_t->mate: constant, no locking
  * ide_drive_t->hwif: constant, no locking
  */

Re: [patch 2/4] ide: Break ide_lock -- replace ide_lock with hwgroup->lock in core ide

[Ravikiran G Thirumalai]

On Tue, Sep 06, 2005 at 04:40:28PM -0700, Ravikiran G Thirumalai wrote:
> Patch to convert ide core code to use hwgroup lock instead of a global
> ide_lock.
>  
> Index: linux-2.6.13/drivers/ide/ide-io.c
> ===================================================================
> --- linux-2.6.13.orig/drivers/ide/ide-io.c	2005-09-06 11:22:29.000000000 -0700
> @@ -1211,11 +1214,11 @@
>  		 */
>  		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();
>  			/* allow other IRQs while we start this request */
>  		startstop = start_request(drive, rq);
> -		spin_lock_irq(&ide_lock);
> +		spin_unlock_irq(&hwgroup->lock);
		^^^^^^^^^^^^^^^^^^^^^^^

My bad,
Fixed patch attached.

Thanks,
Kiran


Patch to convert ide core code to use hwgroup lock instead of a global
ide_lock.

Signed-off-by: Vaibhav V. Nivargi <vaibhav.nivargi@gmail.com>
Signed-off-by: Alok N. Kataria <alokk@calsoftinc.com>
Signed-off-by: Ravikiran Thirumalai <kiran@scalex86.org>
Signed-off-by: Shai Fultheim <shai@scalex86.org>

Index: linux-2.6.13/drivers/ide/ide-cd.c
===================================================================
--- linux-2.6.13.orig/drivers/ide/ide-cd.c	2005-09-06 15:08:07.000000000 -0700
+++ linux-2.6.13/drivers/ide/ide-cd.c	2005-09-06 15:16:15.000000000 -0700
@@ -590,7 +590,8 @@
 
 static void cdrom_end_request (ide_drive_t *drive, int uptodate)
 {
-	struct request *rq = HWGROUP(drive)->rq;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
+	struct request *rq = hwgroup->rq;
 	int nsectors = rq->hard_cur_sectors;
 
 	if ((rq->flags & REQ_SENSE) && uptodate) {
@@ -612,10 +613,10 @@
 			/*
 			 * now end failed request
 			 */
-			spin_lock_irqsave(&ide_lock, flags);
+			spin_lock_irqsave(&hwgroup->lock, flags);
 			end_that_request_chunk(failed, 0, failed->data_len);
 			end_that_request_last(failed);
-			spin_unlock_irqrestore(&ide_lock, flags);
+			spin_unlock_irqrestore(&hwgroup->lock, flags);
 		}
 
 		cdrom_analyze_sense_data(drive, failed, sense);
@@ -636,7 +637,8 @@
    Returns 1 if the request was ended. */
 static int cdrom_decode_status(ide_drive_t *drive, int good_stat, int *stat_ret)
 {
-	struct request *rq = HWGROUP(drive)->rq;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
+	struct request *rq = hwgroup->rq;
 	int stat, err, sense_key;
 	
 	/* Check for errors. */
@@ -698,10 +700,10 @@
 		 * request sense has completed
 		 */
 		if (stat & ERR_STAT) {
-			spin_lock_irqsave(&ide_lock, flags);
+			spin_lock_irqsave(&hwgroup->lock, flags);
 			blkdev_dequeue_request(rq);
 			HWGROUP(drive)->rq = NULL;
-			spin_unlock_irqrestore(&ide_lock, flags);
+			spin_unlock_irqrestore(&hwgroup->lock, flags);
 
 			cdrom_queue_request_sense(drive, rq->sense, rq);
 		} else
@@ -741,9 +743,9 @@
 					 * take a breather relying on the
 					 * unplug timer to kick us again
 					 */
-					spin_lock_irqsave(&ide_lock, flags);
+					spin_lock_irqsave(&hwgroup->lock, flags);
 					blk_plug_device(drive->queue);
-					spin_unlock_irqrestore(&ide_lock,flags);
+					spin_unlock_irqrestore(&hwgroup->lock, flags);
 					return 1;
 				}
 			}
@@ -839,6 +841,7 @@
 	ide_startstop_t startstop;
 	struct cdrom_info *info = drive->driver_data;
 	ide_hwif_t *hwif = drive->hwif;
+	ide_hwgroup_t *hwgroup = hwif->hwgroup;
 
 	/* Wait for the controller to be idle. */
 	if (ide_wait_stat(&startstop, drive, 0, BUSY_STAT, WAIT_READY))
@@ -866,10 +869,10 @@
 		unsigned long flags;
 
 		/* packet command */
-		spin_lock_irqsave(&ide_lock, flags);
+		spin_lock_irqsave(&hwgroup->lock, flags);
 		hwif->OUTBSYNC(drive, WIN_PACKETCMD, IDE_COMMAND_REG);
 		ndelay(400);
-		spin_unlock_irqrestore(&ide_lock, flags);
+		spin_unlock_irqrestore(&hwgroup->lock, flags);
 
 		return (*handler) (drive);
 	}
@@ -1609,7 +1612,8 @@
 static ide_startstop_t cdrom_newpc_intr(ide_drive_t *drive)
 {
 	struct cdrom_info *info = drive->driver_data;
-	struct request *rq = HWGROUP(drive)->rq;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
+	struct request *rq = hwgroup->rq;
 	int dma_error, dma, stat, ireason, len, thislen;
 	u8 lowcyl, highcyl;
 	xfer_func_t *xferfunc;
@@ -1737,11 +1741,11 @@
 	if (!rq->data_len)
 		post_transform_command(rq);
 
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
 	blkdev_dequeue_request(rq);
 	end_that_request_last(rq);
 	HWGROUP(drive)->rq = NULL;
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 	return ide_stopped;
 }
 
Index: linux-2.6.13/drivers/ide/ide-disk.c
===================================================================
--- linux-2.6.13.orig/drivers/ide/ide-disk.c	2005-09-06 15:08:07.000000000 -0700
+++ linux-2.6.13/drivers/ide/ide-disk.c	2005-09-06 15:16:15.000000000 -0700
@@ -786,11 +786,12 @@
 
 static int set_nowerr(ide_drive_t *drive, int arg)
 {
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 	if (ide_spin_wait_hwgroup(drive))
 		return -EBUSY;
 	drive->nowerr = arg;
 	drive->bad_wstat = arg ? BAD_R_STAT : BAD_W_STAT;
-	spin_unlock_irq(&ide_lock);
+	spin_unlock_irq(&hwgroup->lock);
 	return 0;
 }
 
Index: linux-2.6.13/drivers/ide/ide-io.c
===================================================================
--- linux-2.6.13.orig/drivers/ide/ide-io.c	2005-09-06 15:08:07.000000000 -0700
+++ linux-2.6.13/drivers/ide/ide-io.c	2005-09-06 22:35:09.000000000 -0700
@@ -112,9 +112,10 @@
 	struct request *rq;
 	unsigned long flags;
 	int ret = 1;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
-	spin_lock_irqsave(&ide_lock, flags);
-	rq = HWGROUP(drive)->rq;
+	spin_lock_irqsave(&hwgroup->lock, flags);
+	rq = hwgroup->rq;
 
 	if (!nr_sectors)
 		nr_sectors = rq->hard_cur_sectors;
@@ -124,7 +125,7 @@
 	else
 		ret = __ide_end_request(drive, rq, uptodate, nr_sectors);
 
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 	return ret;
 }
 EXPORT_SYMBOL(ide_end_request);
@@ -233,12 +234,13 @@
 static void ide_complete_pm_request (ide_drive_t *drive, struct request *rq)
 {
 	unsigned long flags;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
 #ifdef DEBUG_PM
 	printk("%s: completing PM request, %s\n", drive->name,
 	       blk_pm_suspend_request(rq) ? "suspend" : "resume");
 #endif
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
 	if (blk_pm_suspend_request(rq)) {
 		blk_stop_queue(drive->queue);
 	} else {
@@ -248,7 +250,7 @@
 	blkdev_dequeue_request(rq);
 	HWGROUP(drive)->rq = NULL;
 	end_that_request_last(rq);
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 }
 
 /*
@@ -305,10 +307,11 @@
 	ide_hwif_t *hwif = HWIF(drive);
 	unsigned long flags;
 	struct request *rq;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
-	spin_lock_irqsave(&ide_lock, flags);
-	rq = HWGROUP(drive)->rq;
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
+	rq = hwgroup->rq;
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 
 	if (rq->flags & REQ_DRIVE_CMD) {
 		u8 *args = (u8 *) rq->buffer;
@@ -375,12 +378,12 @@
 		return;
 	}
 
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
 	blkdev_dequeue_request(rq);
-	HWGROUP(drive)->rq = NULL;
+	hwgroup->rq = NULL;
 	rq->errors = err;
 	end_that_request_last(rq);
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 }
 
 EXPORT_SYMBOL(ide_end_drive_cmd);
@@ -1062,7 +1065,7 @@
 
 /*
  * 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)
@@ -1074,7 +1077,7 @@
  * 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
@@ -1090,7 +1093,7 @@
  * 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(hwgroup->lock) 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.
@@ -1105,7 +1108,7 @@
 	/* for atari only: POSSIBLY BROKEN HERE(?) */
 	ide_get_lock(ide_intr, hwgroup);
 
-	/* caller must own ide_lock */
+	/* caller must own hwgroup->lock */
 	BUG_ON(!irqs_disabled());
 
 	while (!hwgroup->busy) {
@@ -1211,11 +1214,11 @@
 		 */
 		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();
 			/* 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)
@@ -1309,7 +1312,7 @@
 	unsigned long	flags;
 	unsigned long	wait = -1;
 
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
 
 	if ((handler = hwgroup->handler) == NULL) {
 		/*
@@ -1340,7 +1343,7 @@
 					/* reset timer */
 					hwgroup->timer.expires  = jiffies + wait;
 					add_timer(&hwgroup->timer);
-					spin_unlock_irqrestore(&ide_lock, flags);
+					spin_unlock_irqrestore(&hwgroup->lock, flags);
 					return;
 				}
 			}
@@ -1350,7 +1353,7 @@
 			 * the handler() function, which means we need to
 			 * globally mask the specific IRQ:
 			 */
-			spin_unlock(&ide_lock);
+			spin_unlock(&hwgroup->lock);
 			hwif  = HWIF(drive);
 #if DISABLE_IRQ_NOSYNC
 			disable_irq_nosync(hwif->irq);
@@ -1377,14 +1380,14 @@
 					ide_error(drive, "irq timeout", hwif->INB(IDE_STATUS_REG));
 			}
 			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);
 }
 
 /**
@@ -1481,11 +1484,11 @@
 	ide_handler_t *handler;
 	ide_startstop_t startstop;
 
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
 	hwif = hwgroup->hwif;
 
 	if (!ide_ack_intr(hwif)) {
-		spin_unlock_irqrestore(&ide_lock, flags);
+		spin_unlock_irqrestore(&hwgroup->lock, flags);
 		return IRQ_NONE;
 	}
 
@@ -1523,7 +1526,7 @@
 			(void) hwif->INB(hwif->io_ports[IDE_STATUS_OFFSET]);
 #endif /* CONFIG_BLK_DEV_IDEPCI */
 		}
-		spin_unlock_irqrestore(&ide_lock, flags);
+		spin_unlock_irqrestore(&hwgroup->lock, flags);
 		return IRQ_NONE;
 	}
 	drive = hwgroup->drive;
@@ -1534,7 +1537,7 @@
 		 *
 		 * [Note - this can occur if the drive is hot unplugged]
 		 */
-		spin_unlock_irqrestore(&ide_lock, flags);
+		spin_unlock_irqrestore(&hwgroup->lock, flags);
 		return IRQ_HANDLED;
 	}
 	if (!drive_is_ready(drive)) {
@@ -1545,7 +1548,7 @@
 		 * their status register is up to date.  Hopefully we have
 		 * enough advance overhead that the latter isn't a problem.
 		 */
-		spin_unlock_irqrestore(&ide_lock, flags);
+		spin_unlock_irqrestore(&hwgroup->lock, flags);
 		return IRQ_NONE;
 	}
 	if (!hwgroup->busy) {
@@ -1554,13 +1557,13 @@
 	}
 	hwgroup->handler = NULL;
 	del_timer(&hwgroup->timer);
-	spin_unlock(&ide_lock);
+	spin_unlock(&hwgroup->lock);
 
 	if (drive->unmask)
 		local_irq_enable();
 	/* 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
@@ -1579,7 +1582,7 @@
 				"on exit\n", drive->name);
 		}
 	}
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 	return IRQ_HANDLED;
 }
 
@@ -1654,7 +1657,7 @@
 		rq->end_io = blk_end_sync_rq;
 	}
 
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
 	if (action == ide_preempt)
 		hwgroup->rq = NULL;
 	if (action == ide_preempt || action == ide_head_wait) {
@@ -1663,7 +1666,7 @@
 	}
 	__elv_add_request(drive->queue, rq, where, 0);
 	ide_do_request(hwgroup, IDE_NO_IRQ);
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 
 	err = 0;
 	if (must_wait) {
Index: linux-2.6.13/drivers/ide/ide-iops.c
===================================================================
--- linux-2.6.13.orig/drivers/ide/ide-iops.c	2005-09-06 15:08:07.000000000 -0700
+++ linux-2.6.13/drivers/ide/ide-iops.c	2005-09-06 15:16:15.000000000 -0700
@@ -945,9 +945,10 @@
 		      unsigned int timeout, ide_expiry_t *expiry)
 {
 	unsigned long flags;
-	spin_lock_irqsave(&ide_lock, flags);
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
+	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);
@@ -972,7 +973,7 @@
 	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 	ide_hwif_t *hwif = HWIF(drive);
 	
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
 	
 	if(hwgroup->handler)
 		BUG();
@@ -988,7 +989,7 @@
 	   devices 
 	*/
 	ndelay(400);
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 }
 
 EXPORT_SYMBOL(ide_execute_command);
@@ -1168,10 +1169,11 @@
 	ide_hwif_t *hwif;
 	ide_hwgroup_t *hwgroup;
 	
-	spin_lock_irqsave(&ide_lock, flags);
 	hwif = HWIF(drive);
 	hwgroup = HWGROUP(drive);
 
+	spin_lock_irqsave(&hwgroup->lock, flags);
+
 	/* We must not reset with running handlers */
 	if(hwgroup->handler != NULL)
 		BUG();
@@ -1186,7 +1188,7 @@
 		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;
 	}
 
@@ -1199,7 +1201,7 @@
 
 #if OK_TO_RESET_CONTROLLER
 	if (!IDE_CONTROL_REG) {
-		spin_unlock_irqrestore(&ide_lock, flags);
+		spin_unlock_irqrestore(&hwgroup->lock, flags);
 		return ide_stopped;
 	}
 
@@ -1239,7 +1241,7 @@
 	
 #endif	/* OK_TO_RESET_CONTROLLER */
 
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 	return ide_started;
 }
 
Index: linux-2.6.13/drivers/ide/ide-lib.c
===================================================================
--- linux-2.6.13.orig/drivers/ide/ide-lib.c	2005-09-06 15:08:07.000000000 -0700
+++ linux-2.6.13/drivers/ide/ide-lib.c	2005-09-06 15:16:15.000000000 -0700
@@ -450,12 +450,11 @@
 	struct request *rq;
 	u8 opcode = 0;
 	int found = 0;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
-	spin_lock(&ide_lock);
-	rq = NULL;
-	if (HWGROUP(drive))
-		rq = HWGROUP(drive)->rq;
-	spin_unlock(&ide_lock);
+	spin_lock(&hwgroup->lock);
+	rq = hwgroup->rq;
+	spin_unlock(&hwgroup->lock);
 	if (!rq)
 		return;
 	if (rq->flags & (REQ_DRIVE_CMD | REQ_DRIVE_TASK)) {
Index: linux-2.6.13/drivers/ide/ide-probe.c
===================================================================
--- linux-2.6.13.orig/drivers/ide/ide-probe.c	2005-09-06 15:12:58.000000000 -0700
+++ linux-2.6.13/drivers/ide/ide-probe.c	2005-09-06 15:16:15.000000000 -0700
@@ -985,7 +985,8 @@
 	 *	do not.
 	 */
 
-	q = blk_init_queue_node(do_ide_request, &ide_lock, hwif_to_node(hwif));
+	q = blk_init_queue_node(do_ide_request, &(HWGROUP(drive)->lock), 
+				hwif_to_node(hwif));
 	if (!q)
 		return 1;
 
@@ -1099,10 +1100,10 @@
 		 * linked list, the first entry is the hwif that owns
 		 * hwgroup->handler - do not change that.
 		 */
-		spin_lock_irq(&ide_lock);
+		spin_lock_irq(&hwgroup->lock);
 		hwif->next = hwgroup->hwif->next;
 		hwgroup->hwif->next = hwif;
-		spin_unlock_irq(&ide_lock);
+		spin_unlock_irq(&hwgroup->lock);
 	} else {
 		hwgroup = kmalloc_node(sizeof(ide_hwgroup_t), GFP_KERNEL,
 					hwif_to_node(hwif->drives[0].hwif));
@@ -1112,6 +1113,7 @@
 		hwif->hwgroup = hwgroup;
 
 		memset(hwgroup, 0, sizeof(ide_hwgroup_t));
+		spin_lock_init(&hwgroup->lock);
 		hwgroup->hwif     = hwif->next = hwif;
 		hwgroup->rq       = NULL;
 		hwgroup->handler  = NULL;
@@ -1159,7 +1161,7 @@
 			printk(KERN_ERR "ide: failed to init %s\n",drive->name);
 			continue;
 		}
-		spin_lock_irq(&ide_lock);
+		spin_lock_irq(&hwgroup->lock);
 		if (!hwgroup->drive) {
 			/* first drive for hwgroup. */
 			drive->next = drive;
@@ -1169,7 +1171,7 @@
 			drive->next = hwgroup->drive->next;
 			hwgroup->drive->next = drive;
 		}
-		spin_unlock_irq(&ide_lock);
+		spin_unlock_irq(&hwgroup->lock);
 	}
 
 #if !defined(__mc68000__) && !defined(CONFIG_APUS) && !defined(__sparc__)
@@ -1193,13 +1195,13 @@
 	up(&ide_cfg_sem);
 	return 0;
 out_unlink:
-	spin_lock_irq(&ide_lock);
 	if (hwif->next == hwif) {
 		BUG_ON(match);
 		BUG_ON(hwgroup->hwif != hwif);
 		kfree(hwgroup);
 	} else {
 		ide_hwif_t *g;
+		spin_lock_irq(&hwgroup->lock);
 		g = hwgroup->hwif;
 		while (g->next != hwif)
 			g = g->next;
@@ -1210,8 +1212,8 @@
 			hwgroup->hwif = g;
 		}
 		BUG_ON(hwgroup->hwif == hwif);
+		spin_unlock_irq(&hwgroup->lock);
 	}
-	spin_unlock_irq(&ide_lock);
 out_up:
 	up(&ide_cfg_sem);
 	return 1;
@@ -1316,8 +1318,9 @@
 static void drive_release_dev (struct device *dev)
 {
 	ide_drive_t *drive = container_of(dev, ide_drive_t, gendev);
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
-	spin_lock_irq(&ide_lock);
+	spin_lock_irq(&hwgroup->lock);
 	if (drive->devfs_name[0] != '\0') {
 		devfs_remove(drive->devfs_name);
 		drive->devfs_name[0] = '\0';
@@ -1329,11 +1332,11 @@
 	}
 	drive->present = 0;
 	/* Messed up locking ... */
-	spin_unlock_irq(&ide_lock);
+	spin_unlock_irq(&hwgroup->lock);
 	blk_cleanup_queue(drive->queue);
-	spin_lock_irq(&ide_lock);
+	spin_lock_irq(&hwgroup->lock);
 	drive->queue = NULL;
-	spin_unlock_irq(&ide_lock);
+	spin_unlock_irq(&hwgroup->lock);
 
 	up(&drive->gendev_rel_sem);
 }
Index: linux-2.6.13/drivers/ide/ide.c
===================================================================
--- linux-2.6.13.orig/drivers/ide/ide.c	2005-09-06 15:08:07.000000000 -0700
+++ linux-2.6.13/drivers/ide/ide.c	2005-09-06 15:16:15.000000000 -0700
@@ -175,7 +175,13 @@
 static int initializing;	/* set while initializing built-in drivers */
 
 DECLARE_MUTEX(ide_cfg_sem);
- __cacheline_aligned_in_smp DEFINE_SPINLOCK(ide_lock);
+
+/* 
+ * Global ide_lock is broken down to per HWGROUP lock. For controller 
+ * serialization when hwgroups are not present, use per-controller
+ * spinlocks.  See drivers/ide/pci/piix.c
+ */
+ __cacheline_aligned_in_smp DEFINE_SPINLOCK(ide_lock __deprecated);
 
 #ifdef CONFIG_BLK_DEV_IDEPCI
 static int ide_scan_direction; /* THIS was formerly 2.2.x pci=reverse */
@@ -587,10 +593,15 @@
 	BUG_ON(in_interrupt());
 	BUG_ON(irqs_disabled());
 	down(&ide_cfg_sem);
-	spin_lock_irq(&ide_lock);
 	hwif = &ide_hwifs[index];
-	if (!hwif->present)
-		goto abort;
+	hwgroup = hwif->hwgroup;
+	spin_lock_irq(&hwgroup->lock);
+	if (!hwif->present) {
+		spin_unlock_irq(&hwgroup->lock);
+		up(&ide_cfg_sem);
+		return;
+	}
+
 	for (unit = 0; unit < MAX_DRIVES; ++unit) {
 		drive = &hwif->drives[unit];
 		if (!drive->present) {
@@ -600,18 +611,17 @@
 			}
 			continue;
 		}
-		spin_unlock_irq(&ide_lock);
+		spin_unlock_irq(&hwgroup->lock);
 		device_unregister(&drive->gendev);
 		down(&drive->gendev_rel_sem);
-		spin_lock_irq(&ide_lock);
+		spin_lock_irq(&hwgroup->lock);
 	}
 	hwif->present = 0;
 
-	spin_unlock_irq(&ide_lock);
+	spin_unlock_irq(&hwgroup->lock);
 
 	destroy_proc_ide_interface(hwif);
 
-	hwgroup = hwif->hwgroup;
 	/*
 	 * free the irq if we were the only hwif using it
 	 */
@@ -624,7 +634,7 @@
 	if (irq_count == 1)
 		free_irq(hwif->irq, hwgroup);
 
-	spin_lock_irq(&ide_lock);
+	spin_lock_irq(&hwgroup->lock);
 	/*
 	 * Note that we only release the standard ports,
 	 * and do not even try to handle any extra ports
@@ -638,6 +648,7 @@
 	 */
 	if (hwif->next == hwif) {
 		BUG_ON(hwgroup->hwif != hwif);
+		spin_unlock_irq(&hwgroup->lock);
 		kfree(hwgroup);
 	} else {
 		/* There is another interface in hwgroup.
@@ -657,10 +668,9 @@
 			hwgroup->hwif = g;
 		}
 		BUG_ON(hwgroup->hwif == hwif);
+		spin_unlock_irq(&hwgroup->lock);
 	}
 
-	/* More messed up locking ... */
-	spin_unlock_irq(&ide_lock);
 	device_unregister(&hwif->gendev);
 	down(&hwif->gendev_rel_sem);
 
@@ -670,7 +680,6 @@
 	blk_unregister_region(MKDEV(hwif->major, 0), MAX_DRIVES<<PARTN_BITS);
 	kfree(hwif->sg_table);
 	unregister_blkdev(hwif->major, hwif->name);
-	spin_lock_irq(&ide_lock);
 
 	if (hwif->dma_base) {
 		(void) ide_release_dma(hwif);
@@ -692,9 +701,6 @@
 	init_hwif_default(hwif, index);
 
 	ide_hwif_restore(hwif, &tmp_hwif);
-
-abort:
-	spin_unlock_irq(&ide_lock);
 	up(&ide_cfg_sem);
 }
 
@@ -1012,9 +1018,10 @@
 {
 	int		val = -EINVAL;
 	unsigned long	flags;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
 	if ((setting->rw & SETTING_READ)) {
-		spin_lock_irqsave(&ide_lock, flags);
+		spin_lock_irqsave(&hwgroup->lock, flags);
 		switch(setting->data_type) {
 			case TYPE_BYTE:
 				val = *((u8 *) setting->data);
@@ -1027,7 +1034,7 @@
 				val = *((u32 *) setting->data);
 				break;
 		}
-		spin_unlock_irqrestore(&ide_lock, flags);
+		spin_unlock_irqrestore(&hwgroup->lock, flags);
 	}
 	return val;
 }
@@ -1037,7 +1044,7 @@
  *	@drive: drive in the group
  *
  *	Wait for an IDE device group to go non busy and then return
- *	holding the ide_lock which guards the hwgroup->busy status
+ *	holding the hwgroup->lock which guards the hwgroup->busy status
  *	and right to use it.
  */
 
@@ -1046,11 +1053,11 @@
 	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 	unsigned long timeout = jiffies + (3 * HZ);
 
-	spin_lock_irq(&ide_lock);
+	spin_lock_irq(&hwgroup->lock);
 
 	while (hwgroup->busy) {
 		unsigned long lflags;
-		spin_unlock_irq(&ide_lock);
+		spin_unlock_irq(&hwgroup->lock);
 		local_irq_set(lflags);
 		if (time_after(jiffies, timeout)) {
 			local_irq_restore(lflags);
@@ -1058,7 +1065,7 @@
 			return -EBUSY;
 		}
 		local_irq_restore(lflags);
-		spin_lock_irq(&ide_lock);
+		spin_lock_irq(&hwgroup->lock);
 	}
 	return 0;
 }
@@ -1087,6 +1094,7 @@
 {
 	int i;
 	u32 *p;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
@@ -1114,7 +1122,7 @@
 				*p = val;
 			break;
 	}
-	spin_unlock_irq(&ide_lock);
+	spin_unlock_irq(&hwgroup->lock);
 	return 0;
 }
 
@@ -1260,6 +1268,7 @@
 	ide_driver_t *drv;
 	int err = 0;
 	void __user *p = (void __user *)arg;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
 	down(&ide_setting_sem);
 	if ((setting = ide_find_setting_by_ioctl(drive, cmd)) != NULL) {
@@ -1377,18 +1386,18 @@
 			 *	spot if we miss one somehow
 			 */
 
-			spin_lock_irqsave(&ide_lock, flags);
+			spin_lock_irqsave(&hwgroup->lock, flags);
 
 			ide_abort(drive, "drive reset");
 
-			if(HWGROUP(drive)->handler)
+			if(hwgroup->handler)
 				BUG();
 				
 			/* Ensure nothing gets queued after we
 			   drop the lock. Reset will clear the busy */
 		   
-			HWGROUP(drive)->busy = 1;
-			spin_unlock_irqrestore(&ide_lock, flags);
+			hwgroup->busy = 1;
+			spin_unlock_irqrestore(&hwgroup->lock, flags);
 			(void) ide_do_reset(drive);
 
 			return 0;
@@ -1874,21 +1883,22 @@
  *	Disconnect a drive from the driver it was attached to and then
  *	clean up the various proc files and other objects attached to it.
  *
- *	Takes ide_setting_sem and ide_lock.
+ *	Takes ide_setting_sem and hwgroup->lock.
  *	Caller must hold none of the locks.
  */
 
 void ide_unregister_subdriver(ide_drive_t *drive, ide_driver_t *driver)
 {
 	unsigned long flags;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 	
 	down(&ide_setting_sem);
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
 #ifdef CONFIG_PROC_FS
 	ide_remove_proc_entries(drive->proc, driver->proc);
 #endif
 	auto_remove_settings(drive);
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 	up(&ide_setting_sem);
 }
 
Index: linux-2.6.13/drivers/scsi/ide-scsi.c
===================================================================
--- linux-2.6.13.orig/drivers/scsi/ide-scsi.c	2005-09-06 15:08:07.000000000 -0700
+++ linux-2.6.13/drivers/scsi/ide-scsi.c	2005-09-06 15:16:15.000000000 -0700
@@ -956,6 +956,7 @@
 	ide_drive_t    *drive = scsi->drive;
 	int		busy;
 	int             ret   = FAILED;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
 	/* In idescsi_eh_abort we try to gently pry our command from the ide subsystem */
 
@@ -974,7 +975,7 @@
 	if (test_bit(IDESCSI_LOG_CMD, &scsi->log))
 		printk (KERN_WARNING "ide-scsi: drive did%s become ready\n", busy?" not":"");
 
-	spin_lock_irq(&ide_lock);
+	spin_lock_irq(&hwgroup->lock);
 
 	/* If there is no pc running we're done (our interrupt took care of it) */
 	if (!scsi->pc) {
@@ -1000,7 +1001,7 @@
 	}
 
 ide_unlock:
-	spin_unlock_irq(&ide_lock);
+	spin_unlock_irq(&hwgroup->lock);
 no_drive:
 	if (test_bit(IDESCSI_LOG_CMD, &scsi->log))
 		printk (KERN_WARNING "ide-scsi: abort returns %s\n", ret == SUCCESS?"success":"failed");
@@ -1015,6 +1016,7 @@
 	ide_drive_t    *drive = scsi->drive;
 	int             ready = 0;
 	int             ret   = SUCCESS;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
 	/* In idescsi_eh_reset we forcefully remove the command from the ide subsystem and reset the device. */
 
@@ -1028,11 +1030,11 @@
 	}
 
 	spin_lock_irq(cmd->device->host->host_lock);
-	spin_lock(&ide_lock);
+	spin_lock(&hwgroup->lock);
 
-	if (!scsi->pc || (req = scsi->pc->rq) != HWGROUP(drive)->rq || !HWGROUP(drive)->handler) {
+	if (!scsi->pc || (req = scsi->pc->rq) != hwgroup->rq || !hwgroup->handler) {
 		printk (KERN_WARNING "ide-scsi: No active request in idescsi_eh_reset\n");
-		spin_unlock(&ide_lock);
+		spin_unlock(&hwgroup->lock);
 		spin_unlock_irq(cmd->device->host->host_lock);
 		return FAILED;
 	}
@@ -1052,10 +1054,10 @@
 		end_that_request_last(req);
 	}
 
-	HWGROUP(drive)->rq = NULL;
-	HWGROUP(drive)->handler = NULL;
-	HWGROUP(drive)->busy = 1;		/* will set this to zero when ide reset finished */
-	spin_unlock(&ide_lock);
+	hwgroup->rq = NULL;
+	hwgroup->handler = NULL;
+	hwgroup->busy = 1;		/* will set this to zero when ide reset finished */
+	spin_unlock(&hwgroup->lock);
 
 	ide_do_reset(drive);
 
Index: linux-2.6.13/include/linux/ide.h
===================================================================
--- linux-2.6.13.orig/include/linux/ide.h	2005-09-06 15:08:07.000000000 -0700
+++ linux-2.6.13/include/linux/ide.h	2005-09-06 15:16:15.000000000 -0700
@@ -961,6 +961,7 @@
 	int pio_clock;
 
 	unsigned char cmd_buf[4];
+	spinlock_t lock;
 } ide_hwgroup_t;
 
 /* structure attached to the request for IDE_TASK_CMDS */
@@ -1480,12 +1481,12 @@
 /*
  * Structure locking:
  *
- * ide_cfg_sem and ide_lock together protect changes to
+ * ide_cfg_sem and hwgroup->lock together protect changes to
  * ide_hwif_t->{next,hwgroup}
  * ide_drive_t->next
  *
- * ide_hwgroup_t->busy: ide_lock
- * ide_hwgroup_t->hwif: ide_lock
+ * ide_hwgroup_t->busy: hwgroup->lock
+ * ide_hwgroup_t->hwif: hwgroup->lock
  * ide_hwif_t->mate: constant, no locking
  * ide_drive_t->hwif: constant, no locking
  */

[patch 3/4] ide: Break ide_lock -- change controller drivers

[Ravikiran G Thirumalai]

Patch to make ide-host controllers use hwgroup lock where serialization with
hwgroup->lock is necessary


Signed-off-by: Vaibhav V. Nivargi <vaibhav.nivargi@gmail.com>
Signed-off-by: Alok N. Kataria <alokk@calsoftinc.com>
Signed-off-by: Ravikiran Thirumalai <kiran@scalex86.org>

Index: linux-2.6.13/drivers/ide/legacy/ht6560b.c
===================================================================
--- linux-2.6.13.orig/drivers/ide/legacy/ht6560b.c	2005-08-28 19:41:01.000000000 -0400
+++ linux-2.6.13/drivers/ide/legacy/ht6560b.c	2005-09-06 15:57:46.113541000 -0400
@@ -256,9 +256,11 @@
 {
 	unsigned long flags;
 	int t = HT_PREFETCH_MODE << 8;
-	
-	spin_lock_irqsave(&ide_lock, flags);
-	
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
+
+	spin_lock_irqsave(&hwgroup->lock, flags);
+	spin_lock(&ide_lock);
+
 	/*
 	 *  Prefetch mode and unmask irq seems to conflict
 	 */
@@ -270,9 +272,10 @@
 		drive->drive_data &= ~t;  /* disable prefetch mode */
 		drive->no_unmask = 0;
 	}
-	
-	spin_unlock_irqrestore(&ide_lock, flags);
-	
+
+	spin_unlock(&ide_lock);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
+
 #ifdef DEBUG
 	printk("ht6560b: drive %s prefetch mode %sabled\n", drive->name, (state ? "en" : "dis"));
 #endif
@@ -282,6 +285,7 @@
 {
 	unsigned long flags;
 	u8 timing;
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 	
 	switch (pio) {
 	case 8:         /* set prefetch off */
@@ -291,14 +295,15 @@
 	}
 	
 	timing = ht_pio2timings(drive, pio);
-	
-	spin_lock_irqsave(&ide_lock, flags);
+
+	spin_lock_irqsave(&hwgroup->lock, flags);
+	spin_lock(&ide_lock);
 	
 	drive->drive_data &= 0xff00;
 	drive->drive_data |= timing;
 	
-	spin_unlock_irqrestore(&ide_lock, flags);
-	
+	spin_unlock(&ide_lock);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 #ifdef DEBUG
 	printk("ht6560b: drive %s tuned to pio mode %#x timing=%#x\n", drive->name, pio, timing);
 #endif
Index: linux-2.6.13/drivers/ide/pci/cmd640.c
===================================================================
--- linux-2.6.13.orig/drivers/ide/pci/cmd640.c	2005-08-28 19:41:01.000000000 -0400
+++ linux-2.6.13/drivers/ide/pci/cmd640.c	2005-09-06 15:50:35.330618750 -0400
@@ -442,11 +442,14 @@
 static void set_prefetch_mode (unsigned int index, int mode)
 {
 	ide_drive_t *drive = cmd_drives[index];
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 	int reg = prefetch_regs[index];
 	u8 b;
 	unsigned long flags;
 
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
+	spin_lock(&ide_lock);
+
 	b = __get_cmd640_reg(reg);
 	if (mode) {	/* want prefetch on? */
 #if CMD640_PREFETCH_MASKS
@@ -462,7 +465,9 @@
 		b |= prefetch_masks[index];	/* disable prefetch */
 	}
 	__put_cmd640_reg(reg, b);
-	spin_unlock_irqrestore(&ide_lock, flags);
+
+	spin_unlock(&ide_lock);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 }
 
 /*
Index: linux-2.6.13/drivers/ide/pci/piix.c
===================================================================
--- linux-2.6.13.orig/drivers/ide/pci/piix.c	2005-09-06 15:49:48.271677750 -0400
+++ linux-2.6.13/drivers/ide/pci/piix.c	2005-09-06 15:56:55.982408000 -0400
@@ -215,6 +215,7 @@
 {
 	ide_hwif_t *hwif	= HWIF(drive);
 	struct pci_dev *dev	= hwif->pci_dev;
+	ide_hwgroup_t *hwgroup  = HWGROUP(drive);
 	int is_slave		= (&hwif->drives[1] == drive);
 	int master_port		= hwif->channel ? 0x42 : 0x40;
 	int slave_port		= 0x44;
@@ -229,7 +230,8 @@
 			    { 2, 3 }, };
 
 	pio = ide_get_best_pio_mode(drive, pio, 5, NULL);
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&hwgroup->lock, flags);
+	spin_lock(&ide_lock);
 	pci_read_config_word(dev, master_port, &master_data);
 	if (is_slave) {
 		master_data = master_data | 0x4000;
@@ -249,7 +251,8 @@
 	pci_write_config_word(dev, master_port, master_data);
 	if (is_slave)
 		pci_write_config_byte(dev, slave_port, slave_data);
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock(&ide_lock);
+	spin_unlock_irqrestore(&hwgroup->lock, flags);
 }
 
 /**

[patch 4/4] ide: Break ide_lock -- remove ide_lock from piix driver

[Ravikiran G Thirumalai]

Patch to convert piix driver to use per-driver/hwgroup lock and kill
ide_lock.  In the case of piix, hwgroup->lock should be sufficient.

Signed-off-by: Ravikiran Thirumalai <kiran@scalex86.org>

Index: linux-2.6.13/drivers/ide/pci/piix.c
===================================================================
--- linux-2.6.13.orig/drivers/ide/pci/piix.c	2005-09-06 12:00:25.000000000 -0700
+++ linux-2.6.13/drivers/ide/pci/piix.c	2005-09-06 13:22:49.000000000 -0700
@@ -231,7 +231,6 @@
 
 	pio = ide_get_best_pio_mode(drive, pio, 5, NULL);
 	spin_lock_irqsave(&hwgroup->lock, flags);
-	spin_lock(&ide_lock);
 	pci_read_config_word(dev, master_port, &master_data);
 	if (is_slave) {
 		master_data = master_data | 0x4000;
@@ -251,7 +250,6 @@
 	pci_write_config_word(dev, master_port, master_data);
 	if (is_slave)
 		pci_write_config_byte(dev, slave_port, slave_data);
-	spin_unlock(&ide_lock);
 	spin_unlock_irqrestore(&hwgroup->lock, flags);
 }
 

Re: [patch 4/4] ide: Break ide_lock -- remove ide_lock from piix driver

[Alan Cox]

On Maw, 2005-09-06 at 16:44 -0700, Ravikiran G Thirumalai wrote:
> Patch to convert piix driver to use per-driver/hwgroup lock and kill
> ide_lock.  In the case of piix, hwgroup->lock should be sufficient.

PIIX requires that both channels are quiescent when retuning in some
cases. It wasn't totally safe before, its now totally broken. Start by
fixing the IDE layer locking properly (or forward porting my patches and
then fixing them for all the refcounting changes and other stuff done
since).

Better yet move to the SATA driver - it can drive never PIIX chips in
PATA mode and its in my experience already more reliable.

Alan

Re: [patch 4/4] ide: Break ide_lock -- remove ide_lock from piix driver

[Ravikiran G Thirumalai]

On Wed, Sep 07, 2005 at 06:06:23PM +0100, Alan Cox wrote:
> On Maw, 2005-09-06 at 16:44 -0700, Ravikiran G Thirumalai wrote:
> > Patch to convert piix driver to use per-driver/hwgroup lock and kill
> > ide_lock.  In the case of piix, hwgroup->lock should be sufficient.
> 
> PIIX requires that both channels are quiescent when retuning in some
> cases. It wasn't totally safe before, its now totally broken. Start by

Then the change to piix controller in my patchset is bad, How about changing
the ide_lock to per-driver lock in this case?  Locking for rest of the
controllers in the system is left equivalent to what ide_lock did earlier..


> fixing the IDE layer locking properly (or forward porting my patches and
> then fixing them for all the refcounting changes and other stuff done
> since).

Can you please point me to the patchset...

Thanks,
Kiran

Re: [patch 4/4] ide: Break ide_lock -- remove ide_lock from piix driver

[Alan Cox]

On Mer, 2005-09-07 at 10:50 -0700, Ravikiran G Thirumalai wrote:
> Then the change to piix controller in my patchset is bad, How about changing
> the ide_lock to per-driver lock in this case?  Locking for rest of the
> controllers in the system is left equivalent to what ide_lock did earlier..

Thats what we did in Fedora Core. Its neccessary because the state of
ide_lock is undefined when the tuning functions get called and the base
kernel didn't handle this well - we had various reports from users of
hangs as a result.

Its not a full fix, really PIIX needs to wrap the entire command issue
with a semaphore or similar to avoid the error handling (CRC error)
breaking the driver. Unfortunately we do the CRC error handling and
speed change in an IRQ handler polled so I've never seen how we could
fix that without rewriting the entire error recovery code to work like
the scsi layer (in a thread).

> > fixing the IDE layer locking properly (or forward porting my patches and
> > then fixing them for all the refcounting changes and other stuff done
> > since).
> 
> Can you please point me to the patchset...

2.6.11-ac kernels. I will send you (off list) the last full version of
the fixes. Part of what it fixes (especially the proc file locking
changes) are mostly handled now by the reworking of the IDE code to use
refcounts that Bartlomiej did, and which is definitely the better way to
fix it.

Alan

Re: [patch 0/4] ide: Break ide_lock to per-hwgroup lock

[Jens Axboe]

On Tue, Sep 06 2005, Ravikiran G Thirumalai wrote:
> The following patchset breaks down the global ide_lock to per-hwgroup lock.
> We have taken the following approach.

Curious, what is the point of this?

-- 
Jens Axboe

Re: [patch 0/4] ide: Break ide_lock to per-hwgroup lock

[Ravikiran G Thirumalai]

On Wed, Sep 07, 2005 at 11:19:24AM +0200, Jens Axboe wrote:
> On Tue, Sep 06 2005, Ravikiran G Thirumalai wrote:
> > The following patchset breaks down the global ide_lock to per-hwgroup lock.
> > We have taken the following approach.
> 
> Curious, what is the point of this?
> 

On smp machines with multiple ide interfaces, we take per-group lock instead
of a global lock, there by breaking the lock to per-irq hwgroups.

Thanks,
Kiran

Re: [patch 0/4] ide: Break ide_lock to per-hwgroup lock

[Jens Axboe]

On Wed, Sep 07 2005, Ravikiran G Thirumalai wrote:
> On Wed, Sep 07, 2005 at 11:19:24AM +0200, Jens Axboe wrote:
> > On Tue, Sep 06 2005, Ravikiran G Thirumalai wrote:
> > > The following patchset breaks down the global ide_lock to per-hwgroup lock.
> > > We have taken the following approach.
> > 
> > Curious, what is the point of this?
> > 
> 
> On smp machines with multiple ide interfaces, we take per-group lock instead
> of a global lock, there by breaking the lock to per-irq hwgroups.

I realize the theory behind breaking up locks, I'm just wondering about
this specific case. Please show actual contention data promoting this
specific case, we don't break up locks "just because".

I'm asking because I've never heard anyone complain about IDE lock
contention and a proper patch usually comes with analysis of why it is
needed.

-- 
Jens Axboe

Re: [patch 0/4] ide: Break ide_lock to per-hwgroup lock

[Bartlomiej Zolnierkiewicz]

On 9/7/05, Jens Axboe <axboe@suse.de> wrote:
> On Wed, Sep 07 2005, Ravikiran G Thirumalai wrote:
> > On Wed, Sep 07, 2005 at 11:19:24AM +0200, Jens Axboe wrote:
> > > On Tue, Sep 06 2005, Ravikiran G Thirumalai wrote:
> > > > The following patchset breaks down the global ide_lock to per-hwgroup lock.
> > > > We have taken the following approach.
> > >
> > > Curious, what is the point of this?
> > >
> >
> > On smp machines with multiple ide interfaces, we take per-group lock instead
> > of a global lock, there by breaking the lock to per-irq hwgroups.
>
> I realize the theory behind breaking up locks, I'm just wondering about
> this specific case. Please show actual contention data promoting this
> specific case, we don't break up locks "just because".
>
> I'm asking because I've never heard anyone complain about IDE lock
> contention and a proper patch usually comes with analysis of why it is
> needed.

Since ide_lock spinlock is used for all drives as queue lock and for all
controllers as IDE lock I guess that with multiple controllers there is a lot
contention on it...

Breaking ide_lock is fine with me however seeing numbers would
greatly help in getting wider acceptance for this change, Ravikiran?

Bartlomiej

Re: [patch 0/4] ide: Break ide_lock to per-hwgroup lock

[Ravikiran G Thirumalai]

On Tue, Sep 27, 2005 at 03:36:40PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On 9/7/05, Jens Axboe <axboe@suse.de> wrote:
> > On Wed, Sep 07 2005, Ravikiran G Thirumalai wrote:
> > > On Wed, Sep 07, 2005 at 11:19:24AM +0200, Jens Axboe wrote:
> > > > On Tue, Sep 06 2005, Ravikiran G Thirumalai wrote:
> > > > > The following patchset breaks down the global ide_lock to per-hwgroup lock.
> > > > > We have taken the following approach.
> > > >
> > > > Curious, what is the point of this?
> > > >
> > >
> >
> > I'm asking because I've never heard anyone complain about IDE lock
> > contention and a proper patch usually comes with analysis of why it is
> > needed.
> 
> Since ide_lock spinlock is used for all drives as queue lock and for all
> controllers as IDE lock I guess that with multiple controllers there is a lot
> contention on it...
> 
> Breaking ide_lock is fine with me however seeing numbers would
> greatly help in getting wider acceptance for this change, Ravikiran?
> 

With the lock breakdown, Iozone read performance went up by 5.5% on a 2 way
x86 xeon box, with two hwifs and two processes reading from disks connected
to each hwif.  There seems to be a small  regression on Iozone write
tests -- about 1.5%.  Maybe this is measurement noise as there is no apparent
reason for this.  We plan to run more tests to see if the regression for
writes is statistically significant.

Btw, there was some problem with the moving of tuning code in this patchset
causing disks not to work in DMA mode. We were fixing that; will publish a
newer patchset with test results soon.

Thanks,
Kiran

Re: [patch 0/4] ide: Break ide_lock to per-hwgroup lock

[Jens Axboe]

On Tue, Sep 27 2005, Ravikiran G Thirumalai wrote:
> On Tue, Sep 27, 2005 at 03:36:40PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > On 9/7/05, Jens Axboe <axboe@suse.de> wrote:
> > > On Wed, Sep 07 2005, Ravikiran G Thirumalai wrote:
> > > > On Wed, Sep 07, 2005 at 11:19:24AM +0200, Jens Axboe wrote:
> > > > > On Tue, Sep 06 2005, Ravikiran G Thirumalai wrote:
> > > > > > The following patchset breaks down the global ide_lock to per-hwgroup lock.
> > > > > > We have taken the following approach.
> > > > >
> > > > > Curious, what is the point of this?
> > > > >
> > > >
> > >
> > > I'm asking because I've never heard anyone complain about IDE lock
> > > contention and a proper patch usually comes with analysis of why it is
> > > needed.
> > 
> > Since ide_lock spinlock is used for all drives as queue lock and for all
> > controllers as IDE lock I guess that with multiple controllers there is a lot
> > contention on it...
> > 
> > Breaking ide_lock is fine with me however seeing numbers would
> > greatly help in getting wider acceptance for this change, Ravikiran?
> > 
> 
> With the lock breakdown, Iozone read performance went up by 5.5% on a 2 way
> x86 xeon box, with two hwifs and two processes reading from disks connected
> to each hwif.  There seems to be a small  regression on Iozone write
> tests -- about 1.5%.  Maybe this is measurement noise as there is no apparent
> reason for this.  We plan to run more tests to see if the regression for
> writes is statistically significant.

You should run it eg 10 times on each kernel to get a feel for the
variance of the results. Were you testing 2 or 4 disks?

Some profile information for both kernels would also be paramount.

> Btw, there was some problem with the moving of tuning code in this patchset
> causing disks not to work in DMA mode. We were fixing that; will publish a
> newer patchset with test results soon.

I take it the numbers posted were for DMA enabled on all disks?

-- 
Jens Axboe

Re: [patch 0/4] ide: Break ide_lock to per-hwgroup lock

[Ravikiran G Thirumalai]

On Tue, Sep 27, 2005 at 05:26:42PM +0200, Jens Axboe wrote:
> On Tue, Sep 27 2005, Ravikiran G Thirumalai wrote:
> 
> You should run it eg 10 times on each kernel to get a feel for the
> variance of the results. Were you testing 2 or 4 disks?
> 

Yes, I was planning to do that,  We were testing with 2 disks.

>...
> I take it the numbers posted were for DMA enabled on all disks?

Yes.

Thanks,
Kiran

Re: [patch 0/4] ide: Break ide_lock to per-hwgroup lock

[Jens Axboe]

On Tue, Sep 27 2005, Ravikiran G Thirumalai wrote:
> On Tue, Sep 27, 2005 at 05:26:42PM +0200, Jens Axboe wrote:
> > On Tue, Sep 27 2005, Ravikiran G Thirumalai wrote:
> > 
> > You should run it eg 10 times on each kernel to get a feel for the
> > variance of the results. Were you testing 2 or 4 disks?
> > 
> 
> Yes, I was planning to do that,  We were testing with 2 disks.

With 2 disks, the 5.5% sounds like an aweful lot. What block size were
the tests run with? To get the file system out of the way, you could run
something as simple as a dd on each device. That should show queueing
contention (which I guess is the prime area of contention) more
isolated. And profiles would still be very interesting to see :-)

-- 
Jens Axboe

Re: [patch 0/4] ide: Break ide_lock to per-hwgroup lock

[Alan Cox]

On Maw, 2005-09-06 at 16:33 -0700, Ravikiran G Thirumalai wrote:
> 2. Change the core ide code to use hwgroup->lock instead of ide_lock.
> Deprecate ide_lock (patch 2)

hwgroups and IDE locking requirements are frequently completely
unrelated. Its clear from the changes proposed you've not tested on real
hardware for each case and you have not studied the documented errata
even for the controllers that are publically specified. This is peoples
data you are endangering, not just risking the odd crash.

> patches -- this is done now for piix controller only.  Eventually, 
> we can change all controllers to remove ide_lock

You could start by fixing the real locking bugs. Or at least trying to
get known fixes past the maintainer and going from there.

Alan

Re: [patch 0/4] ide: Break ide_lock to per-hwgroup lock

[Ravikiran G Thirumalai]

On Wed, Sep 07, 2005 at 06:09:10PM +0100, Alan Cox wrote:
> On Maw, 2005-09-06 at 16:33 -0700, Ravikiran G Thirumalai wrote:
> > 2. Change the core ide code to use hwgroup->lock instead of ide_lock.
> > Deprecate ide_lock (patch 2)
> 
> hwgroups and IDE locking requirements are frequently completely
> unrelated. Its clear from the changes proposed you've not tested on real
> hardware for each case and you have not studied the documented errata

I tested it on a 2way box with a piix controller. It got through Bonnie++.  
I have access to piix controllers only, so that was the only controller 
I changed.  I did not read the errata though... :(

Do you think the approach is unsafe, even if the piix tune routine is
serialized with a per-driver lock?  Bartlomiej?

Thanks,
Kiran

Re: [patch 0/4] ide: Break ide_lock to per-hwgroup lock

[Alan Cox]

On Mer, 2005-09-07 at 11:16 -0700, Ravikiran G Thirumalai wrote:
> I tested it on a 2way box with a piix controller. It got through Bonnie++.  
> I have access to piix controllers only, so that was the only controller 
> I changed.  I did not read the errata though... :(

Errata matter a lot unfortunately - older IDE controllers commonly have
bugs where changing the set up of one interface affects the other even
if a transfer is occuring.

> Do you think the approach is unsafe, even if the piix tune routine is
> serialized with a per-driver lock?  Bartlomiej?

If the documents/errata for the chips are checked then I can see it
making sense. I do worry that it may introduce other races because the
IDE locking is such a mess but that would be true of any change and
eventually has to be addressed.

I think it makes sense, with corrections.