Re: [PATCH] kill ide-geometry.c, fix boot problems

Re: [PATCH] kill ide-geometry.c, fix boot problems

[Andries.Brouwer]

    From alan@lxorguk.ukuu.org.uk  Wed Apr 16 18:26:31 2003

    On Mer, 2003-04-16 at 16:16, Andries.Brouwer@cwi.nl wrote:
    > All traces of ide_xlate_1024 have been removed.
    > Few people need it, and sometimes it was directly
    > harmful. (And it is dead code in 2.5.recent.)
    > There are now boot options "remap" and "remap63"
    > for people with EZD or DM.

    There are lots of people with remap/remap63 needs. This should
    be automated as it was before or it will be a nightmare.
    You are desperate to remove the ide_xlate stuff but you can't
    do that without providing equivalent automatic functionality,
    either thats in base or that the vendors all just merge anyway.

Ha, Alan - what a choice of words. "Nightmare". "Desperate".
It is not that bad.

About this particular patch: what is removed is dead code,
so your complaint is directed against some earlier patch
(maybe 2.5.30 or so). At that time we exchanged a letter or two
and left it at that.

This xlate stuff consists of two halves: geometry stuff and
remapping stuff. Both must die. From the point of view of a
vendor, geometry stuff is uninteresting - it is not used anywhere.
The remapping stuff has some interest, but the interest is really
small, and the vendor does its clients a definite disservice by
automatically assuming that a client wants these strange constructions.

Especially when the disk is to be a Linux-only disk it is very
unlikely that a client should want a disk manager.
I have guided many a user in getting rid of DM.

It is good that this semi-incorrect policy disappears from the kernel.
Of course we do not want to lose power. After 2.5.30 I said: as soon
as somebody needs it, I'll add a boot option. It happened today.

Now you are a vendor and want to do things automatically.
Three possibilities:

(i) Add an ioctl to do the remap at runtime
(I can also add it if you prefer) - only a few lines.
Now the RedHat installer can do the remap in case it detects
a disk manager. That is nice, because that means that in case of
a whole-disk install it could ask the user whether she wants to
preserve this animal. Probably she doesnt.

(ii) Do partition reading in initial userspace.
Now you are free to do what you want. It is userspace.

(iii) Revert some patches.

I get the impression that you prefer (iii). I prefer (ii).
Most realistic may be (i).

Andries

Re: [PATCH] kill ide-geometry.c, fix boot problems

[Jamie Lokier]

Andries.Brouwer@cwi.nl wrote:
> Now the RedHat installer can do the remap in case it detects
> a disk manager. That is nice, because that means that in case of
> a whole-disk install it could ask the user whether she wants to
> preserve this animal. Probably she doesnt.

That is quite nice - ask user at install time whether to remove the
disk manager.

However, after doing that it seems appropriate for a booting kernel to
autodetect the presence or absence of the disk manager and behave
accordingly.

Just like the user gets to choose what goes in the partition table at
install time, and after that it is read automatically.

The user does not (usually) have to supply special kernel options to
tell it where the partitions are, or what the disk geometry is.  The
kernel can do that automatically.  So why does this not apply to known
disk manager remappings?

-- Jamie

Re: [PATCH] kill ide-geometry.c, fix boot problems

[Alan Cox]

On Iau, 2003-04-17 at 00:00, Andries.Brouwer@cwi.nl wrote:
> But of course, it is easy to add some heuristic code.
> I would prefer to keep such code out of the vanilla kernel,
> but a vendor might want it.
> 
> [And note that todays patch did not remove any DM detection.
> This discussion is 37 patch levels late.]


For Linus tree maybe, but my tree never got rid of the stuff because
I never agreed with you.

Re: [PATCH] kill ide-geometry.c, fix boot problems

[Andries.Brouwer]

> I couldn't get rid of the disk manager on an old 486 running RH 5 --
> did I miss something or was it really impossible?

In the beginning it was really difficult.
Later I added the noremap boot option that simplified things.

Re: [PATCH] kill ide-geometry.c, fix boot problems

