[PATCH] 2.5.15 IDE 62

[PATCH] 2.5.15 IDE 62

[Martin Dalecki]

[-- Attachment #1: Type: text/plain, Size: 253 bytes --]

Mon May 13 12:38:11 CEST 2002 ide-clean-62

- Add missing locking around ide_do_request in do_ide_request().

- Check all other places where locks get used for matching pairs in ide.c.

- Streamline device detection reporting to always use ->slot_name.

[-- Attachment #2: ide-clean-62.diff --]
[-- Type: text/plain, Size: 7762 bytes --]

diff -urN linux-2.5.15/drivers/ide/ide.c linux/drivers/ide/ide.c
--- linux-2.5.15/drivers/ide/ide.c	2002-05-13 15:13:11.000000000 +0200
+++ linux/drivers/ide/ide.c	2002-05-13 14:27:39.000000000 +0200
@@ -323,6 +323,7 @@
 	}
 
 	spin_unlock_irqrestore(&ide_lock, flags);
+
 	return ret;
 }
 
@@ -341,6 +342,7 @@
 	ide_hwgroup_t *hwgroup = ch->hwgroup;
 
 	spin_lock_irqsave(&ide_lock, flags);
+
 	if (hwgroup->handler != NULL) {
 		printk("%s: ide_set_handler: handler not null; old=%p, new=%p, from %p\n",
 			drive->name, hwgroup->handler, handler, __builtin_return_address(0));
@@ -349,6 +351,7 @@
 	ch->expiry = expiry;
 	ch->timer.expires = jiffies + timeout;
 	add_timer(&ch->timer);
+
 	spin_unlock_irqrestore(&ide_lock, flags);
 }
 
@@ -1071,8 +1074,10 @@
 	unsigned long flags;
 
 	spin_lock_irqsave(&ide_lock, flags);
+
 	hwgroup->handler = NULL;
 	del_timer(&ch->timer);
+
 	spin_unlock_irqrestore(&ide_lock, flags);
 
 	return start_request(drive, drive->rq);
@@ -1275,10 +1280,12 @@
 			disable_irq_nosync(drive->channel->irq);
 
 		spin_unlock(&ide_lock);
+
 		ide__sti();	/* allow other IRQs while we start this request */
 		startstop = start_request(drive, rq);
 
 		spin_lock_irq(&ide_lock);
+
 		if (masked_irq && drive->channel->irq != masked_irq)
 			enable_irq(drive->channel->irq);
 
@@ -1332,7 +1339,7 @@
 static void ide_do_request(struct ata_channel *channel, int masked_irq)
 {
 	ide_get_lock(&irq_lock, ata_irq_request, hwgroup);/* for atari only: POSSIBLY BROKEN HERE(?) */
-	__cli();	/* necessary paranoia: ensure IRQs are masked on local CPU */
+//	__cli();	/* necessary paranoia: ensure IRQs are masked on local CPU */
 
 	while (!test_and_set_bit(IDE_BUSY, &channel->active)) {
 		struct ata_channel *ch;
@@ -1362,11 +1369,16 @@
 
 		queue_commands(drive, masked_irq);
 	}
+
 }
 
 void do_ide_request(request_queue_t *q)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&ide_lock, flags);
 	ide_do_request(q->queuedata, 0);
+	spin_unlock_irqrestore(&ide_lock, flags);
 }
 
 /*
@@ -1455,7 +1467,9 @@
 					/* reengage timer */
 					ch->timer.expires  = jiffies + wait;
 					add_timer(&ch->timer);
+
 					spin_unlock_irqrestore(&ide_lock, flags);
+
 					return;
 				}
 			}
@@ -1465,7 +1479,9 @@
 			 * the handler() function, which means we need to globally
 			 * mask the specific IRQ:
 			 */
+
 			spin_unlock(&ide_lock);
+
 			ch = drive->channel;
 #if DISABLE_IRQ_NOSYNC
 			disable_irq_nosync(ch->irq);
@@ -1490,7 +1506,9 @@
 			}
 			set_recovery_timer(ch);
 			enable_irq(ch->irq);
+
 			spin_lock_irq(&ide_lock);
+
 			if (startstop == ide_stopped)
 				clear_bit(IDE_BUSY, &ch->active);
 		}
