[PATCH] fix issues with loading PCI ide drivers as modules (linux 2.6.0)

[PATCH] fix issues with loading PCI ide drivers as modules (linux 2.6.0)

[Davin McCall]

Hi,

When loading the piix.ko module (generic IDE support is compiled in) I get error messages- the ports are already in use, the block devices are already registered.

The problems seem to be:

1) the hwif structures aren't getting marked as being used if the generic IDE layer is controlling them (->chipset is left as "ide_unknown" instead of "ide_generic")

2) if the pci module is granted control of an already initialized hwif, the drive probing etc. (including I/O port allocation) is re-run. When it fails, the drives are marked as not-present which doesn't appear to cause any problems but seems dangerous.

Patch below fixes this and allows a chipset-specific module to take over the primary IDE interface correctly. Comments welcome.

Davin



diff -urN linux-2.6.0/drivers/ide/ide-probe.c linux-2.6.0-mine/drivers/ide/ide-probe.c
--- linux-2.6.0/drivers/ide/ide-probe.c	Thu Nov 27 07:44:24 2003
+++ linux-2.6.0-mine/drivers/ide/ide-probe.c	Sat Jan  3 13:08:22 2004
@@ -864,6 +864,12 @@
 int hwif_init (ide_hwif_t *hwif);
 int probe_hwif_init (ide_hwif_t *hwif)
 {
+	if( hwif->chipset == ide_pci_takeover )
+	{
+		hwif->chipset = ide_pci;
+		return 0;
+	}
+	
 	hwif->initializing = 1;
 	probe_hwif(hwif);
 	hwif_init(hwif);
@@ -1343,6 +1349,8 @@
 			int unit;
 			if (!hwif->present)
 				continue;
+			
+			if( hwif->chipset == ide_unknown ) hwif->chipset = ide_generic;
 			for (unit = 0; unit < MAX_DRIVES; ++unit)
 				if (hwif->drives[unit].present)
 					ata_attach(&hwif->drives[unit]);
diff -urN linux-2.6.0/drivers/ide/ide.c linux-2.6.0-mine/drivers/ide/ide.c
--- linux-2.6.0/drivers/ide/ide.c	Thu Nov 27 07:43:29 2003
+++ linux-2.6.0-mine/drivers/ide/ide.c	Sat Jan  3 11:34:11 2004
@@ -1915,7 +1915,7 @@
 	const char max_hwif  = '0' + (MAX_HWIFS - 1);
 
 	
-	if (strncmp(s,"hd",2) == 0 && s[2] == '=')	/* hd= is for hd.c   */
+	if (strncmp(s,"hd=",3) == 0 )	/* hd= is for hd.c   */
 		return 0;				/* driver and not us */
 
 	if (strncmp(s,"ide",3) &&
diff -urN linux-2.6.0/drivers/ide/setup-pci.c linux-2.6.0-mine/drivers/ide/setup-pci.c
--- linux-2.6.0/drivers/ide/setup-pci.c	Thu Nov 27 07:43:39 2003
+++ linux-2.6.0-mine/drivers/ide/setup-pci.c	Sat Jan  3 12:52:41 2004
@@ -449,7 +449,17 @@
 	} else if (hwif->io_ports[IDE_CONTROL_OFFSET] != (ctl | 2)) {
 		goto fixup_address;
 	}
-	hwif->chipset = ide_pci;
+
+	/*
+	 * if the chipset of the returned hwif is currently ide_generic, the PCI
+	 * ide driver can take over control of the hwif. We use the special
+	 * ide_pci_takeover value to prevent re-probing the drives etc in
+	 * probe_hwif_init.
+	 */
+	 
+	if( hwif->chipset == ide_generic ) hwif->chipset = ide_pci_takeover;
+	else hwif->chipset = ide_pci;
+
 	hwif->pci_dev = dev;
 	hwif->cds = (struct ide_pci_device_s *) d;
 	hwif->channel = port;
diff -urN linux-2.6.0/include/linux/ide.h linux-2.6.0-mine/include/linux/ide.h
--- linux-2.6.0/include/linux/ide.h	Thu Nov 27 07:43:36 2003
+++ linux-2.6.0-mine/include/linux/ide.h	Sat Jan  3 04:55:42 2004
@@ -275,6 +275,10 @@
 /*
  * hwif_chipset_t is used to keep track of the specific hardware
  * chipset used by each IDE interface, if known.
+ *
+ * ide_pci_takeover is a temporary state used when a chipset-specific
+ * driver is loaded as a module and has to take control of a controller
+ * previously being supervised by the generic pci driver.
  */
 typedef enum {	ide_unknown,	ide_generic,	ide_pci,
 		ide_cmd640,	ide_dtc2278,	ide_ali14xx,
@@ -282,7 +286,7 @@
 		ide_pdc4030,	ide_rz1000,	ide_trm290,
 		ide_cmd646,	ide_cy82c693,	ide_4drives,
 		ide_pmac,	ide_etrax100,	ide_acorn,
-		ide_pc9800
+		ide_pc9800, ide_pci_takeover
 } hwif_chipset_t;
 
 /*

Re: [PATCH] fix issues with loading PCI ide drivers as modules (linux 2.6.0)

[Bartlomiej Zolnierkiewicz]

On Saturday 03 of January 2004 05:28, Davin McCall wrote:
> Hi,

Hi,

> When loading the piix.ko module (generic IDE support is compiled in) I get
> error messages- the ports are already in use, the block devices are already
> registered.
>
> The problems seem to be:
>
> 1) the hwif structures aren't getting marked as being used if the generic
> IDE layer is controlling them (->chipset is left as "ide_unknown" instead
> of "ide_generic")

Are you aware that your change brakes "idex=base", "idex=base,ctl"
and "idex=base,ctl,irq" kernel parameters?  If these parameters are used
hwif->chipset is also set to ide_generic.  Now if controller is a PCI one
and PCI IDE support is compiled in hwif->chipset will be set to
ide_pci_takeover and drives won't be probed.

> 2) if the pci module is granted control of an already initialized hwif, the
> drive probing etc. (including I/O port allocation) is re-run. When it
> fails, the drives are marked as not-present which doesn't appear to cause
> any problems but seems dangerous.

When it fails controller+drives are not being programmed correctly
(because probe_hwif() returns early).  "takeover" is not supported because
you need to reprogram controller/drive to do DMA, but probing code
(which does also reprogramming) can race with actual data transfer.

> Patch below fixes this and allows a chipset-specific module to take over
> the primary IDE interface correctly. Comments welcome.

I think proper fix is to add IDE generic host driver and make it modular.

--bart

Re: [PATCH] fix issues with loading PCI ide drivers as modules (linux 2.6.0)

[Davin McCall]

On Sun, 4 Jan 2004 02:56:57 +0100
Bartlomiej Zolnierkiewicz <B.Zolnierkiewicz@elka.pw.edu.pl> wrote:

> Are you aware that your change brakes "idex=base", "idex=base,ctl"
> and "idex=base,ctl,irq" kernel parameters?  If these parameters are used
> hwif->chipset is also set to ide_generic.  Now if controller is a PCI one
> and PCI IDE support is compiled in hwif->chipset will be set to
> ide_pci_takeover and drives won't be probed.

Ok. I see the problem.

> When it fails controller+drives are not being programmed correctly
> (because probe_hwif() returns early).  "takeover" is not supported because
> you need to reprogram controller/drive to do DMA, but probing code
> (which does also reprogramming) can race with actual data transfer.

... So basically, it's not nearly as simple as I had hoped.

However, there are still two genuine but easily solveable problems that I can see:

1) unless "idex=base,ctl,irq" is used, the hwif->chipset is left as "ide_unknown"
   (this means for that the hwif can get re-allocated in setup-pci.c - ide_match_hwif() -
    and clobbered)
2) if "idex=base,ctl,irq" IS used, the hwif structure will still get clobbered when a PCI
   chipset module is loaded.

What about this is a solution to these problems:
 - set hwif->chipset to "ide_generic" instead of leaving it as "ide_unknown" (ide-probe.c);
 - if ide_match_hwif() returns an already allocated hwif, do not clobber it in ide_hwif_configure() (setup-pci.c)

Two individual patches below; again any comments appreciated!

Davin



--- ide-probe.c.orig	Sun Jan  4 14:17:22 2004
+++ ide-probe.c	Sun Jan  4 13:58:44 2004
@@ -1343,6 +1343,7 @@
 			int unit;
 			if (!hwif->present)
 				continue;
+			if (hwif->chipset == ide_unknown) hwif_chipset = ide_generic;
 			for (unit = 0; unit < MAX_DRIVES; ++unit)
 				if (hwif->drives[unit].present)
 					ata_attach(&hwif->drives[unit]);


--- setup-pci.c.orig	Sun Jan  4 14:17:30 2004
+++ setup-pci.c	Sun Jan  4 14:12:23 2004
@@ -441,6 +441,9 @@
 	}
 	if ((hwif = ide_match_hwif(base, d->bootable, d->name)) == NULL)
 		return NULL;	/* no room in ide_hwifs[] */
+	if (hwif->chipset != ide_unknown)
+		return NULL;  /* clash with already allocated hwif */
+
 	if (hwif->io_ports[IDE_DATA_OFFSET] != base) {
 fixup_address:
 		ide_init_hwif_ports(&hwif->hw, base, (ctl | 2), NULL);

Re: [PATCH] fix issues with loading PCI ide drivers as modules (linux 2.6.0)

[Bartlomiej Zolnierkiewicz]

On Sunday 04 of January 2004 04:21, Davin McCall wrote:
> However, there are still two genuine but easily solveable problems that I
> can see:
>
> 1) unless "idex=base,ctl,irq" is used, the hwif->chipset is left as
> "ide_unknown" (this means for that the hwif can get re-allocated in
> setup-pci.c - ide_match_hwif() - and clobbered)

Hmm.  What if hwif is freed by a driver?

> 2) if "idex=base,ctl,irq" IS used, the hwif structure will still get
> clobbered when a PCI chipset module is loaded.
>
> What about this is a solution to these problems:
>  - set hwif->chipset to "ide_generic" instead of leaving it as
> "ide_unknown" (ide-probe.c); - if ide_match_hwif() returns an already
> allocated hwif, do not clobber it in ide_hwif_configure() (setup-pci.c)