[Chuck Ebbert]

> Especially when the disk is to be a Linux-only disk it is very
> unlikely that a client should want a disk manager.
> I have guided many a user in getting rid of DM.


  I couldn't get rid of the disk manager on an old 486 running RH 5 --
did I miss something or was it really impossible?


--
 Chuck

Re: [PATCH] kill ide-geometry.c, fix boot problems

[Andries.Brouwer]

    From: Jamie Lokier <jamie@shareable.org>

    Andries.Brouwer@cwi.nl wrote:
    > Now the RedHat installer can do the remap in case it detects
    > a disk manager. That is nice, because that means that in case of
    > a whole-disk install it could ask the user whether she wants to
    > preserve this animal. Probably she doesnt.

    That is quite nice - ask user at install time whether to remove the
    disk manager.

    However, after doing that it seems appropriate for a booting kernel to
    autodetect the presence or absence of the disk manager and behave
    accordingly.

Once the redhat installer has determined that there really
is a disk manager, and that the user wants it that way, it can
add the boot parameter hda=remap63.

I dislike autodetection. The word sounds so nice. But a synonym
is guessing.

If kernel guessing is required it means that some interface is missing.
Policy should be determined from userspace.

But of course, it is easy to add some heuristic code.
I would prefer to keep such code out of the vanilla kernel,
but a vendor might want it.

[And note that todays patch did not remove any DM detection.
This discussion is 37 patch levels late.]


Andries

Re: [PATCH] kill ide-geometry.c, fix boot problems

[Alan Cox]

On Mer, 2003-04-16 at 16:16, Andries.Brouwer@cwi.nl wrote:
> All traces of ide_xlate_1024 have been removed.
> Few people need it, and sometimes it was directly
> harmful. (And it is dead code in 2.5.recent.)
> There are now boot options "remap" and "remap63"
> for people with EZD or DM.

There are lots of people with remap/remap63 needs. This should be automated
as it was before or it will be a nightmare. You are desperate to remove
the ide_xlate stuff but you can't do that without providing equivalent
automatic functionality, either thats in base or that the vendors all
just merge anyway.

[PATCH] kill ide-geometry.c, fix boot problems

[Andries.Brouwer]

[Bug 588] is about boot problems of someone with
Disk Manager on 2.5.67. The patch below fixes the
problems, and is a large cleanup.

All traces of ide_xlate_1024 have been removed.
Few people need it, and sometimes it was directly
harmful. (And it is dead code in 2.5.recent.)
There are now boot options "remap" and "remap63"
for people with EZD or DM.

Andries

>> please report

> That patch worked just fine ;)
> Now it booted, and averything is nice.

This also means that someone can close the bug report.

 Documentation/ide.txt      |    4 +
 drivers/ide/Makefile       |    2 
 drivers/ide/ide-geometry.c |  135 ---------------------------------------------
 drivers/ide/ide-probe.c    |    4 -
 drivers/ide/ide.c          |   19 ++++--
 fs/partitions/msdos.c      |   18 +-----
 include/linux/ide.h        |    6 --
 7 files changed, 20 insertions(+), 168 deletions(-)

--------------------------------------------------------

diff -u --recursive --new-file -X /linux/dontdiff a/Documentation/ide.txt b/Documentation/ide.txt
--- a/Documentation/ide.txt	Tue Mar 18 11:48:10 2003
+++ b/Documentation/ide.txt	Wed Apr 16 14:50:15 2003
@@ -230,6 +230,10 @@
  "hdx=cdrom"		: drive is present, and is a cdrom drive
  
  "hdx=cyl,head,sect"	: disk drive is present, with specified geometry
+
+ "hdx=remap"		: remap access of sector 0 to sector 1 (for EZD)
+
+ "hdx=remap63"		: remap the drive: shift all by 63 sectors (for DM)
  
  "hdx=autotune"		: driver will attempt to tune interface speed
 			  to the fastest PIO mode supported,