@@ -1621,6 +1639,7 @@
 		printk(KERN_ERR "%s: %s: hwgroup was not busy!?\n", drive->name, __FUNCTION__);
 	hwgroup->handler = NULL;
 	del_timer(&ch->timer);
+
 	spin_unlock(&ide_lock);
 
 	if (ch->unmask)
@@ -1725,7 +1744,9 @@
 	rq->rq_dev = mk_kdev(major,(drive->select.b.unit)<<PARTN_BITS);
 	if (action == ide_wait)
 		rq->waiting = &wait;
+
 	spin_lock_irqsave(&ide_lock, flags);
+
 	if (blk_queue_empty(&drive->queue) || action == ide_preempt) {
 		if (action == ide_preempt)
 			drive->rq = NULL;
@@ -1737,7 +1758,9 @@
 	}
 	q->elevator.elevator_add_req_fn(q, rq, queue_head);
 	ide_do_request(drive->channel, 0);
+
 	spin_unlock_irqrestore(&ide_lock, flags);
+
 	if (action == ide_wait) {
 		wait_for_completion(&wait);	/* wait for it to be serviced */
 		return rq->errors ? -EIO : 0;	/* return -EIO if errors */
@@ -1767,12 +1790,15 @@
 	spin_lock_irqsave(&ide_lock, flags);
 
 	if (drive->busy || (drive->usage > 1)) {
+
 		spin_unlock_irqrestore(&ide_lock, flags);
+
 		return -EBUSY;
 	}
 
 	drive->busy = 1;
 	MOD_INC_USE_COUNT;
+
 	spin_unlock_irqrestore(&ide_lock, flags);
 
 	res = wipe_partitions(i_rdev);
@@ -1789,6 +1815,7 @@
 	drive->busy = 0;
 	wake_up(&drive->wqueue);
 	MOD_DEC_USE_COUNT;
+
 	return res;
 }
 
@@ -1950,6 +1977,7 @@
 	 * All clear?  Then blow away the buffer cache
 	 */
 	spin_unlock_irqrestore(&ide_lock, flags);
+
 	for (unit = 0; unit < MAX_DRIVES; ++unit) {
 		struct ata_device * drive = &ch->drives[unit];
 
@@ -1964,6 +1992,7 @@
 			}
 		}
 	}
+
 	spin_lock_irqsave(&ide_lock, flags);
 
 	/*
@@ -2221,11 +2250,14 @@
 	spin_lock_irq(&ide_lock);
 
 	while (test_bit(IDE_BUSY, &drive->channel->active)) {
+
 		spin_unlock_irq(&ide_lock);
+
 		if (time_after(jiffies, timeout)) {
 			printk("%s: channel busy\n", drive->name);
 			return -EBUSY;
 		}
+
 		spin_lock_irq(&ide_lock);
 	}
 
@@ -3455,7 +3487,7 @@
 #if defined(CONFIG_BLK_DEV_IDE) || defined(CONFIG_BLK_DEV_IDE_MODULE)
 # if defined(__mc68000__) || defined(CONFIG_APUS)
 	if (ide_hwifs[0].io_ports[IDE_DATA_OFFSET]) {
-		ide_get_lock(&irq_lock, NULL, NULL);/* for atari only */
+		// ide_get_lock(&irq_lock, NULL, NULL);/* for atari only */
 		disable_irq(ide_hwifs[0].irq);	/* disable_irq_nosync ?? */
 //		disable_irq_nosync(ide_hwifs[0].irq);
 	}
diff -urN linux-2.5.15/drivers/ide/ide-pci.c linux/drivers/ide/ide-pci.c
--- linux-2.5.15/drivers/ide/ide-pci.c	2002-05-13 15:13:11.000000000 +0200
+++ linux/drivers/ide/ide-pci.c	2002-05-13 15:01:41.000000000 +0200
@@ -553,15 +553,14 @@
 			}
 		}
 	}
-
-	printk("ATA: %s: controller on PCI bus %02x dev %02x\n",
-			dev->name, dev->bus->number, dev->devfn);
+	printk(KERN_INFO "ATA: %s: controller on PCI slot %s dev %02x\n",
+			dev->name, dev->slot_name, dev->devfn);
 	setup_pci_device(dev, d);
 	if (!dev2)
 		return;
 	d2 = d;