This brakes "idex=base..." parameters for PCI chipsets.
They shouldn't be needed in this case, but...

--bart

Re: [PATCH] fix issues with loading PCI ide drivers as modules (linux 2.6.0)

[Davin McCall]

Sorry, I'm resending this as I forgot to CC: it to the lists.

On Sun, 4 Jan 2004 04:52:17 +0100
Bartlomiej Zolnierkiewicz <B.Zolnierkiewicz@elka.pw.edu.pl> wrote:

> > 1) unless "idex=base,ctl,irq" is used, the hwif->chipset is left as
> > "ide_unknown" (this means for that the hwif can get re-allocated in
> > setup-pci.c - ide_match_hwif() - and clobbered)
> 
> Hmm.  What if hwif is freed by a driver?

I don't think I'm really sure what you're asking. (which driver frees hwif? why is it a problem? I see a "ide_unregister" call, it resets the hwif to default state - this should be fine.

> > What about this is a solution to these problems:
> >  - set hwif->chipset to "ide_generic" instead of leaving it as
> > "ide_unknown" (ide-probe.c); - if ide_match_hwif() returns an already
> > allocated hwif, do not clobber it in ide_hwif_configure() (setup-pci.c)
>
> This brakes "idex=base..." parameters for PCI chipsets.
> They shouldn't be needed in this case, but...

As far as i can see "idex=base.." is broken for PCI chipsets anyway- if the detected PCI base doesn't match the forced one, the PCI will be allocated a seperate hwif (ie as a seperate ideX) anyway. So you can't force the base port of a PCI-chipset controller.

Do you mean that, if "idex=base..." is give, and the base is correct for the PCI device, then it should work ok? If so it seems the easiest way to fix it is to introduce another dummy chipset type (lets say "ide_generic_forced") which is set (instead of ide_generic) when "idex=.." is parsed. Then check for this in ide_hwif_configure(). Would also need to modify ide_match_hwif() (so it returns a match for "ide_generic_forced" as well as for "ide_generic") and ide_probe_init() would have to change "ide_generic_force" to "ide_generic" (to handle the case that no PCI chipset took control).

So we handle these situations:
- idex=... specified and no PCI chipset
- idex=... specified and PCI chipset present
- PCI chipset module loaded after ide initialization complete

Does that sound ok? If so I will write another patch.

Davin

Re: [PATCH] fix issues with loading PCI ide drivers as modules (linux 2.6.0)

[Bartlomiej Zolnierkiewicz]

On Sunday 04 of January 2004 07:31, Davin McCall wrote:
> > > What about this is a solution to these problems:
> > >  - set hwif->chipset to "ide_generic" instead of leaving it as
> > > "ide_unknown" (ide-probe.c); - if ide_match_hwif() returns an already
> > > allocated hwif, do not clobber it in ide_hwif_configure() (setup-pci.c)
> >
> > This brakes "idex=base..." parameters for PCI chipsets.
> > They shouldn't be needed in this case, but...
>
> As far as i can see "idex=base.." is broken for PCI chipsets anyway- if the
> detected PCI base doesn't match the forced one, the PCI will be allocated a
> seperate hwif (ie as a seperate ideX) anyway. So you can't force the base
> port of a PCI-chipset controller.

Yes, but you can force order.

> Do you mean that, if "idex=base..." is give, and the base is correct for
> the PCI device, then it should work ok? If so it seems the easiest way to

Yes.

> fix it is to introduce another dummy chipset type (lets say
> "ide_generic_forced") which is set (instead of ide_generic) when "idex=.."
> is parsed. Then check for this in ide_hwif_configure(). Would also need to
> modify ide_match_hwif() (so it returns a match for "ide_generic_forced" as
> well as for "ide_generic") and ide_probe_init() would have to change
> "ide_generic_force" to "ide_generic" (to handle the case that no PCI
> chipset took control).

Ehh, more hwif->chipset crap.

> So we handle these situations:
> - idex=... specified and no PCI chipset
> - idex=... specified and PCI chipset present
> - PCI chipset module loaded after ide initialization complete

Please explain me why to change current behavior.  "takeover" still want be
possible, so it will be only cosmetic change which complicates code.

--bart

Re: [PATCH] fix issues with loading PCI ide drivers as modules (linux 2.6.0)

[Davin McCall]

On Sun, 4 Jan 2004 15:47:52 +0100
Bartlomiej Zolnierkiewicz <B.Zolnierkiewicz@elka.pw.edu.pl> wrote:

> Please explain me why to change current behavior.  "takeover" still want be
> possible, so it will be only cosmetic change which complicates code.
> 
> --bart
> 

Alright - I have a PIIX4 chipset in my system. when I compile the PIIX4 ide driver as a module (ie. "piix.ko") and load it after booting the system I get kernel messages because
1) it can't allocate the I/O ports for the ide interfaces, io 0x170-0x177  0x1F0-0x1F7 etc
   because they have been allocated by the non-PCI driver (ide.c)
2) it can't register block devices; they've already been allocated

Looking at the code it is clear that when I get these messages, the hwif will then be marked "not present" (ie. hwif->present = 0) and the drives (hda, hdb and hdc in my case) will also be marked not present.

The "hwif" structures corresponding to ide0/ide1 have been trashed - even though the piix module shouldn't have been granted control of the interfaces. The reason is that "ide_hwif_configure()" (setup-pci.c) calls "ide_match_hwif" which returns whatever hwif structure matches the io-base address (which the chipset is "ide_unknown" or "ide_generic"), and then, ide_hwif_configure() USES that hwif and for instance sets its chipset to "ide_pci" and returns it to the caller (ide_pci_setup_ports) which continues to trash it, tries to allocate the base and ctl ports (which fails) and register the block devices (which fails).

You can verify that the hwif has been modified by "cat /proc/ide/ide0/model" before and after inserting the piix module - before, i get "unknown", afterwards i get "pci".

Sure, the current code doesn't cause a crash - but it's very, very ugly. It generates some confusing error messages, and it makes it look like the module has taken control of the IDE interfaces but really the drives haven't been re-probed etc.

Is this not worth fixing?

> 
> Ehh, more hwif->chipset crap.
> 