diff -u --recursive --new-file -X /linux/dontdiff a/drivers/ide/Makefile b/drivers/ide/Makefile
--- a/drivers/ide/Makefile	Tue Mar 25 04:54:31 2003
+++ b/drivers/ide/Makefile	Wed Apr 16 14:46:25 2003
@@ -12,7 +12,7 @@
 
 # Core IDE code - must come before legacy
 
-obj-$(CONFIG_BLK_DEV_IDE)		+= ide-io.o ide-probe.o ide-geometry.o ide-iops.o ide-taskfile.o ide.o ide-lib.o ide-default.o
+obj-$(CONFIG_BLK_DEV_IDE)		+= ide-io.o ide-probe.o ide-iops.o ide-taskfile.o ide.o ide-lib.o ide-default.o
 obj-$(CONFIG_BLK_DEV_IDEDISK)		+= ide-disk.o
 obj-$(CONFIG_BLK_DEV_IDECD)		+= ide-cd.o
 obj-$(CONFIG_BLK_DEV_IDETAPE)		+= ide-tape.o
diff -u --recursive --new-file -X /linux/dontdiff a/drivers/ide/ide-geometry.c b/drivers/ide/ide-geometry.c
--- a/drivers/ide/ide-geometry.c	Tue Mar 25 04:54:31 2003
+++ b/drivers/ide/ide-geometry.c	Thu Jan  1 01:00:00 1970
@@ -1,135 +0,0 @@
-/*
- * linux/drivers/ide/ide-geometry.c
- */
-#include <linux/config.h>
-#include <linux/ide.h>
-#include <linux/mc146818rtc.h>
-#include <asm/io.h>
-
-extern unsigned long current_capacity (ide_drive_t *);
-
-/*
- * If heads is nonzero: find a translation with this many heads and S=63.
- * Otherwise: find out how OnTrack Disk Manager would translate the disk.
- */
-
-static void ontrack(ide_drive_t *drive, int heads, unsigned int *c, int *h, int *s) 
-{
-	static const u8 dm_head_vals[] = {4, 8, 16, 32, 64, 128, 255, 0};
-	const u8 *headp = dm_head_vals;
-	unsigned long total;
-
-	/*
-	 * The specs say: take geometry as obtained from Identify,
-	 * compute total capacity C*H*S from that, and truncate to
-	 * 1024*255*63. Now take S=63, H the first in the sequence
-	 * 4, 8, 16, 32, 64, 128, 255 such that 63*H*1024 >= total.
-	 * [Please tell aeb@cwi.nl in case this computes a
-	 * geometry different from what OnTrack uses.]
-	 */
-	total = DRIVER(drive)->capacity(drive);
-
-	*s = 63;
-
-	if (heads) {
-		*h = heads;
-		*c = total / (63 * heads);
-		return;
-	}
-
-	while (63 * headp[0] * 1024 < total && headp[1] != 0)
-		 headp++;
-	*h = headp[0];
-	*c = total / (63 * headp[0]);
-}
-
-/*
- * This routine is called from the partition-table code in pt/msdos.c.
- * It has two tasks:
- * (i) to handle Ontrack DiskManager by offsetting everything by 63 sectors,
- *  or to handle EZdrive by remapping sector 0 to sector 1.
- * (ii) to invent a translated geometry.
- * Part (i) is suppressed if the user specifies the "noremap" option
- * on the command line.
- * Part (ii) is suppressed if the user specifies an explicit geometry.
- *
- * The ptheads parameter is either 0 or tells about the number of
- * heads shown by the end of the first nonempty partition.
- * If this is either 16, 32, 64, 128, 240 or 255 we'll believe it.
- *
- * The xparm parameter has the following meaning:
- *	 0 = convert to CHS with fewer than 1024 cyls
- *	     using the same method as Ontrack DiskManager.
- *	 1 = same as "0", plus offset everything by 63 sectors.
- *	-1 = similar to "0", plus redirect sector 0 to sector 1.
- *	 2 = convert to a CHS geometry with "ptheads" heads.
- *
- * Returns 0 if the translation was not possible, if the device was not 
- * an IDE disk drive, or if a geometry was "forced" on the commandline.
- * Returns 1 if the geometry translation was successful.
- */
-
-int ide_xlate_1024 (struct block_device *bdev, int xparm, int ptheads, const char *msg)
-{
-	ide_drive_t *drive = bdev->bd_disk->private_data;
-	const char *msg1 = "";
-	int heads = 0;
-	int c, h, s;
-	int transl = 1;		/* try translation */
-	int ret = 0;
-
-	/* remap? */
-	if (drive->remap_0_to_1 != 2) {
-		if (xparm == 1) {		/* DM */
-			drive->sect0 = 63;
-			msg1 = " [remap +63]";
-			ret = 1;
-		} else if (xparm == -1) {	/* EZ-Drive */
-			if (drive->remap_0_to_1 == 0) {
-				drive->remap_0_to_1 = 1;
-				msg1 = " [remap 0->1]";
-				ret = 1;
-			}
-		}
-	}
-
-	/* There used to be code here that assigned drive->id->CHS
-	   to drive->CHS and that to drive->bios_CHS. However,
-	   some disks have id->C/H/S = 4092/16/63 but are larger than 2.1 GB.
-	   In such cases that code was wrong.  Moreover,
-	   there seems to be no reason to do any of these things. */
-
-	/* translate? */
-	if (drive->forced_geom)
-		transl = 0;
-
-	/* does ptheads look reasonable? */
-	if (ptheads == 32 || ptheads == 64 || ptheads == 128 ||
-	    ptheads == 240 || ptheads == 255)
-		heads = ptheads;
-
-	if (xparm == 2) {
-		if (!heads ||
-		   (drive->bios_head >= heads && drive->bios_sect == 63))
-			transl = 0;
-	}
-	if (xparm == -1) {
-		if (drive->bios_head > 16)
-			transl = 0;     /* we already have a translation */
-	}
-
-	if (transl) {
-		ontrack(drive, heads, &c, &h, &s);
-		drive->bios_cyl = c;
-		drive->bios_head = h;
-		drive->bios_sect = s;
-		ret = 1;
-	}
-
-	set_capacity(drive->disk, current_capacity(drive));
-
-	if (ret)
-		printk("%s%s [%d/%d/%d]", msg, msg1,
-		       drive->bios_cyl, drive->bios_head, drive->bios_sect);
-	return ret;
-}
diff -u --recursive --new-file -X /linux/dontdiff a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c
--- a/drivers/ide/ide-probe.c	Tue Mar 25 04:54:31 2003
+++ b/drivers/ide/ide-probe.c	Wed Apr 16 14:46:25 2003
@@ -1442,8 +1442,6 @@
 }
 
 #ifdef MODULE