-	printk("ATA: %s: controller on PCI bus %02x dev %02x\n",
-			dev2->name, dev2->bus->number, dev2->devfn);
+	printk(KERN_INFO "ATA: %s: controller on PCI slot %s dev %02x\n",
+			dev2->name, dev2->slot_name, dev2->devfn);
 	setup_pci_device(dev2, d2);
 }
 
@@ -584,8 +583,8 @@
 		}
 	}
 
-	printk("%s: IDE controller on PCI bus %02x dev %02x\n",
-		dev->name, dev->bus->number, dev->devfn);
+	printk(KERN_INFO "ATA: %s: controller on PCI slot %s dev %02x\n",
+		dev->name, dev->slot_name, dev->devfn);
 	setup_pci_device(dev, d);
 	if (!dev2) {
 		return;
@@ -601,8 +600,8 @@
 		}
 	}
 	d2 = d;
-	printk("%s: IDE controller on PCI bus %02x dev %02x\n",
-		dev2->name, dev2->bus->number, dev2->devfn);
+	printk(KERN_INFO "ATA: %s: controller on PCI slot %s dev %02x\n",
+		dev2->name, dev2->slot_name, dev2->devfn);
 	setup_pci_device(dev2, d2);
 
 }
@@ -623,7 +622,7 @@
 	switch(class_rev) {
 		case 5:
 		case 4:
-		case 3:	printk("%s: IDE controller on PCI slot %s\n", dev->name, dev->slot_name);
+		case 3:	printk(KERN_INFO "ATA: %s: controller on PCI slot %s\n", dev->name, dev->slot_name);
 			setup_pci_device(dev, d);
 			return;
 		default:	break;
@@ -639,17 +638,17 @@
 			pci_read_config_byte(dev2, PCI_INTERRUPT_PIN, &pin2);
 			if ((pin1 != pin2) && (dev->irq == dev2->irq)) {
 				d->bootable = ON_BOARD;
-				printk("%s: onboard version of chipset, pin1=%d pin2=%d\n", dev->name, pin1, pin2);
+				printk(KERN_INFO "ATAL: %s: onboard version of chipset, pin1=%d pin2=%d\n", dev->name, pin1, pin2);
 			}
 			break;
 		}
 	}
-	printk("%s: IDE controller on PCI slot %s\n", dev->name, dev->slot_name);
+	printk(KERN_INFO "ATA: %s: controller on PCI slot %s\n", dev->name, dev->slot_name);
 	setup_pci_device(dev, d);
 	if (!dev2)
 		return;
 	d2 = d;
-	printk("%s: IDE controller on PCI slot %s\n", dev2->name, dev2->slot_name);
+	printk(KERN_INFO "ATA: %s: controller on PCI slot %s\n", dev2->name, dev2->slot_name);
 	setup_pci_device(dev2, d2);
 }
 