Alright, this newer patch below mostly avoids the "hwif->chipset crap" (it doesn't introduce any new chipset types). But it has to export the "initializing" variable from ide.c (I changed its name to "ide_initializing").

Plus, everything works as before - including "idex=..." parameters.

Davin.




diff -urN linux-2.6.0-vanilla/drivers/ide/ide-probe.c linux-2.6.0/drivers/ide/ide-probe.c
--- linux-2.6.0-vanilla/drivers/ide/ide-probe.c	Thu Nov 27 07:44:24 2003
+++ linux-2.6.0/drivers/ide/ide-probe.c	Sun Jan  4 14:46:04 2004
@@ -1343,6 +1343,7 @@
 			int unit;
 			if (!hwif->present)
 				continue;
+			if (hwif->chipset == ide_unknown) hwif->chipset = ide_generic;
 			for (unit = 0; unit < MAX_DRIVES; ++unit)
 				if (hwif->drives[unit].present)
 					ata_attach(&hwif->drives[unit]);
diff -urN linux-2.6.0-vanilla/drivers/ide/ide.c linux-2.6.0/drivers/ide/ide.c
--- linux-2.6.0-vanilla/drivers/ide/ide.c	Thu Nov 27 07:43:29 2003
+++ linux-2.6.0/drivers/ide/ide.c	Sun Jan  4 19:36:42 2004
@@ -173,7 +173,9 @@
 
 static int idebus_parameter;	/* holds the "idebus=" parameter */
 static int system_bus_speed;	/* holds what we think is VESA/PCI bus speed */
-static int initializing;	/* set while initializing built-in drivers */
+
+int ide_initializing;		/* set while initializing built-in drivers */
+EXPORT_SYMBOL(ide_initializing);
 
 DECLARE_MUTEX(ide_cfg_sem);
 spinlock_t ide_lock __cacheline_aligned_in_smp = SPIN_LOCK_UNLOCKED;
@@ -1011,8 +1013,8 @@
 			hwif = &ide_hwifs[index];
 			if (hwif->hold)
 				continue;
-			if ((!hwif->present && !hwif->mate && !initializing) ||
-			    (!hwif->hw.io_ports[IDE_DATA_OFFSET] && initializing))
+			if ((!hwif->present && !hwif->mate && !ide_initializing) ||
+			    (!hwif->hw.io_ports[IDE_DATA_OFFSET] && ide_initializing))
 				goto found;
 		}
 		for (index = 0; index < MAX_HWIFS; index++)
@@ -1032,7 +1034,7 @@
 	hwif->noprobe = 0;
 	hwif->chipset = hw->chipset;
 
-	if (!initializing) {
+	if (!ide_initializing) {
 		ide_probe_module();
 #ifdef CONFIG_PROC_FS
 		create_proc_ide_interfaces();
@@ -1042,7 +1044,7 @@
 	if (hwifp)
 		*hwifp = hwif;
 
-	return (initializing || hwif->present) ? index : -1;
+	return (ide_initializing || hwif->present) ? index : -1;
 }
 
 EXPORT_SYMBOL(ide_register_hw);
@@ -2606,9 +2608,9 @@
 		(void)qd65xx_init();
 #endif
 
-	initializing = 1;
+	ide_initializing = 1;
 	ide_init_builtin_drivers();
-	initializing = 0;
+	ide_initializing = 0;
 
 	return 0;
 }
diff -urN linux-2.6.0-vanilla/drivers/ide/setup-pci.c linux-2.6.0/drivers/ide/setup-pci.c
--- linux-2.6.0-vanilla/drivers/ide/setup-pci.c	Thu Nov 27 07:43:39 2003
+++ linux-2.6.0/drivers/ide/setup-pci.c	Sun Jan  4 19:41:09 2004
@@ -441,6 +441,9 @@
 	}
 	if ((hwif = ide_match_hwif(base, d->bootable, d->name)) == NULL)
 		return NULL;	/* no room in ide_hwifs[] */
+	if (hwif->chipset == ide_generic && ! ide_initializing )
+		return NULL;  /* clash with already allocated hwif */
+
 	if (hwif->io_ports[IDE_DATA_OFFSET] != base) {
 fixup_address:
 		ide_init_hwif_ports(&hwif->hw, base, (ctl | 2), NULL);
diff -urN linux-2.6.0-vanilla/include/linux/ide.h linux-2.6.0/include/linux/ide.h
--- linux-2.6.0-vanilla/include/linux/ide.h	Thu Nov 27 07:43:36 2003
+++ linux-2.6.0/include/linux/ide.h	Sun Jan  4 19:40:28 2004
@@ -1246,6 +1246,8 @@
 extern ide_devices_t   *idetape;
 extern ide_devices_t   *idescsi;
 
+extern int ide_initializing;
+
 #endif
 extern int noautodma;
 

Re: [PATCH] fix issues with loading PCI ide drivers as modules (linux 2.6.0)

[Bartlomiej Zolnierkiewicz]

On Monday 05 of January 2004 03:09, Davin McCall wrote:
> Sure, the current code doesn't cause a crash - but it's very, very ugly. It
> generates some confusing error messages, and it makes it look like the
> module has taken control of the IDE interfaces but really the drives
> haven't been re-probed etc.
>
> Is this not worth fixing?

You are right.  Thanks for very good explanation.

> > Ehh, more hwif->chipset crap.
>
> Alright, this newer patch below mostly avoids the "hwif->chipset crap" (it
> doesn't introduce any new chipset types). But it has to export the
> "initializing" variable from ide.c (I changed its name to
> "ide_initializing").

You don't need to export "initializing" variable from ide.c,
just use "pre_init" variable from setup-pci.c :-).

> Plus, everything works as before - including "idex=..." parameters.

Except when using them for IDE PCI modules with non default ports:
- hwif->chipset is set to ide_generic during boot
- main IDE driver initialization
- module load fails (because hwif->chipset == ide_generic && !initializing)

You can fix it by replacing all current occurrences of ide_generic by some
new type (ide_forced).  It will also clear confusion about ide_generic name.

> @@ -1343,6 +1343,7 @@
>  			int unit;
>  			if (!hwif->present)
>  				continue;
> +			if (hwif->chipset == ide_unknown) hwif->chipset = ide_generic;

very minor nitpick:

if (hwif->chipset == ide_unknown)
	hwif->chipset = ide_generic;

Please correct patch and I will merge it.

cheers,
--bart

Re: [PATCH] fix issues with loading PCI ide drivers as modules (linux 2.6.0)

[Davin McCall]

Ok - fourth try! Hopefully I'm getting better.

On Mon, 5 Jan 2004 15:16:03 +0100
Bartlomiej Zolnierkiewicz <B.Zolnierkiewicz@elka.pw.edu.pl> wrote:

> You don't need to export "initializing" variable from ide.c,
> just use "pre_init" variable from setup-pci.c :-).
> 

Yes - of course.

But actually I have found an even better solution - "hwif->present" tells us if the hwif is currently being controlled by some chipset driver (that's what it's there for!) so I check that instead.

> > Plus, everything works as before - including "idex=..." parameters.
> 
> Except when using them for IDE PCI modules with non default ports:
> - hwif->chipset is set to ide_generic during boot
> - main IDE driver initialization
> - module load fails (because hwif->chipset == ide_generic && !initializing)
> 
> You can fix it by replacing all current occurrences of ide_generic by some
> new type (ide_forced).  It will also clear confusion about ide_generic name.

I've changed all occurrences of "ide_generic" to "ide_forced" and removed "ide_generic" altogether.

Now, "ide_forced" means what it should. The chipset is changed to "ide_unknown" or "ide_pci" when some chipset driver takes over (be it the standard ide, or some PCI specific). So no hwif which is actually present will be left as "ide_forced".

Also I have slightly modified match_hwif to check hwif->present when it is trying to find a free entry (otherwise, there is a chance that it will return one that cannot be used, even when there is a truly free one available).

This doesn't *exactly* match what you said, but it seems to me to be very clean and solve all the issues at once. If you can see any problems let me know and I will revert it to the previous way (with your improvements incorporated).

regards
Davin


diff -urN linux-2.6.0-vanilla/drivers/ide/ide-probe.c linux-2.6.0/drivers/ide/ide-probe.c
--- linux-2.6.0-vanilla/drivers/ide/ide-probe.c	Thu Nov 27 07:44:24 2003
+++ linux-2.6.0/drivers/ide/ide-probe.c	Tue Jan  6 12:54:50 2004
@@ -1343,6 +1343,8 @@
 			int unit;
 			if (!hwif->present)
 				continue;
+			if (hwif->chipset == ide_forced)
+				hwif->chipset = ide_unknown;
 			for (unit = 0; unit < MAX_DRIVES; ++unit)
 				if (hwif->drives[unit].present)
 					ata_attach(&hwif->drives[unit]);
diff -urN linux-2.6.0-vanilla/drivers/ide/ide-proc.c linux-2.6.0/drivers/ide/ide-proc.c
--- linux-2.6.0-vanilla/drivers/ide/ide-proc.c	Thu Nov 27 07:44:17 2003
+++ linux-2.6.0/drivers/ide/ide-proc.c	Tue Jan  6 12:40:58 2004
@@ -348,9 +348,13 @@
 	int		len;
 	const char	*name;
 