-extern int (*ide_xlate_1024_hook)(struct block_device *, int, int, const char *);
-
 int init_module (void)
 {
 	unsigned int index;
@@ -1452,14 +1450,12 @@
 		ide_unregister(index);
 	ideprobe_init();
 	create_proc_ide_interfaces();
-	ide_xlate_1024_hook = ide_xlate_1024;
 	return 0;
 }
 
 void cleanup_module (void)
 {
 	ide_probe = NULL;
-	ide_xlate_1024_hook = 0;
 }
 MODULE_LICENSE("GPL");
 #endif /* MODULE */
diff -u --recursive --new-file -X /linux/dontdiff a/drivers/ide/ide.c b/drivers/ide/ide.c
--- a/drivers/ide/ide.c	Tue Apr  8 09:36:37 2003
+++ b/drivers/ide/ide.c	Wed Apr 16 14:46:25 2003
@@ -1711,6 +1711,8 @@
  * "hdx=nowerr"		: ignore the WRERR_STAT bit on this drive
  * "hdx=cdrom"		: drive is present, and is a cdrom drive
  * "hdx=cyl,head,sect"	: disk drive is present, with specified geometry
+ * "hdx=remap63"	: add 63 to all sector numbers (for OnTrack DM)
+ * "hdx=remap"		: remap 0->1 (for EZDrive)
  * "hdx=noremap"	: do not remap 0->1 even though EZD was detected
  * "hdx=autotune"	: driver will attempt to tune interface speed
  *				to the fastest PIO mode supported,