@@ -679,6 +678,10 @@
 	}
 
 	if (!d) {
+		/* Only check the device calls, if it wasn't listed, since
+		 * there are in esp. some pdc202xx chips which "work around"
+		 * beeing grabbed by generic drivers.
+		 */
 		if ((dev->class >> 8) == PCI_CLASS_STORAGE_IDE) {
 			printk(KERN_INFO "ATA: unknown interface: %s, on PCI slot %s\n",
 					dev->name, dev->slot_name);

Re: [PATCH] 2.5.15 IDE 62

[Martin Dalecki]

Uz.ytkownik Jens Axboe napisa?:
> On Mon, May 13 2002, Martin Dalecki wrote:
> 
>>Mon May 13 12:38:11 CEST 2002 ide-clean-62
>>
>>- Add missing locking around ide_do_request in do_ide_request().
> 
> 
> This is broken, do_ide_request() is already called with the request lock
> held. tq_disk run -> generic_unplug_device (grab lock) ->
> __generic_unplug_device -> do_ide_request(). You just introduced a
> deadlock.
> 
> This code would have caused hangs or massive corruption immediately if
> ide_lock wasn't ready held there. Not to mention instant spin_unlock
> BUG() triggers in queue_command()
> 

Oops. Indeed I see now that the ide_lock is exported to
the upper layers above it in ide-probe.c

blk_init_queue(q, do_ide_request, &ide_lock);

But this is problematic in itself, since it means that
we are basically serialiazing between *all* requests
on all channels.

So I think we should have per channel locks on this level
right? This is anyway our unit for serialization.
(I'm just surprised that blk_init_queue() doesn't
provide queue specific locking and relies on exported
locks from the drivers...)

Re: [PATCH] 2.5.15 IDE 62

[Jens Axboe]

On Mon, May 13 2002, Martin Dalecki wrote:
> Mon May 13 12:38:11 CEST 2002 ide-clean-62
> 
> - Add missing locking around ide_do_request in do_ide_request().

This is broken, do_ide_request() is already called with the request lock
held. tq_disk run -> generic_unplug_device (grab lock) ->
__generic_unplug_device -> do_ide_request(). You just introduced a
deadlock.

This code would have caused hangs or massive corruption immediately if
ide_lock wasn't ready held there. Not to mention instant spin_unlock
BUG() triggers in queue_command()

-- 
Jens Axboe

Re: [PATCH] 2.5.15 IDE 62

[Tom Rini]

On Mon, May 13, 2002 at 02:17:04PM +0200, Martin Dalecki wrote:

> Mon May 13 12:38:11 CEST 2002 ide-clean-62

Hello.  Since include/linux/ide.h has a 'u8', a 'u16', and a 'u64' can
you apply the following so that it doesn't rely in <asm/types.h> being
included indirectly?

-- 
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

===== include/linux/ide.h 1.60 vs edited =====
--- 1.60/include/linux/ide.h	Thu May  9 11:43:58 2002
+++ edited/include/linux/ide.h	Mon May 13 08:34:32 2002
@@ -17,6 +17,7 @@
 #include <linux/bitops.h>
 #include <asm/byteorder.h>
 #include <asm/hdreg.h>
+#include <asm/types.h>
 
 /*
  * This is the multiple IDE interface driver, as evolved from hd.c.

Re: [PATCH] 2.5.15 IDE 62

[Jens Axboe]

On Mon, May 13 2002, Martin Dalecki wrote:
> Uz.ytkownik Jens Axboe napisa?:
> >On Mon, May 13 2002, Martin Dalecki wrote:
> >
> >>Mon May 13 12:38:11 CEST 2002 ide-clean-62
> >>
> >>- Add missing locking around ide_do_request in do_ide_request().
> >
> >
> >This is broken, do_ide_request() is already called with the request lock
> >held. tq_disk run -> generic_unplug_device (grab lock) ->
> >__generic_unplug_device -> do_ide_request(). You just introduced a
> >deadlock.
> >
> >This code would have caused hangs or massive corruption immediately if
> >ide_lock wasn't ready held there. Not to mention instant spin_unlock
> >BUG() triggers in queue_command()
> >
> 
> Oops. Indeed I see now that the ide_lock is exported to
> the upper layers above it in ide-probe.c
> 
> blk_init_queue(q, do_ide_request, &ide_lock);
> 
> But this is problematic in itself, since it means that
> we are basically serialiazing between *all* requests
> on all channels.

Correct.

> So I think we should have per channel locks on this level
> right? This is anyway our unit for serialization.
> (I'm just surprised that blk_init_queue() doesn't
> provide queue specific locking and relies on exported
> locks from the drivers...)

Sure go ahead and fine grain it, I had no time to go that much into
detail when ripping out io_request_lock. A drive->lock passed to
blk_init_queue would do nicely.

But beware that ide locking is a lot nastier than you think. I saw other
irq changes earlier, I just want to make sure that you are _absolutely_
certain that these changes are safe??

-- 
Jens Axboe

Re: [PATCH] 2.5.15 IDE 62

[Martin Dalecki]

Uz.ytkownik Jens Axboe napisa?:
> On Mon, May 13 2002, Martin Dalecki wrote:
> 
>>Uz.ytkownik Jens Axboe napisa?:
>>
>>>On Mon, May 13 2002, Martin Dalecki wrote:
>>>
>>>
>>>>Mon May 13 12:38:11 CEST 2002 ide-clean-62
>>>>
>>>>- Add missing locking around ide_do_request in do_ide_request().
>>>
>>>
>>>This is broken, do_ide_request() is already called with the request lock
>>>held. tq_disk run -> generic_unplug_device (grab lock) ->
>>>__generic_unplug_device -> do_ide_request(). You just introduced a
>>>deadlock.
>>>
>>>This code would have caused hangs or massive corruption immediately if
>>>ide_lock wasn't ready held there. Not to mention instant spin_unlock
>>>BUG() triggers in queue_command()
>>>
>>
>>Oops. Indeed I see now that the ide_lock is exported to
>>the upper layers above it in ide-probe.c
>>
>>blk_init_queue(q, do_ide_request, &ide_lock);
>>
>>But this is problematic in itself, since it means that
>>we are basically serialiazing between *all* requests
>>on all channels.
> 
> 
> Correct.
> 
> 
>>So I think we should have per channel locks on this level
>>right? This is anyway our unit for serialization.
>>(I'm just surprised that blk_init_queue() doesn't
>>provide queue specific locking and relies on exported
>>locks from the drivers...)
> 
> 
> Sure go ahead and fine grain it, I had no time to go that much into
> detail when ripping out io_request_lock. A drive->lock passed to
> blk_init_queue would do nicely.
> 
> But beware that ide locking is a lot nastier than you think. I saw other
> irq changes earlier, I just want to make sure that you are _absolutely_
> certain that these changes are safe??

Well on the channel level they are safe modulo cmd640 and rz1000.
We can handle them by serializing them on the global lock
in do_ide_request. Like:

if (ch->drive[0].serialized|| ch->drive[1].serialized)
    then
    spin_lock(serialize_lock);

The other case are shared PCI irq's between two channel,
but this case I can easly test on my HPT772 controller card.

You could have observed the hwgroup_t melting down... step by step.

Re: [PATCH] 2.5.15 IDE 62

[Martin Dalecki]

> But beware that ide locking is a lot nastier than you think. I saw other
> irq changes earlier, I just want to make sure that you are _absolutely_
> certain that these changes are safe??

Well, I'm at least warned now to double check with the current
BIO stuff...

Re: [PATCH] 2.5.15 IDE 62

[Martin Dalecki]

Uz.ytkownik benh@kernel.crashing.org napisa?:
>>>So I think we should have per channel locks on this level
>>>right? This is anyway our unit for serialization.
>>>(I'm just surprised that blk_init_queue() doesn't
>>>provide queue specific locking and relies on exported
>>>locks from the drivers...)
>>
>>Sure go ahead and fine grain it, I had no time to go that much into
>>detail when ripping out io_request_lock. A drive->lock passed to
>>blk_init_queue would do nicely.
>>
>>But beware that ide locking is a lot nastier than you think. I saw other
>>irq changes earlier, I just want to make sure that you are _absolutely_
>>certain that these changes are safe??
> 
> 
> You'll probably need a per-host lock (but that one can be safely
> hidden in the host controller driver I beleive) since some hosts
> share some registers for their 2 channels (timings can be bitfields
> in a single register controlling 2 channels, I'm not too sure about
> legacy DMA).

Just to clarify it... From the host view it's not the chipset
it's a channel we have to deal with. And there are typically two
channels on a host. For the serialized parts, we have to
possiblities:

1. Preserve the current behaviour of using additionally a global
lock.

2. "Cheat" and reuse the lock from the primary channel during
the initialization of the secondary channel.

Hmmm.... Thinking a bit about it I'm now conviced that 2. is more
elegant then 1. And finally this will
just allow us to make the hwgroup_t go entierly away.


The only thing that worries me are the checks for hwgroupt_t's
only remaining member -> handler use to determine whatever some
IRQ is pending or not.

Re: [PATCH] 2.5.15 IDE 62

[Neil Conway]

On Mon, May 13 2002, Martin Dalecki wrote:
>Oops. Indeed I see now that the ide_lock is exported to
>the upper layers above it in ide-probe.c
>
>blk_init_queue(q, do_ide_request, &ide_lock);
>
>But this is problematic in itself, since it means that
>we are basically serialiazing between *all* requests
>on all channels.

Surely not.  If you look at the line above the one you quoted above, you
see the per-channel serialization being requested:

q->queuedata = drive->channel;

cheers,
Neil
PS: 2.4 doesn't even have the spinlock as a parameter to
blk_init_queue().

Re: [PATCH] 2.5.15 IDE 62

[Martin Dalecki]

Uz.ytkownik Jens Axboe napisa?:
> On Mon, May 13 2002, Linus Torvalds wrote:
> 
>>>Well on the channel level they are safe modulo cmd640 and rz1000.
>>>We can handle them by serializing them on the global lock
>>>in do_ide_request. Like:
>>>
>>>if (ch->drive[0].serialized|| ch->drive[1].serialized)
>>>    then
>>>    spin_lock(serialize_lock);
>>
>>NO.
>>
>>The whole point of having a per-queue lock pointer is that this should be
>>initialized at queue creation time. Don't add more crud to the IDE
>>locking, we want to get _rid_ of the locking that IDE has thought
>>(traditionally incorrectly) that it could do better than the higher
>>levels.
>>
>>So when you create the queue, you should decide at THAT point whether you
>>just want to pass in the same lock or not.
>>
>>For a cmd640, you make sure that both queues get created with the same
>>lock. And for non-broken chipsets, you use per-queue locks.
>>
>>And then you make sure that nobody EVER uses any other lock than the queue
>>lock.
> 
> 
> Completely agreed. And when we finally use the queue as the
> serialization point for "everything", then it all falls into place
> nicely.


Well actually I came to the same conclusion regarding the dealing
with broken chipsets. However please note that:

1. queues are per device, since we have to deal with
the fact that the code flow can be different whatever:

1.1. The drive is doing DMA transfers.

1.2. The drive is doing TCQ. (could and should be unifyed with 1.1.)

1.3. The drive is doing ATAPI.

2. Operations are per channel and not per queue.

Therefore the queue locking and basic serialization
has to be on the channel level, with the "lock recycling trick"
for the two interface chips, which can't distingish properly
between primary and secondary channel.

OK?

Re: [PATCH] 2.5.15 IDE 62

[Linus Torvalds]

[ Martin - just a heads up that I'm not applying 62, so don't make new IDE
  patches relative to that ]

On Mon, 13 May 2002, Martin Dalecki wrote:
>
> Well on the channel level they are safe modulo cmd640 and rz1000.
> We can handle them by serializing them on the global lock
> in do_ide_request. Like:
>
> if (ch->drive[0].serialized|| ch->drive[1].serialized)
>     then
>     spin_lock(serialize_lock);

NO.

The whole point of having a per-queue lock pointer is that this should be
initialized at queue creation time. Don't add more crud to the IDE
locking, we want to get _rid_ of the locking that IDE has thought
(traditionally incorrectly) that it could do better than the higher
levels.

So when you create the queue, you should decide at THAT point whether you
just want to pass in the same lock or not.

For a cmd640, you make sure that both queues get created with the same
lock. And for non-broken chipsets, you use per-queue locks.

And then you make sure that nobody EVER uses any other lock than the queue
lock.

			Linus

Re: [PATCH] 2.5.15 IDE 62

[Jens Axboe]

On Mon, May 13 2002, Linus Torvalds wrote:
> > Well on the channel level they are safe modulo cmd640 and rz1000.
> > We can handle them by serializing them on the global lock
> > in do_ide_request. Like:
> >
> > if (ch->drive[0].serialized|| ch->drive[1].serialized)
> >     then
> >     spin_lock(serialize_lock);
> 
> NO.
> 
> The whole point of having a per-queue lock pointer is that this should be
> initialized at queue creation time. Don't add more crud to the IDE
> locking, we want to get _rid_ of the locking that IDE has thought
> (traditionally incorrectly) that it could do better than the higher
> levels.
> 
> So when you create the queue, you should decide at THAT point whether you
> just want to pass in the same lock or not.
> 
> For a cmd640, you make sure that both queues get created with the same
> lock. And for non-broken chipsets, you use per-queue locks.
> 
> And then you make sure that nobody EVER uses any other lock than the queue
> lock.

Completely agreed. And when we finally use the queue as the
serialization point for "everything", then it all falls into place
nicely.

-- 
Jens Axboe

Re: [PATCH] 2.5.15 IDE 62

[Jens Axboe]

On Mon, May 13 2002, Neil Conway wrote:
> PS: 2.4 doesn't even have the spinlock as a parameter to
> blk_init_queue().

Right, the "queue" (well global in 2.4) lock has always been grabbed
prior to request_fn entry.

-- 
Jens Axboe

Re: [PATCH] 2.5.15 IDE 62

[benh]

>> So I think we should have per channel locks on this level
>> right? This is anyway our unit for serialization.
>> (I'm just surprised that blk_init_queue() doesn't
>> provide queue specific locking and relies on exported
>> locks from the drivers...)
>
>Sure go ahead and fine grain it, I had no time to go that much into
>detail when ripping out io_request_lock. A drive->lock passed to
>blk_init_queue would do nicely.
>
>But beware that ide locking is a lot nastier than you think. I saw other
>irq changes earlier, I just want to make sure that you are _absolutely_
>certain that these changes are safe??

You'll probably need a per-host lock (but that one can be safely
hidden in the host controller driver I beleive) since some hosts
share some registers for their 2 channels (timings can be bitfields
in a single register controlling 2 channels, I'm not too sure about
legacy DMA).

Ben.

Re: [PATCH] 2.5.15 IDE 62

[benh]

>And then you make sure that nobody EVER uses any other lock than the queue
>lock.

Except that some controllers are perfectly safe to use both channels
at the same time, except when dealing with rare and sensible operations
(like changing channel settings) where a common set of registers is
shared between channels.

The controller driver in this case will want a lock per channel queue
and an internal lock to protect access to these shared registers. But
that lock can (and has to be) hidden in the controller driver.

Ben.

Re: [PATCH] 2.5.15 IDE 62

[benh]

>Just to clarify it... From the host view it's not the chipset
>it's a channel we have to deal with. And there are typically two
>channels on a host. For the serialized parts, we have to
>possiblities:
>
>1. Preserve the current behaviour of using additionally a global
>lock.
>
>2. "Cheat" and reuse the lock from the primary channel during
>the initialization of the secondary channel.
>
>Hmmm.... Thinking a bit about it I'm now conviced that 2. is more
>elegant then 1. And finally this will
>just allow us to make the hwgroup_t go entierly away.

I would do things differently. From the common point of view,
what we deal with is

   controller
    /        \
 channel x, channel y, ....

That is an _arbitrary_ number of channels. So the host driver
should just register individual "channels" to the IDE layer, 
each one has it's queue lock, period.

Now, if for any reason, the host specific code has to synchronize
between several of it's channels when dealing with things like
chipset configuration, it's up to that host driver to know about
it and deal with it; which make perfect sense to be done with a
third lock specific to protecting those specific registers that
are shared and that is completely internal to the host chipset
driver.

The only case I see where the host may have to additionally go
and grab the other channel's locks (the queue lock or whatever
you call it) is if the actual setting change on one channel 
has side effect on a currently transferring other channel.

But that is completely internal to the host, and yes, I agree
that reusing the other channel's lock is probably the best solution.

But in cases where you just have 2 bitfields in the same register
that need serialized access from both channels, a simple lock
protecting only that register seems to be plenty enough.

What did I miss ?

Ben.

Re: [PATCH] 2.5.15 IDE 62

[Martin Dalecki]

Uz.ytkownik benh@kernel.crashing.org napisa?:
>>Just to clarify it... From the host view it's not the chipset
>>it's a channel we have to deal with. And there are typically two
>>channels on a host. For the serialized parts, we have to
>>possiblities:
>>
>>1. Preserve the current behaviour of using additionally a global
>>lock.
>>
>>2. "Cheat" and reuse the lock from the primary channel during
>>the initialization of the secondary channel.
>>
>>Hmmm.... Thinking a bit about it I'm now conviced that 2. is more
>>elegant then 1. And finally this will
>>just allow us to make the hwgroup_t go entierly away.
> 
> 
> I would do things differently. From the common point of view,
> what we deal with is
> 
>    controller
>     /        \
>  channel x, channel y, ....
> 
> That is an _arbitrary_ number of channels. So the host driver
> should just register individual "channels" to the IDE layer, 
> each one has it's queue lock, period.
> 
> Now, if for any reason, the host specific code has to synchronize
> between several of it's channels when dealing with things like
> chipset configuration, it's up to that host driver to know about
> it and deal with it; which make perfect sense to be done with a
> third lock specific to protecting those specific registers that
> are shared and that is completely internal to the host chipset
> driver.
> 
> The only case I see where the host may have to additionally go
> and grab the other channel's locks (the queue lock or whatever
> you call it) is if the actual setting change on one channel 
> has side effect on a currently transferring other channel.

The problem is that not all setup register file access
is localized to the particular host chip driver. Go look
for rz1000 - it does not export any correposponding function.
And we have therefore to deal with it on the generic level.

> But that is completely internal to the host, and yes, I agree
> that reusing the other channel's lock is probably the best solution.
> 
> But in cases where you just have 2 bitfields in the same register
> that need serialized access from both channels, a simple lock
> protecting only that register seems to be plenty enough.
> 
> What did I miss ?

See above.

Re: [PATCH] 2.5.15 IDE 62

[Benjamin Herrenschmidt]

>1. The fact that there are some cases where the initialization code
>doesn't necessarily go down to the host chip drivers right now.

Well, I'd rather see the init code be some kind of "lib" called
by the host driver, but well...

>2. Most of the current code...

Sure ;) Note that the real point I missed is about broken controllers
that can't grok dealing with simultaneous requests on both channels
(like cmd640) in which case, you probably need the same request
queue to serialize access to both of them. In practice, that means
more or less dealing with both channels like the same way we do
with slave vs. master.

My understanding though is that the block layer can't (yet ?) quite
deal with a single request queue for several target devices, and
it seems that the whole point of the old "busy" flag along with
andre taskfile stuff was to perform some kind of fair arbitration
between which channels/targets got a chance to process requests.

>BTW> The code will be much cleaner in the upcomming ide 65, since
>the allocation of the structures shared between two channels will be
>simple pushed down to the corresponding host chip drivers instead of
>the "match search" done after the channles have been initialized.

Great ! We are slowly going toward a real host controller driver
template finally ;)

>Since most of the host chip drivers are not reentrant anyway we will
>be able to save quite a lot of allocation code as well.

Re: [PATCH] 2.5.15 IDE 62

[Martin Dalecki]

Uz.ytkownik benh@kernel.crashing.org napisa?:
>>Just to clarify it... From the host view it's not the chipset
>>it's a channel we have to deal with. And there are typically two
>>channels on a host. For the serialized parts, we have to
>>possiblities:
>>
>>1. Preserve the current behaviour of using additionally a global
>>lock.
>>
>>2. "Cheat" and reuse the lock from the primary channel during
>>the initialization of the secondary channel.
>>
>>Hmmm.... Thinking a bit about it I'm now conviced that 2. is more
>>elegant then 1. And finally this will
>>just allow us to make the hwgroup_t go entierly away.
> 
> 
> I would do things differently. From the common point of view,
> what we deal with is
> 
>    controller
>     /        \
>  channel x, channel y, ....
> 
> That is an _arbitrary_ number of channels. So the host driver
> should just register individual "channels" to the IDE layer, 
> each one has it's queue lock, period.
> 
> Now, if for any reason, the host specific code has to synchronize
> between several of it's channels when dealing with things like
> chipset configuration, it's up to that host driver to know about
> it and deal with it; which make perfect sense to be done with a
> third lock specific to protecting those specific registers that
> are shared and that is completely internal to the host chipset
> driver.
> 
> The only case I see where the host may have to additionally go
> and grab the other channel's locks (the queue lock or whatever
> you call it) is if the actual setting change on one channel 
> has side effect on a currently transferring other channel.
> 
> But that is completely internal to the host, and yes, I agree
> that reusing the other channel's lock is probably the best solution.
> 
> But in cases where you just have 2 bitfields in the same register
> that need serialized access from both channels, a simple lock
> protecting only that register seems to be plenty enough.
> 
> What did I miss ?

1. The fact that there are some cases where the initialization code
doesn't necessarily go down to the host chip drivers right now.

2. Most of the current code...

BTW> The code will be much cleaner in the upcomming ide 65, since
the allocation of the structures shared between two channels will be
simple pushed down to the corresponding host chip drivers instead of
the "match search" done after the channles have been initialized.

Since most of the host chip drivers are not reentrant anyway we will
be able to save quite a lot of allocation code as well.