+	/*
+	 * Note: Don't need to handle "ide_forced" as that should never be set at
+	 * this stage.
+	 */
+	
 	switch (hwif->chipset) {
 		case ide_unknown:	name = "(none)";	break;
-		case ide_generic:	name = "generic";	break;
 		case ide_pci:		name = "pci";		break;
 		case ide_cmd640:	name = "cmd640";	break;
 		case ide_dtc2278:	name = "dtc2278";	break;
diff -urN linux-2.6.0-vanilla/drivers/ide/ide.c linux-2.6.0/drivers/ide/ide.c
--- linux-2.6.0-vanilla/drivers/ide/ide.c	Thu Nov 27 07:43:29 2003
+++ linux-2.6.0/drivers/ide/ide.c	Tue Jan  6 12:41:19 2004
@@ -2185,7 +2185,7 @@
 				memcpy(hwif->io_ports, hwif->hw.io_ports, sizeof(hwif->io_ports));
 				hwif->irq      = vals[2];
 				hwif->noprobe  = 0;
-				hwif->chipset  = ide_generic;
+				hwif->chipset  = ide_forced;
 				goto done;
 
 			case 0: goto bad_option;
diff -urN linux-2.6.0-vanilla/drivers/ide/pci/cmd640.c linux-2.6.0/drivers/ide/pci/cmd640.c
--- linux-2.6.0-vanilla/drivers/ide/pci/cmd640.c	Thu Nov 27 07:45:36 2003
+++ linux-2.6.0/drivers/ide/pci/cmd640.c	Tue Jan  6 12:42:20 2004
@@ -419,7 +419,7 @@
 	cmd_hwif1 = &ide_hwifs[1]; /* default, if not found below */
 	for (i = 0; i < MAX_HWIFS; i++) {
 		ide_hwif_t *hwif = &ide_hwifs[i];
-		if (hwif->chipset == ide_unknown || hwif->chipset == ide_generic) {
+		if (hwif->chipset == ide_unknown || hwif->chipset == ide_forced) {
 			if (hwif->io_ports[IDE_DATA_OFFSET] == 0x1f0)
 				cmd_hwif0 = hwif;
 			else if (hwif->io_ports[IDE_DATA_OFFSET] == 0x170)
diff -urN linux-2.6.0-vanilla/drivers/ide/setup-pci.c linux-2.6.0/drivers/ide/setup-pci.c
--- linux-2.6.0-vanilla/drivers/ide/setup-pci.c	Thu Nov 27 07:43:39 2003
+++ linux-2.6.0/drivers/ide/setup-pci.c	Tue Jan  6 13:44:45 2004
@@ -59,7 +59,7 @@
 	for (h = 0; h < MAX_HWIFS; ++h) {
 		hwif = &ide_hwifs[h];
 		if (hwif->io_ports[IDE_DATA_OFFSET] == io_base) {
-			if (hwif->chipset == ide_generic)
+			if (hwif->chipset == ide_forced)
 				return hwif; /* a perfect match */
 		}
 	}
@@ -91,19 +91,19 @@
 	if (bootable) {
 		for (h = 0; h < MAX_HWIFS; ++h) {
 			hwif = &ide_hwifs[h];
-			if (hwif->chipset == ide_unknown)
+			if (hwif->chipset == ide_unknown && !hwif->present)
 				return hwif;	/* pick an unused entry */
 		}
 	} else {
 		for (h = 2; h < MAX_HWIFS; ++h) {
 			hwif = ide_hwifs + h;
-			if (hwif->chipset == ide_unknown)
+			if (hwif->chipset == ide_unknown && !hwif->present)
 				return hwif;	/* pick an unused entry */
 		}
 	}
 	for (h = 0; h < 2; ++h) {
 		hwif = ide_hwifs + h;
-		if (hwif->chipset == ide_unknown)
+		if (hwif->chipset == ide_unknown && !hwif->present)
 			return hwif;	/* pick an unused entry */
 	}
 	printk(KERN_ERR "%s: too many IDE interfaces, no room in table\n", name);
@@ -441,6 +441,8 @@
 	}
 	if ((hwif = ide_match_hwif(base, d->bootable, d->name)) == NULL)
 		return NULL;	/* no room in ide_hwifs[] */
+	if (hwif->present)
+		return NULL;
 	if (hwif->io_ports[IDE_DATA_OFFSET] != base) {
 fixup_address:
 		ide_init_hwif_ports(&hwif->hw, base, (ctl | 2), NULL);
diff -urN linux-2.6.0-vanilla/include/linux/ide.h linux-2.6.0/include/linux/ide.h
--- linux-2.6.0-vanilla/include/linux/ide.h	Thu Nov 27 07:43:36 2003
+++ linux-2.6.0/include/linux/ide.h	Tue Jan  6 12:36:46 2004
@@ -276,7 +276,7 @@
  * hwif_chipset_t is used to keep track of the specific hardware
  * chipset used by each IDE interface, if known.
  */
-typedef enum {	ide_unknown,	ide_generic,	ide_pci,
+typedef enum {	ide_unknown,	ide_forced,	ide_pci,
 		ide_cmd640,	ide_dtc2278,	ide_ali14xx,
 		ide_qd65xx,	ide_umc8672,	ide_ht6560b,
 		ide_pdc4030,	ide_rz1000,	ide_trm290,

Re: [PATCH] fix issues with loading PCI ide drivers as modules (linux 2.6.0)

[Bartlomiej Zolnierkiewicz]

On Tuesday 06 of January 2004 03:51, Davin McCall wrote:
> Ok - fourth try! Hopefully I'm getting better.
>
> On Mon, 5 Jan 2004 15:16:03 +0100
>
> Bartlomiej Zolnierkiewicz <B.Zolnierkiewicz@elka.pw.edu.pl> wrote:
> > You don't need to export "initializing" variable from ide.c,
> > just use "pre_init" variable from setup-pci.c :-).
>
> Yes - of course.
>
> But actually I have found an even better solution - "hwif->present" tells
> us if the hwif is currently being controlled by some chipset driver (that's
> what it's there for!) so I check that instead.

This is wrong, driver owns hwif before probing for drives
(when at least one drive is found hwif->present is set to one),
so hwif entries can be modified even with hwif->present equal to zero.

--bart

Re: [PATCH] fix issues with loading PCI ide drivers as modules (linux 2.6.0)

[Davin McCall]

On Tue, 6 Jan 2004 12:13:39 +0100
Bartlomiej Zolnierkiewicz <B.Zolnierkiewicz@elka.pw.edu.pl> wrote:

> > But actually I have found an even better solution - "hwif->present" tells
> > us if the hwif is currently being controlled by some chipset driver (that's
> > what it's there for!) so I check that instead.
> 
> This is wrong, driver owns hwif before probing for drives
> (when at least one drive is found hwif->present is set to one),
> so hwif entries can be modified even with hwif->present equal to zero.

For the purposes of the patch that I provided, it doesn't matter. The hwif will only be used if both !hwif->present and chipset==ide_unknown or ide_forced. If !hwif->present, then there are no drives with active IO and so it is safe for another chipset to take over the interface.

Anyway I have re-done the previous patch the way you wanted, so you can take your pick. Personally I prefer the previous one.

Note with this new patch, I no longer needed to check initialize/pre_init variable in ide_hwif_configure - because ide_match_hwif() will never return with a hwif whose chipset is ide_generic (only ide_forced or ide_unknown).

Davin


diff -urN linux-2.6.0-vanilla/drivers/ide/ide-probe.c linux-2.6.0/drivers/ide/ide-probe.c
--- linux-2.6.0-vanilla/drivers/ide/ide-probe.c	Thu Nov 27 07:44:24 2003
+++ linux-2.6.0/drivers/ide/ide-probe.c	Tue Jan  6 23:13:28 2004
@@ -1343,6 +1343,8 @@
 			int unit;
 			if (!hwif->present)
 				continue;
+			if (hwif->chipset == ide_unknown || hwif->chipset == ide_forced)
+				hwif->chipset = ide_generic;
 			for (unit = 0; unit < MAX_DRIVES; ++unit)
 				if (hwif->drives[unit].present)
 					ata_attach(&hwif->drives[unit]);
diff -urN linux-2.6.0-vanilla/drivers/ide/ide-proc.c linux-2.6.0/drivers/ide/ide-proc.c
--- linux-2.6.0-vanilla/drivers/ide/ide-proc.c	Thu Nov 27 07:44:17 2003
+++ linux-2.6.0/drivers/ide/ide-proc.c	Tue Jan  6 23:07:52 2004
@@ -348,8 +348,11 @@
 	int		len;
 	const char	*name;
 
+	/*
+	 *  Neither ide_unknown nor ide_forced should be set at this point.
+	 */
+	
 	switch (hwif->chipset) {
-		case ide_unknown:	name = "(none)";	break;
 		case ide_generic:	name = "generic";	break;
 		case ide_pci:		name = "pci";		break;
 		case ide_cmd640:	name = "cmd640";	break;
diff -urN linux-2.6.0-vanilla/drivers/ide/ide.c linux-2.6.0/drivers/ide/ide.c
--- linux-2.6.0-vanilla/drivers/ide/ide.c	Thu Nov 27 07:43:29 2003
+++ linux-2.6.0/drivers/ide/ide.c	Tue Jan  6 23:08:17 2004
@@ -2185,7 +2185,7 @@
 				memcpy(hwif->io_ports, hwif->hw.io_ports, sizeof(hwif->io_ports));
 				hwif->irq      = vals[2];
 				hwif->noprobe  = 0;
-				hwif->chipset  = ide_generic;
+				hwif->chipset  = ide_forced;
 				goto done;
 
 			case 0: goto bad_option;
diff -urN linux-2.6.0-vanilla/drivers/ide/pci/cmd640.c linux-2.6.0/drivers/ide/pci/cmd640.c
--- linux-2.6.0-vanilla/drivers/ide/pci/cmd640.c	Thu Nov 27 07:45:36 2003
+++ linux-2.6.0/drivers/ide/pci/cmd640.c	Tue Jan  6 13:07:51 2004
@@ -419,7 +419,7 @@
 	cmd_hwif1 = &ide_hwifs[1]; /* default, if not found below */
 	for (i = 0; i < MAX_HWIFS; i++) {
 		ide_hwif_t *hwif = &ide_hwifs[i];
-		if (hwif->chipset == ide_unknown || hwif->chipset == ide_generic) {
+		if (hwif->chipset == ide_unknown || hwif->chipset == ide_forced) {
 			if (hwif->io_ports[IDE_DATA_OFFSET] == 0x1f0)
 				cmd_hwif0 = hwif;
 			else if (hwif->io_ports[IDE_DATA_OFFSET] == 0x170)
diff -urN linux-2.6.0-vanilla/drivers/ide/setup-pci.c linux-2.6.0/drivers/ide/setup-pci.c
--- linux-2.6.0-vanilla/drivers/ide/setup-pci.c	Thu Nov 27 07:43:39 2003
+++ linux-2.6.0/drivers/ide/setup-pci.c	Tue Jan  6 23:18:50 2004
@@ -59,7 +59,7 @@
 	for (h = 0; h < MAX_HWIFS; ++h) {
 		hwif = &ide_hwifs[h];
 		if (hwif->io_ports[IDE_DATA_OFFSET] == io_base) {
-			if (hwif->chipset == ide_generic)
+			if (hwif->chipset == ide_forced)
 				return hwif; /* a perfect match */
 		}
 	}
diff -urN linux-2.6.0-vanilla/include/linux/ide.h linux-2.6.0/include/linux/ide.h
--- linux-2.6.0-vanilla/include/linux/ide.h	Thu Nov 27 07:43:36 2003
+++ linux-2.6.0/include/linux/ide.h	Tue Jan  6 23:06:23 2004
@@ -282,7 +282,7 @@
 		ide_pdc4030,	ide_rz1000,	ide_trm290,
 		ide_cmd646,	ide_cy82c693,	ide_4drives,
 		ide_pmac,	ide_etrax100,	ide_acorn,
-		ide_pc9800
+		ide_pc9800, ide_forced
 } hwif_chipset_t;
 
 /*

Re: [PATCH] fix issues with loading PCI ide drivers as modules (linux 2.6.0)

[Davin McCall]

Hmm. Actually, If I am permitted to change my mind, I do actually prefer this latest patch...
:-)

Davin

[PATCH] various IDE patches/cleanups

[Davin McCall]

I've been doing some hacking on the IDE layer, just to fix a few issues I noticed going through the code. Due to the complex nature of the code I'm bound to have missed some things and perhaps misunderstood others. Nevertheless I'm posting these patches in the hope that they can be tested on other machines, rejected, or even accepted.

Comments and criticisms are welcome.

The first patch, below, is already included in the -mm tree. The further patches are appearing here for the first time.

All patches are against linux-2.6.0 but apply correctly against 2.6.2-rc2.

Davin

---
First patch: issues loading pci chipset drivers as modules. (don't let the module trash data structures used by current interfaces)


diff -urN linux-2.6.0-vanilla/drivers/ide/ide-probe.c linux-2.6.0/drivers/ide/ide-probe.c
--- linux-2.6.0-vanilla/drivers/ide/ide-probe.c	Thu Nov 27 07:44:24 2003
+++ linux-2.6.0/drivers/ide/ide-probe.c	Tue Jan  6 23:13:28 2004
@@ -1343,6 +1343,8 @@
 			int unit;
 			if (!hwif->present)
 				continue;
+			if (hwif->chipset == ide_unknown || hwif->chipset == ide_forced)
+				hwif->chipset = ide_generic;
 			for (unit = 0; unit < MAX_DRIVES; ++unit)
 				if (hwif->drives[unit].present)
 					ata_attach(&hwif->drives[unit]);
diff -urN linux-2.6.0-vanilla/drivers/ide/ide-proc.c linux-2.6.0/drivers/ide/ide-proc.c
--- linux-2.6.0-vanilla/drivers/ide/ide-proc.c	Thu Nov 27 07:44:17 2003
+++ linux-2.6.0/drivers/ide/ide-proc.c	Tue Jan  6 23:07:52 2004
@@ -348,8 +348,11 @@
 	int		len;
 	const char	*name;
 
+	/*
+	 *  Neither ide_unknown nor ide_forced should be set at this point.
+	 */
+	
 	switch (hwif->chipset) {
-		case ide_unknown:	name = "(none)";	break;
 		case ide_generic:	name = "generic";	break;
 		case ide_pci:		name = "pci";		break;
 		case ide_cmd640:	name = "cmd640";	break;
diff -urN linux-2.6.0-vanilla/drivers/ide/ide.c linux-2.6.0/drivers/ide/ide.c
--- linux-2.6.0-vanilla/drivers/ide/ide.c	Thu Nov 27 07:43:29 2003
+++ linux-2.6.0/drivers/ide/ide.c	Tue Jan  6 23:08:17 2004
@@ -2185,7 +2185,7 @@
 				memcpy(hwif->io_ports, hwif->hw.io_ports, sizeof(hwif->io_ports));
 				hwif->irq      = vals[2];
 				hwif->noprobe  = 0;
-				hwif->chipset  = ide_generic;
+				hwif->chipset  = ide_forced;
 				goto done;
 
 			case 0: goto bad_option;
diff -urN linux-2.6.0-vanilla/drivers/ide/pci/cmd640.c linux-2.6.0/drivers/ide/pci/cmd640.c
--- linux-2.6.0-vanilla/drivers/ide/pci/cmd640.c	Thu Nov 27 07:45:36 2003
+++ linux-2.6.0/drivers/ide/pci/cmd640.c	Tue Jan  6 13:07:51 2004
@@ -419,7 +419,7 @@
 	cmd_hwif1 = &ide_hwifs[1]; /* default, if not found below */
 	for (i = 0; i < MAX_HWIFS; i++) {
 		ide_hwif_t *hwif = &ide_hwifs[i];
-		if (hwif->chipset == ide_unknown || hwif->chipset == ide_generic) {
+		if (hwif->chipset == ide_unknown || hwif->chipset == ide_forced) {
 			if (hwif->io_ports[IDE_DATA_OFFSET] == 0x1f0)
 				cmd_hwif0 = hwif;
 			else if (hwif->io_ports[IDE_DATA_OFFSET] == 0x170)
diff -urN linux-2.6.0-vanilla/drivers/ide/setup-pci.c linux-2.6.0/drivers/ide/setup-pci.c
--- linux-2.6.0-vanilla/drivers/ide/setup-pci.c	Thu Nov 27 07:43:39 2003
+++ linux-2.6.0/drivers/ide/setup-pci.c	Tue Jan  6 23:18:50 2004
@@ -59,7 +59,7 @@
 	for (h = 0; h < MAX_HWIFS; ++h) {
 		hwif = &ide_hwifs[h];
 		if (hwif->io_ports[IDE_DATA_OFFSET] == io_base) {
-			if (hwif->chipset == ide_generic)
+			if (hwif->chipset == ide_forced)
 				return hwif; /* a perfect match */
 		}
 	}
diff -urN linux-2.6.0-vanilla/include/linux/ide.h linux-2.6.0/include/linux/ide.h
--- linux-2.6.0-vanilla/include/linux/ide.h	Thu Nov 27 07:43:36 2003
+++ linux-2.6.0/include/linux/ide.h	Tue Jan  6 23:06:23 2004
@@ -282,7 +282,7 @@
 		ide_pdc4030,	ide_rz1000,	ide_trm290,
 		ide_cmd646,	ide_cy82c693,	ide_4drives,
 		ide_pmac,	ide_etrax100,	ide_acorn,
-		ide_pc9800
+		ide_pc9800, ide_forced
 } hwif_chipset_t;
 
 /*

Re: [PATCH] various IDE patches/cleanups

[Davin McCall]

2nd patch notes
---------------

The function "ide_stall_queue" is used to "give back excess bandwidth" for
a drive. What this means is that some request comes in a drive, and it is
unable to process the request immediately (because the hardware is still
seeking or powering up or somesuch) then, instead of just waiting for the
hardware, we can stall the queue. See drivers/ide/ide-cdrom.c for example.

The intention would seem to be to stall the drive - there doesn't seem to
be any necessity to stall other drives in the same hwgroup (or even on the
same interface). But this is exactly what the current code does.

This patch changes the behaviour to stall only the drive, not the whole
hwgroup.

- (ide-io.c)
  - ide_do_request()
    - Set hwgroup->busy false when sleeping, so that requests on other drives
      can still come in.
    - if no requests can be served (choose_drive returns NULL) and we are
      already sleeping, break out early (after setting ->busy false)
    - if a request is chosen (choose_drive) and ->sleeping is currently set,
      remove the expiry timer and clear ->sleeping.
  - ide_timer_expiry()
    - no need to clear ->busy when the sleep timer expires, as we now sleep
      with ->busy clear.



diff -urN linux-2.6.0-patch1/drivers/ide/ide-io.c linux-2.6.0/drivers/ide/ide-io.c
--- linux-2.6.0-patch1/drivers/ide/ide-io.c	Thu Nov 27 07:45:21 2003
+++ linux-2.6.0/drivers/ide/ide-io.c	Wed Jan 28 22:55:00 2004
@@ -843,6 +843,8 @@
 		drive = choose_drive(hwgroup);
 		if (drive == NULL) {
 			unsigned long sleep = 0;
+			hwgroup->busy = ata_pending_commands(hwgroup->drive);
+			if (hwgroup->sleeping) break;
 			hwgroup->rq = NULL;
 			drive = hwgroup->drive;
 			do {
@@ -865,8 +867,6 @@
 				/* so that ide_timer_expiry knows what to do */
 				hwgroup->sleeping = 1;
 				mod_timer(&hwgroup->timer, sleep);
-				/* we purposely leave hwgroup->busy==1
-				 * while sleeping */
 			} else {
 				/* Ugly, but how can we sleep for the lock
 				 * otherwise? perhaps from tq_disk?
@@ -874,12 +874,20 @@
 
 				/* for atari only */
 				ide_release_lock();
-				hwgroup->busy = 0;
 			}
 
 			/* no more work for this hwgroup (for now) */
 			return;
 		}
+		
+		if (hwgroup->sleeping) {
+			if (!del_timer(&hwgroup->timer)) {
+				hwgroup->busy = 0;
+				break;  /* let the timer handler finish; it will call us again */
+			}
+			hwgroup->sleeping = 0;
+		}
+		
 		hwif = HWIF(drive);
 		if (hwgroup->hwif->sharing_irq &&
 		    hwif != hwgroup->hwif &&
@@ -1060,7 +1068,6 @@
 		 */
 		if (hwgroup->sleeping) {
 			hwgroup->sleeping = 0;
-			hwgroup->busy = 0;
 		}
 	} else {
 		ide_drive_t *drive = hwgroup->drive;

Re: [PATCH] various IDE patches/cleanups

[Davin McCall]

patch 3
-------
Simple patch to check the return of del_timer() in ide_intr(), to avoid
problems with marginal timeouts.

- (ide-io.c)
  - ide_intr()
    - check return from del_timer, exit if timer has expired.


diff -urN linux-2.6.0-patch2/drivers/ide/ide-io.c linux-2.6.0/drivers/ide/ide-io.c
--- linux-2.6.0-patch2/drivers/ide/ide-io.c	Wed Jan 28 22:55:00 2004
+++ linux-2.6.0/drivers/ide/ide-io.c	Wed Jan 28 23:49:17 2004
@@ -1303,8 +1303,12 @@
 		hwgroup->busy = 1;	/* paranoia */
 		printk(KERN_ERR "%s: ide_intr: hwgroup->busy was 0 ??\n", drive->name);
 	}
+	if (!del_timer(&hwgroup->timer)) {
+		/* timer has expired, ide_timer_expiry is waiting to get lock */
+		spin_unlock(&ide_lock);
+		return IRQ_HANDLED;
+	}
 	hwgroup->handler = NULL;
-	del_timer(&hwgroup->timer);
 	spin_unlock(&ide_lock);
 
 	if (drive->unmask)

Re: [PATCH] various IDE patches/cleanups

[Davin McCall]

4th patch notes
---------------

choose_drive() selects which drive in a hwgroup will have a request serviced.
It tries not to return a drive whose queue is plugged, but there's a small
mistake which means it might do this sometimes.

Once we've guarenteed that the returned drive is never plugged, it's possible
to remove a check in ide_do_request() (or actually, modify it, seeing as the
drive can still get plugged between requests in the tcq case).

- (ide-io.c)
  - ide_do_request()
    - Move check of blk_queue_plugged() just after the check for the service
      routine returning ide_released. So we only check it between tcq
      requests.
  - choose_drive()
    - A plugged drive can't benefit from "nice" behaviour. Neither can
      a drive with no requests in its queue.

--- linux-2.6.0-patch3/drivers/ide/ide-io.c	Thu Jan 29 12:43:26 2004
+++ linux-2.6.0/drivers/ide/ide-io.c	Thu Jan 29 12:48:09 2004
@@ -776,7 +776,9 @@
 				if (!drive->sleep
 				/* FIXME: use time_before */
 				 && 0 < (signed long)(WAKEUP(drive) - (jiffies - best->service_time))
-				 && 0 < (signed long)((jiffies + t) - WAKEUP(drive)))
+				 && 0 < (signed long)((jiffies + t) - WAKEUP(drive))
+				 && !blk_queue_plugged(drive->queue)
+				 && !elv_queue_empty(drive->queue))
 				{
 					ide_stall_queue(best, IDE_MIN(t, 10 * WAIT_MIN_SLEEP));
 					goto repeat;
@@ -908,14 +910,6 @@
 			break;
 		}
 
-		if (blk_queue_plugged(drive->queue)) {
-			if (drive->using_tcq)
-				break;
-
-			printk(KERN_ERR "ide: huh? queue was plugged!\n");
-			break;
-		}
-
 		/*
 		 * we know that the queue isn't empty, but this can happen
 		 * if the q->prep_rq_fn() decides to kill a request
@@ -967,8 +961,11 @@
 		spin_lock_irq(&ide_lock);
 		if (hwif->irq != masked_irq)
 			enable_irq(hwif->irq);
-		if (startstop == ide_released)
+		if (startstop == ide_released) {
+			if (blk_queue_plugged(drive->queue))
+				break;		
 			goto queue_next;
+		}
 		if (startstop == ide_stopped)
 			hwgroup->busy = 0;
 	}

Re: [PATCH] various IDE patches/cleanups

[Davin McCall]

5th patch notes
---------------

Ultimately this patch avoids the possibility in several places of returning
EBUSY error. This mainly affects /proc interface and ioctl's.

What we do is put a wait queue in the hwgroup structure, which can be used
to receive notification that the hwgroup is no longer busy. This avoids the
"test - sleep - test ..." style of hwgroup grabbing.

- (ide.h)
  - add notbusyq (wait_queue_head_t) to struct hwgroup_s.
  - remove function declaration ide_spin_wait_hwgroup
  - add function declaration ide_wait_hwgroup
  - define inline function ide_kick_queue
    (restart queue processing after grab)
- (ide-probe.c)
  - initialise the waitqueue when allocating the hwgroup
- (ide-io.c)
  - ide_do_request()
    - First thing in the service loop, check the wait queue. If it's not
      empty, do not process further requests, and wake a waiter if no requests
      are currently being executed (leave ->busy set to avoid re-entrance;
      it's the waiters responsibility to clear it).
- (ide.c)
  - new function ide_wait_hwgroup. A replacement for ide_spin_wait_hwgroup.
    - must be called with ide_lock held.
    - ide_spin_wait_hwgroup, if successful, returned with ide_lock held
      and ->busy clear. The new function returns with ide_lock held but
      busy may be clear or set.
  - remove ide_spin_wait_hwgroup().
  - ide_write_setting()
    - trivial: correct comment
    - use new ide_wait_hwgroup mechanism
- (ide-disk.c)
  - set_nowerr()
    - use new ide_wait_hwgroup().
- (ide-proc.c)
  - proc_ide_write_config()
    - use new ide_wait_hwgroup() mechanism.


diff -urN linux-2.6.0-patch4/drivers/ide/ide-disk.c linux-2.6.0/drivers/ide/ide-disk.c
--- linux-2.6.0-patch4/drivers/ide/ide-disk.c	Thu Jan 29 13:53:38 2004
+++ linux-2.6.0/drivers/ide/ide-disk.c	Thu Jan 29 13:56:30 2004
@@ -1360,10 +1360,11 @@
 
 static int set_nowerr(ide_drive_t *drive, int arg)
 {
-	if (ide_spin_wait_hwgroup(drive))
-		return -EBUSY;
+	spin_lock_irq(&ide_lock);
+	ide_wait_hwgroup(HWGROUP(drive));
 	drive->nowerr = arg;
 	drive->bad_wstat = arg ? BAD_R_STAT : BAD_W_STAT;
+	ide_kick_queue(HWGROUP(drive));
 	spin_unlock_irq(&ide_lock);
 	return 0;
 }
diff -urN linux-2.6.0-patch4/drivers/ide/ide-io.c linux-2.6.0/drivers/ide/ide-io.c
--- linux-2.6.0-patch4/drivers/ide/ide-io.c	Thu Jan 29 12:48:09 2004
+++ linux-2.6.0/drivers/ide/ide-io.c	Thu Jan 29 13:56:30 2004
@@ -842,10 +842,21 @@
 
 	while (!hwgroup->busy) {
 		hwgroup->busy = 1;
+		if (waitqueue_active(&hwgroup->notbusyq)) {
+			/*
+			 * check each drive to make sure there are no pending commands ie. that
+			 * we are truly "not busy".
+			 */
+			if (ata_pending_commands(hwgroup->drive))
+				return;
+			wake_up(&hwgroup->notbusyq);
+			return;
+		}
+		
 		drive = choose_drive(hwgroup);
 		if (drive == NULL) {
 			unsigned long sleep = 0;
-			hwgroup->busy = ata_pending_commands(hwgroup->drive);
+			hwgroup->busy = ata_pending_commands(drive);
 			if (hwgroup->sleeping) break;
 			hwgroup->rq = NULL;
 			drive = hwgroup->drive;
diff -urN linux-2.6.0-patch4/drivers/ide/ide-probe.c linux-2.6.0/drivers/ide/ide-probe.c
--- linux-2.6.0-patch4/drivers/ide/ide-probe.c	Thu Jan 29 13:53:38 2004
+++ linux-2.6.0/drivers/ide/ide-probe.c	Thu Jan 29 13:56:30 2004
@@ -1050,6 +1050,7 @@
 		init_timer(&hwgroup->timer);
 		hwgroup->timer.function = &ide_timer_expiry;
 		hwgroup->timer.data = (unsigned long) hwgroup;
+		init_waitqueue_head(&hwgroup->notbusyq);
 	}
 
 	/*
diff -urN linux-2.6.0-patch4/drivers/ide/ide-proc.c linux-2.6.0/drivers/ide/ide-proc.c
--- linux-2.6.0-patch4/drivers/ide/ide-proc.c	Thu Jan 29 13:53:38 2004
+++ linux-2.6.0/drivers/ide/ide-proc.c	Thu Jan 29 13:56:30 2004
@@ -127,6 +127,8 @@
 	int		for_real = 0;
 	unsigned long	startn = 0, n, flags;
 	const char	*start = NULL, *msg = NULL;
+	ide_hwgroup_t *mygroup = (ide_hwgroup_t *)hwif->hwgroup;
+	ide_hwgroup_t *mategroup = NULL;
 
 	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
 		return -EACCES;
@@ -145,22 +147,19 @@
 	do {
 		const char *p;
 		if (for_real) {
-			unsigned long timeout = jiffies + (3 * HZ);
-			ide_hwgroup_t *mygroup = (ide_hwgroup_t *)(hwif->hwgroup);
-			ide_hwgroup_t *mategroup = NULL;
 			if (hwif->mate && hwif->mate->hwgroup)
 				mategroup = (ide_hwgroup_t *)(hwif->mate->hwgroup);
-			spin_lock_irqsave(&ide_lock, flags);
-			while (mygroup->busy ||
-			       (mategroup && mategroup->busy)) {
-				spin_unlock_irqrestore(&ide_lock, flags);
-				if (time_after(jiffies, timeout)) {
-					printk("/proc/ide/%s/config: channel(s) busy, cannot write\n", hwif->name);
-					spin_unlock_irqrestore(&ide_lock, flags);
-					return -EBUSY;
-				}
-				spin_lock_irqsave(&ide_lock, flags);
+			if (mategroup == mygroup)
+				mategroup = NULL;
+			if (mategroup && mategroup < mygroup) {
+				/* arrange a consistent locking ordering, to avoid deadlock */
+				mygroup = mategroup;
+				mategroup = hwif->hwgroup;
 			}
+			spin_lock_irqsave(&ide_lock, flags);
+			ide_wait_hwgroup(mygroup);
+			if (mategroup)
+				ide_wait_hwgroup(mategroup);				
 		}
 		p = buffer;
 		n = count;
@@ -242,6 +241,9 @@
 							break;
 					}
 					if (rc) {
+						ide_kick_queue(mygroup);
+						if (mategroup)
+							ide_kick_queue(mategroup);						
 						spin_unlock_irqrestore(&ide_lock, flags);
 						printk("proc_ide_write_config: error writing %s at bus %02x dev %02x reg 0x%x value 0x%x\n",
 							msg, dev->bus->number, dev->devfn, reg, val);
@@ -284,6 +286,9 @@
 			}
 		}
 	} while (!for_real++);
+	ide_kick_queue(mygroup);
+	if (mategroup)
+		ide_kick_queue(mategroup);
 	spin_unlock_irqrestore(&ide_lock, flags);
 	return count;
 parse_error:
diff -urN linux-2.6.0-patch4/drivers/ide/ide.c linux-2.6.0/drivers/ide/ide.c
--- linux-2.6.0-patch4/drivers/ide/ide.c	Thu Jan 29 13:53:38 2004
+++ linux-2.6.0/drivers/ide/ide.c	Thu Jan 29 13:56:30 2004
@@ -1270,32 +1270,47 @@
 	return val;
 }
 
-int ide_spin_wait_hwgroup (ide_drive_t *drive)
+/*
+ * ide_wait_hwgroup  - Wait for the hwgroup to become not busy.
+ * @hwgroup: the group to wait for.
+ *
+ * Call with "ide_lock" spinlock held.
+ *
+ * If it returns with hwgroup->busy clear, caller can simply release ide_lock
+ * to continue processing. To release ide_lock without releasing the hwgroup,
+ * set ->busy before releasing ide_lock.
+ *
+ * If it returns with hwgroup->busy set, use ide_kick_queue() to release the
+ * hwgroup. ide_lock can be released at any time (but must be re-acquired
+ * before calling ide_kick_queue, and released afterwards).
+ *
+ * ide_kick_queue() can be used in the former case (->busy clear) also.
+ */
+void ide_wait_hwgroup (ide_hwgroup_t *hwgroup)
 {
-	ide_hwgroup_t *hwgroup = HWGROUP(drive);
-	unsigned long timeout = jiffies + (3 * HZ);
-
+	DEFINE_WAIT(wait);
+	
+	if (!hwgroup->busy)
+		return;
+	
+	/*
+	 * The wait queue is effectively protected by ide_lock, so it's possible
+	 * to bypass some of the usual mechanisms here
+	 */
+	wait.flags |= WQ_FLAG_EXCLUSIVE;
+	__add_wait_queue_tail(&hwgroup->notbusyq,&wait);
+	set_current_state(TASK_UNINTERRUPTIBLE);
+	spin_unlock_irq(&ide_lock);
+	schedule();
+	
 	spin_lock_irq(&ide_lock);
-
-	while (hwgroup->busy) {
-		unsigned long lflags;
-		spin_unlock_irq(&ide_lock);
-		local_irq_set(lflags);
-		if (time_after(jiffies, timeout)) {
-			local_irq_restore(lflags);
-			printk(KERN_ERR "%s: channel busy\n", drive->name);
-			return -EBUSY;
-		}
-		local_irq_restore(lflags);
-		spin_lock_irq(&ide_lock);
-	}
-	return 0;
+	return;
 }
-
-EXPORT_SYMBOL(ide_spin_wait_hwgroup);
+	
+EXPORT_SYMBOL(ide_wait_hwgroup);
 
 /**
- *	ide_write_setting	-	read an IDE setting
+ *	ide_write_setting	-	write an IDE setting
  *	@drive: drive to read from
  *	@setting: drive setting
  *	@val: value
@@ -1306,10 +1321,6 @@
  *	BUGS: the data return and error are the same return value
  *	so an error -EINVAL and true return of the same value cannot
  *	be told apart
- *
- *	FIXME:  This should be changed to enqueue a special request
- *	to the driver to change settings, and then wait on a sema for completion.
- *	The current scheme of polling is kludgy, though safe enough.
  */
 int ide_write_setting (ide_drive_t *drive, ide_settings_t *setting, int val)
 {
@@ -1324,8 +1335,8 @@
 		return -EINVAL;
 	if (setting->set)
 		return setting->set(drive, val);
-	if (ide_spin_wait_hwgroup(drive))
-		return -EBUSY;
+	spin_lock_irq(&ide_lock);
+	ide_wait_hwgroup(HWGROUP(drive));
 	switch (setting->data_type) {
 		case TYPE_BYTE:
 			*((u8 *) setting->data) = val;
@@ -1342,6 +1353,7 @@
 				*p = val;
 			break;
 	}
+	ide_kick_queue(HWGROUP(drive));
 	spin_unlock_irq(&ide_lock);
 	return 0;
 }
diff -urN linux-2.6.0-patch4/include/linux/ide.h linux-2.6.0/include/linux/ide.h
--- linux-2.6.0-patch4/include/linux/ide.h	Thu Jan 29 13:52:44 2004
+++ linux-2.6.0/include/linux/ide.h	Thu Jan 29 13:55:50 2004
@@ -1066,6 +1066,9 @@
 		/* ide_system_bus_speed */
 	int pio_clock;
 
+		/* wait for group to become not-busy */
+	wait_queue_head_t notbusyq;
+
 	unsigned char cmd_buf[4];
 } ide_hwgroup_t;
 
@@ -1606,11 +1609,25 @@
  */
 extern void ide_stall_queue(ide_drive_t *drive, unsigned long timeout);
 
-extern int ide_spin_wait_hwgroup(ide_drive_t *);
+extern void ide_wait_hwgroup(ide_hwgroup_t *);
 extern void ide_timer_expiry(unsigned long);
 extern irqreturn_t ide_intr(int irq, void *dev_id, struct pt_regs *regs);
 extern void do_ide_request(request_queue_t *);
+extern void ide_do_request(ide_hwgroup_t *, int masked_irq);
 extern void ide_init_subdrivers(void);
+
+/*
+ * For use after ide_wait_hwgroup(). Kick a queue if it needs to be kicked.
+ * Ie. ensure requests are processed. Call with ide_lock held.
+ */
+static inline void ide_kick_queue(ide_hwgroup_t *q)
+{
+	if (q->busy) {
+		q->busy = 0;
+		if (q->drive)
+			do_ide_request(q->drive->queue);
+	}
+}
 
 extern struct block_device_operations ide_fops[];
 extern ide_proc_entry_t generic_subdriver_entries[];

Re: [PATCH] various IDE patches/cleanups

[Davin McCall]

Damn, I found a problem with this. Needs to set hwgroup->expiry to NULL before releasing ide_lock; that prevents ide_timer_expiry() from running the expiry() handler and instead it will simulate an interrupt.

Otherwise, expiry() may return non-zero which will cause the timer to be reset. The interrupt would effectively be lost. In some cases (abuses?) this would lock the whole hwgroup forever as the expiry() function always returns > 0 (ide-cd.c, cdrom_timer_expiry() for example).

Here's the revised patch.


diff -urN linux-2.6.0-patch2/drivers/ide/ide-io.c linux-2.6.0/drivers/ide/ide-io.c
--- linux-2.6.0-patch2/drivers/ide/ide-io.c	Wed Jan 28 22:55:00 2004
+++ linux-2.6.0/drivers/ide/ide-io.c	Wed Jan 28 23:49:17 2004
@@ -1303,8 +1303,12 @@
 		hwgroup->busy = 1;	/* paranoia */
 		printk(KERN_ERR "%s: ide_intr: hwgroup->busy was 0 ??\n", drive->name);
 	}
+	if (!del_timer(&hwgroup->timer)) {
+		/* timer has expired, ide_timer_expiry is waiting to get lock */
+		hwgroup->expiry = NULL;
+		spin_unlock(&ide_lock);
+		return IRQ_HANDLED;
+	}
 	hwgroup->handler = NULL;
-	del_timer(&hwgroup->timer);
 	spin_unlock(&ide_lock);
 
 	if (drive->unmask)

Re: [PATCH] various IDE patches/cleanups

[Davin McCall]

Grumble grumble "patch" doesn't like my hand-editing.
Try again.


diff -urN linux-2.6.0-patch2/drivers/ide/ide-io.c linux-2.6.0/drivers/ide/ide-io.c
--- linux-2.6.0-patch2/drivers/ide/ide-io.c	Wed Jan 28 22:55:00 2004
+++ linux-2.6.0/drivers/ide/ide-io.c	Wed Jan 28 23:49:17 2004
@@ -1303,8 +1303,13 @@
 		hwgroup->busy = 1;	/* paranoia */
 		printk(KERN_ERR "%s: ide_intr: hwgroup->busy was 0 ??\n", drive->name);
 	}