@@ -1830,11 +1832,11 @@
 	 * Look for drive options:  "hdx="
 	 */
 	if (s[0] == 'h' && s[1] == 'd' && s[2] >= 'a' && s[2] <= max_drive) {
-		const char *hd_words[] = {"none", "noprobe", "nowerr", "cdrom",
-				"serialize", "autotune", "noautotune",
-				"slow", "swapdata", "bswap", "flash",
-				"remap", "noremap", "scsi", "biostimings",
-				NULL};
+		const char *hd_words[] = {
+			"none", "noprobe", "nowerr", "cdrom", "serialize",
+			"autotune", "noautotune", "slow", "swapdata", "bswap",
+			"flash", "remap", "noremap", "scsi", "biostimings",
+			"remap63", NULL };
 		unit = s[2] - 'a';
 		hw   = unit / MAX_DRIVES;
 		unit = unit % MAX_DRIVES;
@@ -1884,8 +1886,8 @@
 			case -8: /* "slow" */
 				drive->slow = 1;
 				goto done;
-			case -9: /* "swapdata" or "bswap" */
-			case -10:
+			case -9: /* "swapdata" */
+			case -10: /* "bswap" */
 				drive->bswap = 1;
 				goto done;
 			case -11: /* "flash" */
@@ -1908,6 +1910,9 @@
 			case -15: /* "biostimings" */
 				drive->autotune = IDE_TUNE_BIOS;
 				goto done;
+			case -16: /* "remap63" */
+				drive->sect0 = 63;
+				goto done;
 			case 3: /* cyl,head,sect */
 				drive->media	= ide_disk;
 				drive->cyl	= drive->bios_cyl  = vals[0];
diff -u --recursive --new-file -X /linux/dontdiff a/fs/partitions/msdos.c b/fs/partitions/msdos.c
--- a/fs/partitions/msdos.c	Mon Feb 24 23:02:56 2003
+++ b/fs/partitions/msdos.c	Wed Apr 16 14:46:25 2003
@@ -20,27 +20,15 @@
  */
 
 #include <linux/config.h>
-#include <linux/buffer_head.h>		/* for invalidate_bdev() */
-
-#ifdef CONFIG_BLK_DEV_IDE
-#include <linux/hdreg.h>
-#include <linux/ide.h>	/* IDE xlate */
-#elif defined(CONFIG_BLK_DEV_IDE_MODULE)
-#include <linux/module.h>
-
-int (*ide_xlate_1024_hook)(struct block_device *, int, int, const char *);
-EXPORT_SYMBOL(ide_xlate_1024_hook);
-#define ide_xlate_1024 ide_xlate_1024_hook
-#endif
 
 #include "check.h"
 #include "msdos.h"
 #include "efi.h"
 
 /*
- * Many architectures don't like unaligned accesses, which is
- * frequently the case with the nr_sects and start_sect partition
- * table entries.
+ * Many architectures don't like unaligned accesses, while
+ * the nr_sects and start_sect partition table entries are
+ * at a 2 (mod 4) address.
  */
 #include <asm/unaligned.h>
 
diff -u --recursive --new-file -X /linux/dontdiff a/include/linux/ide.h b/include/linux/ide.h
--- a/include/linux/ide.h	Tue Mar 25 04:54:45 2003
+++ b/include/linux/ide.h	Wed Apr 16 14:46:25 2003
@@ -1317,12 +1317,6 @@
 extern int ide_wait_stat(ide_startstop_t *, ide_drive_t *, u8, u8, unsigned long);
 
 /*
- * This routine is called from the partition-table code in genhd.c
- * to "convert" a drive to a logical geometry with fewer than 1024 cyls.
- */
-extern int ide_xlate_1024(struct block_device *, int, int, const char *);
-
-/*
  * Return the current idea about the total capacity of this drive.
  */
 extern unsigned long current_capacity (ide_drive_t *drive);