+	if (!del_timer(&hwgroup->timer)) {
+		/* timer has expired, ide_timer_expiry is waiting to get lock */
+		hwgroup->expiry = NULL;
+		spin_unlock(&ide_lock);
+		return IRQ_HANDLED;
+	}
 	hwgroup->handler = NULL;
-	del_timer(&hwgroup->timer);
 	spin_unlock(&ide_lock);
 
 	if (drive->unmask)

Re: [PATCH] various IDE patches/cleanups

[Bartlomiej Zolnierkiewicz]

Hi,

On Friday 30 of January 2004 04:27, you wrote:
> I've been doing some hacking on the IDE layer, just to fix a few issues I
> noticed going through the code. Due to the complex nature of the code I'm
> bound to have missed some things and perhaps misunderstood others.
> Nevertheless I'm posting these patches in the hope that they can be tested
> on other machines, rejected, or even accepted.
>
> Comments and criticisms are welcome.

Thanks!  I'm slowly reading through them (yeah, code is really complex).
Patch 3rd and 5th seems okay, 2nd and 4th need some more checking/thinking.

> The first patch, below, is already included in the -mm tree. The further
> patches are appearing here for the first time.

It was pushed to Linus and merged.

--bart

Re: [PATCH] various IDE patches/cleanups

[Davin McCall]

Hi,

On Tue, 3 Feb 2004 20:41:09 +0100
Bartlomiej Zolnierkiewicz <B.Zolnierkiewicz@elka.pw.edu.pl> wrote:

> Thanks!  I'm slowly reading through them (yeah, code is really complex).

I'm also trying to put together some brief documentation on how the code/data structures fit together. Perhaps I should post it when I'm done? Might be useful for anyone else looking at the IDE layer.

> Patch 3rd and 5th seems okay, 2nd and 4th need some more checking/thinking.

3rd and 5th are also the most worthwhile, I think. 2nd and 4th are really only optimizations. Hopefully there are no problems with them, though.

> > The first patch, below, is already included in the -mm tree. The further
> > patches are appearing here for the first time.
> 
> It was pushed to Linus and merged.

Good :-)  Considering how many tries it took me to get it right, I'm glad it's gotten in...

/Davin