ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

[Erik Andersen]

About 5 months ago, a patch to ide-disk.c was applied creating
revision 1.13 with the comment "PATCH: resync IDE with -ac".

    http://linux.bkbits.net:8080/linux-2.4/diffs/drivers/ide/ide-disk.c@1.13?nav=index.html|src/|src/drivers|src/drivers/ide|hist/drivers/ide/ide-disk.c

The linked patch added the IDE_STROKE_LIMIT macro and uses this
macro in a couple of tests.  Anybody know what the intent of this
IDE_STROKE_LIMIT macro is?  I ask since it completely breaks
CONFIG_IDEDISK_STROKE and I can't see any reason for it being
there.  It looks to me like it just needs to be ripped right back
out.  Am I missing something?

I have created the following patch which makes this option work
as expected once again in 2.4.x.  It also removes the irrelevant
idedisk_supports_host_protected_area() and makes the HPA
detection actually display useful information for a change, i.e.,
when CONFIG_IDEDISK_STROKE is disabled you get something like:

    hdb: Host Protected Area detected.
	    current capacity is 30001 sectors (15 MB)
	    native  capacity is 234441648 sectors (120034 MB)
    hdb: 30001 sectors (15 MB) w/2048KiB Cache, CHS=29/16/63, UDMA(100)

and when CONFIG_IDEDISK_STROKE is enabled you get:

    hdb: Host Protected Area detected.
	    current capacity is 30001 sectors (15 MB)
	    native  capacity is 234441648 sectors (120034 MB)
    hdb: Host Protected Area Disabled
    hdb: 234441648 sectors (120034 MB) w/2048KiB Cache, CHS=232581/16/63, UDMA(100)

Thanks,

 -Erik

--
Erik B. Andersen             http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

--- linux/drivers/ide/ide-disk.c.orig
+++ linux/drivers/ide/ide-disk.c
@@ -69,6 +69,7 @@
 #include <asm/irq.h>
 #include <asm/uaccess.h>
 #include <asm/io.h>
+#include <asm/div64.h>
 
 /* FIXME: some day we shouldnt need to look in here! */
 
@@ -1131,18 +1132,6 @@
 #endif /* CONFIG_IDEDISK_STROKE */
 
 /*
- * Tests if the drive supports Host Protected Area feature.
- * Returns true if supported, false otherwise.
- */
-static inline int idedisk_supports_host_protected_area(ide_drive_t *drive)
-{
-	int flag = (drive->id->cfs_enable_1 & 0x0400) ? 1 : 0;
-	if (flag)
-		printk("%s: host protected area => %d\n", drive->name, flag);
-	return flag;
-}
-
-/*
  * Compute drive->capacity, the full capacity of the drive
  * Called with drive->id != NULL.
  *
@@ -1156,7 +1145,6 @@
  * in above order (i.e., if value of higher priority is available,
  * reset will be ignored).
  */
-#define IDE_STROKE_LIMIT	(32000*1024*2)
 static void init_idedisk_capacity (ide_drive_t  *drive)
 {
 	struct hd_driveid *id = drive->id;
@@ -1168,8 +1156,6 @@
 	drive->capacity48 = 0;
 	drive->select.b.lba = 0;
 
-	(void) idedisk_supports_host_protected_area(drive);
-
 	if (id->cfs_enable_2 & 0x0400) {
 		capacity_2 = id->lba_capacity_2;
 		drive->head		= drive->bios_head = 255;
@@ -1177,7 +1163,19 @@
 		drive->cyl = (unsigned int) capacity_2 / (drive->head * drive->sect);
 		drive->select.b.lba	= 1;
 		set_max_ext = idedisk_read_native_max_address_ext(drive);
-		if (set_max_ext > capacity_2 && capacity_2 > IDE_STROKE_LIMIT) {
+		if (set_max_ext > capacity_2)
+		{
+		    /* Sigh.  We can not just divide unsigned long longs... */
+		    unsigned long long nativeMb, currentMb;
+		    nativeMb = set_max_ext * 512;
+		    currentMb = capacity_2 * 512;
+		    /* do_div is a macro so we can not safely combine steps */
+		    nativeMb = do_div(nativeMb, 1000000);
+		    currentMb = do_div(currentMb, 1000000);
+		    printk(KERN_INFO "%s: Host Protected Area detected.\n"
+			    "    current capacity is %lld sectors (%lld MB)\n"
+			    "    native  capacity is %lld sectors (%lld MB)\n",
+			    drive->name, capacity_2, currentMb, set_max_ext, nativeMb);
 #ifdef CONFIG_IDEDISK_STROKE
 			set_max_ext = idedisk_read_native_max_address_ext(drive);
 			set_max_ext = idedisk_set_max_address_ext(drive, set_max_ext);
@@ -1186,13 +1184,10 @@
 				drive->cyl = (unsigned int) set_max_ext / (drive->head * drive->sect);
 				drive->select.b.lba = 1;
 				drive->id->lba_capacity_2 = capacity_2;
+				printk(KERN_INFO "%s: Host Protected Area Disabled\n", drive->name);
                         }
-#else /* !CONFIG_IDEDISK_STROKE */
-			printk(KERN_INFO "%s: setmax_ext LBA %llu, native  %llu\n",
-				drive->name, set_max_ext, capacity_2);
 #endif /* CONFIG_IDEDISK_STROKE */
 		}
-		drive->cyl = (unsigned int) capacity_2 / (drive->head * drive->sect);
 		drive->bios_cyl		= drive->cyl;
 		drive->capacity48	= capacity_2;
 		drive->capacity		= (unsigned long) capacity_2;
@@ -1204,7 +1199,14 @@
 		drive->select.b.lba = 1;
 	}
 
-	if (set_max > capacity && capacity > IDE_STROKE_LIMIT) {
+	if (set_max > capacity)
+	{
+	    printk(KERN_INFO "%s: Host Protected Area detected.\n"
+		    "    current capacity is %ld sectors (%ld MB)\n"
+		    "    native  capacity is %ld sectors (%ld MB)\n",
+		    drive->name, capacity, 
+		    (capacity - capacity/625 + 974)/1950,
+		    set_max, (set_max - set_max/625 + 974)/1950);
 #ifdef CONFIG_IDEDISK_STROKE
 		set_max = idedisk_read_native_max_address(drive);
 		set_max = idedisk_set_max_address(drive, set_max);
@@ -1213,10 +1215,8 @@
 			drive->cyl = set_max / (drive->head * drive->sect);
 			drive->select.b.lba = 1;
 			drive->id->lba_capacity = capacity;
+			printk(KERN_INFO "%s: Host Protected Area Disabled\n", drive->name);
 		}
-#else /* !CONFIG_IDEDISK_STROKE */
-		printk(KERN_INFO "%s: setmax LBA %lu, native  %lu\n",
-			drive->name, set_max, capacity);
 #endif /* CONFIG_IDEDISK_STROKE */
 	}
 

Re: ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

[Andries Brouwer]

On Sat, Aug 02, 2003 at 02:42:05AM -0600, Erik Andersen wrote:

> Anybody know what the intent of this IDE_STROKE_LIMIT macro is?
> I ask since it completely breaks CONFIG_IDEDISK_STROKE and I can't see
> any reason for it being there.  It looks to me like it just needs to be
> ripped right back out.

I agree entirely.


> I have created the following patch which makes this option work
> as expected once again in 2.4.x.

Will you also submit the corresponding 2.6 patch?


> #define IDE_STROKE_LIMIT	(32000*1024*2)

I can guess where this limit came from.
The most common way people meet this situation is when they have a
large disk, so large that their BIOS faints when seeing it, so that
they have to use a clipping jumper to fake a smaller disk. Such
jumpers usually clip to about 32 GB (namely 66055248 sectors).
So 65536000 is a lower limit to what one sees with clips.

Maybe it is intended to protect against old disks that do not
understand these new commands. Andre? Bart? Alan?


> +	    printk(KERN_INFO "%s: Host Protected Area detected.\n"

This text might lead to some confusion: as just remarked, usually
we have a clipping jumper, not a proper Host Protected Area.


#ifdef CONFIG_IDEDISK_STROKE

I am also unhappy about the fact that this is a configuration option,
one of the zillion we have. A boot parameter would be a better choice.

Does anyone know about disks that get unhappy if we just do this
stuff unconditionally (or, to be more precise, do it when the IDENTIFY
output says we can) ?
Maybe not even a boot parameter is needed and we can do the right thing
automatically.


Andries

Re: ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

[Bartlomiej Zolnierkiewicz]

On Sat, 2 Aug 2003, Andries Brouwer wrote:

> Maybe it is intended to protect against old disks that do not
> understand these new commands. Andre? Bart? Alan?

Some Samsung disks lock up.  Probably we should check if HPA
command set is supported instead of using IDE_STROKE_LIMIT.

--
Bartlomiej

Re: ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

[Andries Brouwer]

On Sat, Aug 02, 2003 at 03:10:43PM +0200, Bartlomiej Zolnierkiewicz wrote:

> On Sat, 2 Aug 2003, Andries Brouwer wrote:
> 
> > Maybe it is intended to protect against old disks that do not
> > understand these new commands. Andre? Bart? Alan?
> 
> Some Samsung disks lock up.  Probably we should check if HPA
> command set is supported instead of using IDE_STROKE_LIMIT.

OK, so we have to investigate. This strange test was inserted
in 2.4 and 2.5 via Alan, and google gives me Alan's changelog:

Linux 2.5.66-ac1
o Don't issue WIN_SET_MAX on older drivers (Jens Axboe)
  (Breaks some Samsung)

So, now the question is to Jens: what was the situation?
What disk, kernel, identify output?

If possible we would like to remove the test and test the
right bits instead. But if that Samsung disk claims it
supports HPA and doesnt..

Andries


[By the way, google also shows examples where this test
breaks a setup, so removing it might be a good idea
under all circumstances. The usual jumper goes from
above 32GB to 32GB, and from below 32GB to 2GB.
There are also examples of the latter kind solved
by STROKE, but no longer by STROKE + faulty test.]

Re: ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

[Alan Cox]

On Sad, 2003-08-02 at 18:42, Andries Brouwer wrote:
> OK, so we have to investigate. This strange test was inserted
> in 2.4 and 2.5 via Alan, and google gives me Alan's changelog:
> 
> Linux 2.5.66-ac1
> o Don't issue WIN_SET_MAX on older drivers (Jens Axboe)
>   (Breaks some Samsung)

Some older Samsung drives don't abort WIN_SET_MAX but the firmware
hangs hence the check.

> If possible we would like to remove the test and test the
> right bits instead. But if that Samsung disk claims it
> supports HPA and doesnt..

That would be better if it is the case

[PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

[Erik Andersen]

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

On Sat Aug 02, 2003 at 10:06:19PM +0100, Alan Cox wrote:
> On Sad, 2003-08-02 at 18:42, Andries Brouwer wrote:
> > OK, so we have to investigate. This strange test was inserted
> > in 2.4 and 2.5 via Alan, and google gives me Alan's changelog:
> > 
> > Linux 2.5.66-ac1
> > o Don't issue WIN_SET_MAX on older drivers (Jens Axboe)
> >   (Breaks some Samsung)
> 
> Some older Samsung drives don't abort WIN_SET_MAX but the firmware
> hangs hence the check.

Ok, I think I can actually test that one.

    <rummages in ye olde box of hardware>

Cool, found it, I have an ancient Samsung SHD-3212A (426MB)
drive that will hopefully show the problem.

    <sound of testing in the distance>

Ok, found the problem.  The current code (in addition to being
badly written) does not even bother to test if the drive supports
the HPA feature set before issuing a WIN_SET_MAX call.  In my
case, it didn't crash my Samsung drive, but it certainly did make
it complain rather loudly.

I have rewritten the init_idedisk_capacity() function and taught
it to behave itself.  It is now much cleaner IMHO, and will only
issues SET_MAX* calls to drives that claim they support such
things.  I've tested this patch with a 200GB drive, a 120GB
drive, an 80GB drive and my ancient Samsung drive and in each
case (48bit LBA, 28bit LBA, 28bit CHS w/o support for HPA), my
new version appears to the Right Thing(tm).

Attached is a patch vs 2.4.22-pre10, and a patch vs 2.6.0-pre2. 
Please apply,

 -Erik

--
Erik B. Andersen             http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

[-- Attachment #2: fixup-ide-hpa-2.4.patch --]
[-- Type: text/plain, Size: 7092 bytes --]

--- linux/drivers/ide/ide-disk.c.orig	2003-08-02 15:58:32.000000000 -0600
+++ linux/drivers/ide/ide-disk.c	2003-08-02 16:54:17.000000000 -0600
@@ -69,6 +69,7 @@
 #include <asm/irq.h>
 #include <asm/uaccess.h>
 #include <asm/io.h>
+#include <asm/div64.h>
 
 /* FIXME: some day we shouldnt need to look in here! */
 
@@ -1131,18 +1132,6 @@
 #endif /* CONFIG_IDEDISK_STROKE */
 
 /*
- * Tests if the drive supports Host Protected Area feature.
- * Returns true if supported, false otherwise.
- */
-static inline int idedisk_supports_host_protected_area(ide_drive_t *drive)
-{
-	int flag = (drive->id->cfs_enable_1 & 0x0400) ? 1 : 0;
-	if (flag)
-		printk("%s: host protected area => %d\n", drive->name, flag);
-	return flag;
-}
-
-/*
  * Compute drive->capacity, the full capacity of the drive
  * Called with drive->id != NULL.
  *
@@ -1156,77 +1145,103 @@
  * in above order (i.e., if value of higher priority is available,
  * reset will be ignored).
  */
-#define IDE_STROKE_LIMIT	(32000*1024*2)
 static void init_idedisk_capacity (ide_drive_t  *drive)
 {
-	struct hd_driveid *id = drive->id;
-	unsigned long capacity = drive->cyl * drive->head * drive->sect;
-	unsigned long set_max = idedisk_read_native_max_address(drive);
-	unsigned long long capacity_2 = capacity;
-	unsigned long long set_max_ext;
-
-	drive->capacity48 = 0;
-	drive->select.b.lba = 0;
-
-	(void) idedisk_supports_host_protected_area(drive);
-
-	if (id->cfs_enable_2 & 0x0400) {
-		capacity_2 = id->lba_capacity_2;
-		drive->head		= drive->bios_head = 255;
-		drive->sect		= drive->bios_sect = 63;
-		drive->cyl = (unsigned int) capacity_2 / (drive->head * drive->sect);
-		drive->select.b.lba	= 1;
-		set_max_ext = idedisk_read_native_max_address_ext(drive);
-		if (set_max_ext > capacity_2 && capacity_2 > IDE_STROKE_LIMIT) {
-#ifdef CONFIG_IDEDISK_STROKE
-			set_max_ext = idedisk_read_native_max_address_ext(drive);
-			set_max_ext = idedisk_set_max_address_ext(drive, set_max_ext);
-			if (set_max_ext) {
-				drive->capacity48 = capacity_2 = set_max_ext;
-				drive->cyl = (unsigned int) set_max_ext / (drive->head * drive->sect);
-				drive->select.b.lba = 1;
-				drive->id->lba_capacity_2 = capacity_2;
-                        }
-#else /* !CONFIG_IDEDISK_STROKE */
-			printk(KERN_INFO "%s: setmax_ext LBA %llu, native  %llu\n",
-				drive->name, set_max_ext, capacity_2);
-#endif /* CONFIG_IDEDISK_STROKE */
-		}
-		drive->cyl = (unsigned int) capacity_2 / (drive->head * drive->sect);
-		drive->bios_cyl		= drive->cyl;
-		drive->capacity48	= capacity_2;
-		drive->capacity		= (unsigned long) capacity_2;
-		return;
-	/* Determine capacity, and use LBA if the drive properly supports it */
+	struct hd_driveid *id;
+	unsigned long capacity;
+	unsigned long long capacity_2;
+
+	id = drive->id;
+	capacity = drive->cyl * drive->head * drive->sect;
+	capacity_2 = capacity;
+
+
+	/* Does this drive support the 48-bit Address Feature Set
+	 * and does it have that enabled at the moment? */
+	if ((id->command_set_2 & 0x0400) && (id->cfs_enable_2 & 0x0400)) {
+		//drive speaks 48-bit LBA
+		drive->capacity = (unsigned long) capacity_2 = id->lba_capacity_2;
+		drive->capacity48 = capacity_2;
+		drive->head = drive->bios_head = 255;
+		drive->sect = drive->bios_sect = 63;
+		drive->cyl  = (unsigned int) capacity_2 / (drive->head * drive->sect);
+		drive->bios_cyl	= drive->cyl;
+		drive->select.b.lba = 1;
 	} else if ((id->capability & 2) && lba_capacity_is_ok(id)) {
-		capacity = id->lba_capacity;
+		// drive speaks 28-bit LBA
+		drive->capacity = capacity = id->lba_capacity;
+		drive->capacity48 = 0;
 		drive->cyl = capacity / (drive->head * drive->sect);
 		drive->select.b.lba = 1;
+	} else {
+		// drive speaks boring old 28-bit CHS
+		drive->capacity = capacity;
+		drive->capacity48 = 0;
+		drive->select.b.lba = 0;
 	}
 
-	if (set_max > capacity && capacity > IDE_STROKE_LIMIT) {
+
+	/* If this drive supports the Host Protected Area feature set, 
+	 * then we may need to change our opinion about the drive's size... */
+	if (id->command_set_1 & 0x0400 && id->cfs_enable_1 & 0x0400) 
+	{
+		/* Does this drive support the 48-bit Address Feature Set
+		 * and does it have that enabled as well? */
+		if ((id->command_set_2 & 0x0400) && (id->cfs_enable_2 & 0x0400)) 
+		{
+			unsigned long long set_max_ext;
+			set_max_ext = idedisk_read_native_max_address_ext(drive);
+
+			/* If our capacity is smaller than the native capacity, 
+			 * we have an HPA created using SET MAX ADDRESS EXT... */
+			if (set_max_ext > capacity_2)
+			{
+				/* Sigh.  We have to jump through extra hoops to 
+				 * divide unsigned long longs within the kernel */
+				unsigned long long nativeMb, currentMb;
+				nativeMb = set_max_ext * 512;
+				currentMb = capacity_2 * 512;
+				/* do_div is a macro so we can not safely combine steps */
+				nativeMb = do_div(nativeMb, 1000000);
+				currentMb = do_div(currentMb, 1000000);
+				printk(KERN_INFO "%s: Host Protected Area detected.\n"
+					"    current capacity is %lld sectors (%lld MB)\n"
+					"    native  capacity is %lld sectors (%lld MB)\n",
+					drive->name, capacity_2, currentMb, set_max_ext, nativeMb);
 #ifdef CONFIG_IDEDISK_STROKE
-		set_max = idedisk_read_native_max_address(drive);
-		set_max = idedisk_set_max_address(drive, set_max);
-		if (set_max) {
-			drive->capacity = capacity = set_max;
-			drive->cyl = set_max / (drive->head * drive->sect);
-			drive->select.b.lba = 1;
-			drive->id->lba_capacity = capacity;
-		}
-#else /* !CONFIG_IDEDISK_STROKE */
-		printk(KERN_INFO "%s: setmax LBA %lu, native  %lu\n",
-			drive->name, set_max, capacity);
+				set_max_ext = idedisk_set_max_address_ext(drive, set_max_ext);
+				if (set_max_ext) {
+					drive->capacity48 = drive->id->lba_capacity_2 = set_max_ext;
+					drive->cyl = (unsigned int) set_max_ext / (drive->head * drive->sect);
+					drive->select.b.lba = 1;
+					printk(KERN_INFO "%s: Host Protected Area Disabled\n", drive->name);
+				}
 #endif /* CONFIG_IDEDISK_STROKE */
-	}
+			}
+		} else {
+			unsigned long set_max;
+			set_max = idedisk_read_native_max_address(drive);
 
-	drive->capacity = capacity;
+			if (set_max > capacity)
+			{
+				printk(KERN_INFO "%s: Host Protected Area detected.\n"
+					"    current capacity is %ld sectors (%ld MB)\n"
+					"    native  capacity is %ld sectors (%ld MB)\n",
+					drive->name, capacity, 
+					(capacity - capacity/625 + 974)/1950,
+					set_max, (set_max - set_max/625 + 974)/1950);
+#ifdef CONFIG_IDEDISK_STROKE
+				set_max = idedisk_set_max_address(drive, set_max);
+				if (set_max) {
+					drive->capacity = drive->id->lba_capacity = set_max;
+					drive->cyl = set_max / (drive->head * drive->sect);
+					drive->select.b.lba = 1;
+					printk(KERN_INFO "%s: Host Protected Area Disabled\n", drive->name);
+				}
+#endif /* CONFIG_IDEDISK_STROKE */
+			}
 
-	if ((id->command_set_2 & 0x0400) && (id->cfs_enable_2 & 0x0400)) {
-		drive->capacity48 = id->lba_capacity_2;
-		drive->head = 255;
-		drive->sect = 63;
-		drive->cyl = (unsigned long)(drive->capacity48) / (drive->head * drive->sect);
+		}
 	}
 }
 

[-- Attachment #3: fixup-ide-hpa-2.6.patch --]
[-- Type: text/plain, Size: 7127 bytes --]

--- linux-2.6.0-test2/drivers/ide/ide-disk.c.orig	2003-07-27 11:01:09.000000000 -0600
+++ linux-2.6.0-test2/drivers/ide/ide-disk.c	2003-08-02 17:17:27.000000000 -0600
@@ -67,6 +67,7 @@
 #include <asm/irq.h>
 #include <asm/uaccess.h>
 #include <asm/io.h>
+#include <asm/div64.h>
 
 /* FIXME: some day we shouldn't need to look in here! */
 
@@ -1066,18 +1067,6 @@
 #endif /* CONFIG_IDEDISK_STROKE */
 
 /*
- * Tests if the drive supports Host Protected Area feature.
- * Returns true if supported, false otherwise.
- */
-static inline int idedisk_supports_host_protected_area(ide_drive_t *drive)
-{
-	int flag = (drive->id->cfs_enable_1 & 0x0400) ? 1 : 0;
-	if (flag)
-		printk(KERN_INFO "%s: host protected area => %d\n", drive->name, flag);
-	return flag;
-}
-
-/*
  * Compute drive->capacity, the full capacity of the drive
  * Called with drive->id != NULL.
  *
@@ -1091,77 +1080,103 @@
  * in above order (i.e., if value of higher priority is available,
  * reset will be ignored).
  */
-#define IDE_STROKE_LIMIT	(32000*1024*2)
 static void init_idedisk_capacity (ide_drive_t  *drive)
 {
-	struct hd_driveid *id = drive->id;
-	unsigned long capacity = drive->cyl * drive->head * drive->sect;
-	unsigned long set_max = idedisk_read_native_max_address(drive);
-	unsigned long long capacity_2 = capacity;
-	unsigned long long set_max_ext;
-
-	drive->capacity48 = 0;
-	drive->select.b.lba = 0;
-
-	(void) idedisk_supports_host_protected_area(drive);
-
-	if (id->cfs_enable_2 & 0x0400) {
-		capacity_2 = id->lba_capacity_2;
-		drive->head		= drive->bios_head = 255;
-		drive->sect		= drive->bios_sect = 63;
-		drive->cyl = (unsigned int) capacity_2 / (drive->head * drive->sect);
-		drive->select.b.lba	= 1;
-		set_max_ext = idedisk_read_native_max_address_ext(drive);
-		if (set_max_ext > capacity_2 && capacity_2 > IDE_STROKE_LIMIT) {
-#ifdef CONFIG_IDEDISK_STROKE
-			set_max_ext = idedisk_read_native_max_address_ext(drive);
-			set_max_ext = idedisk_set_max_address_ext(drive, set_max_ext);
-			if (set_max_ext) {
-				drive->capacity48 = capacity_2 = set_max_ext;
-				drive->cyl = (unsigned int) set_max_ext / (drive->head * drive->sect);
-				drive->select.b.lba = 1;
-				drive->id->lba_capacity_2 = capacity_2;
-                        }
-#else /* !CONFIG_IDEDISK_STROKE */
-			printk(KERN_INFO "%s: setmax_ext LBA %llu, native  %llu\n",
-				drive->name, set_max_ext, capacity_2);
-#endif /* CONFIG_IDEDISK_STROKE */
-		}
-		drive->cyl = (unsigned int) capacity_2 / (drive->head * drive->sect);
-		drive->bios_cyl		= drive->cyl;
-		drive->capacity48	= capacity_2;
-		drive->capacity		= (unsigned long) capacity_2;
-		return;
-	/* Determine capacity, and use LBA if the drive properly supports it */
+	struct hd_driveid *id;
+	unsigned long capacity;
+	unsigned long long capacity_2;
+
+	id = drive->id;
+	capacity = drive->cyl * drive->head * drive->sect;
+	capacity_2 = capacity;
+
+
+	/* Does this drive support the 48-bit Address Feature Set
+	 * and does it have that enabled at the moment? */
+	if ((id->command_set_2 & 0x0400) && (id->cfs_enable_2 & 0x0400)) {
+		//drive speaks 48-bit LBA
+		drive->capacity = (unsigned long) capacity_2 = id->lba_capacity_2;
+		drive->capacity48 = capacity_2;
+		drive->head = drive->bios_head = 255;
+		drive->sect = drive->bios_sect = 63;
+		drive->cyl  = (unsigned int) capacity_2 / (drive->head * drive->sect);
+		drive->bios_cyl	= drive->cyl;
+		drive->select.b.lba = 1;
 	} else if ((id->capability & 2) && lba_capacity_is_ok(id)) {
-		capacity = id->lba_capacity;
+		// drive speaks 28-bit LBA
+		drive->capacity = capacity = id->lba_capacity;
+		drive->capacity48 = 0;
 		drive->cyl = capacity / (drive->head * drive->sect);
 		drive->select.b.lba = 1;
+	} else {
+		// drive speaks boring old 28-bit CHS
+		drive->capacity = capacity;
+		drive->capacity48 = 0;
+		drive->select.b.lba = 0;
 	}
 
-	if (set_max > capacity && capacity > IDE_STROKE_LIMIT) {
+
+	/* If this drive supports the Host Protected Area feature set, 
+	 * then we may need to change our opinion about the drive's size... */
+	if (id->command_set_1 & 0x0400 && id->cfs_enable_1 & 0x0400) 
+	{
+		/* Does this drive support the 48-bit Address Feature Set
+		 * and does it have that enabled as well? */
+		if ((id->command_set_2 & 0x0400) && (id->cfs_enable_2 & 0x0400)) 
+		{
+			unsigned long long set_max_ext;
+			set_max_ext = idedisk_read_native_max_address_ext(drive);
+
+			/* If our capacity is smaller than the native capacity, 
+			 * we have an HPA created using SET MAX ADDRESS EXT... */
+			if (set_max_ext > capacity_2)
+			{
+				/* Sigh.  We have to jump through extra hoops to 
+				 * divide unsigned long longs within the kernel */
+				unsigned long long nativeMb, currentMb;
+				nativeMb = set_max_ext * 512;
+				currentMb = capacity_2 * 512;
+				/* do_div is a macro so we can not safely combine steps */
+				nativeMb = do_div(nativeMb, 1000000);
+				currentMb = do_div(currentMb, 1000000);
+				printk(KERN_INFO "%s: Host Protected Area detected.\n"
+					"    current capacity is %lld sectors (%lld MB)\n"
+					"    native  capacity is %lld sectors (%lld MB)\n",
+					drive->name, capacity_2, currentMb, set_max_ext, nativeMb);
 #ifdef CONFIG_IDEDISK_STROKE
-		set_max = idedisk_read_native_max_address(drive);
-		set_max = idedisk_set_max_address(drive, set_max);
-		if (set_max) {
-			drive->capacity = capacity = set_max;
-			drive->cyl = set_max / (drive->head * drive->sect);
-			drive->select.b.lba = 1;
-			drive->id->lba_capacity = capacity;
-		}
-#else /* !CONFIG_IDEDISK_STROKE */
-		printk(KERN_INFO "%s: setmax LBA %lu, native  %lu\n",
-			drive->name, set_max, capacity);
+				set_max_ext = idedisk_set_max_address_ext(drive, set_max_ext);
+				if (set_max_ext) {
+					drive->capacity48 = drive->id->lba_capacity_2 = set_max_ext;
+					drive->cyl = (unsigned int) set_max_ext / (drive->head * drive->sect);
+					drive->select.b.lba = 1;
+					printk(KERN_INFO "%s: Host Protected Area Disabled\n", drive->name);
+				}
 #endif /* CONFIG_IDEDISK_STROKE */
-	}
+			}
+		} else {
+			unsigned long set_max;
+			set_max = idedisk_read_native_max_address(drive);
 
-	drive->capacity = capacity;
+			if (set_max > capacity)
+			{
+				printk(KERN_INFO "%s: Host Protected Area detected.\n"
+					"    current capacity is %ld sectors (%ld MB)\n"
+					"    native  capacity is %ld sectors (%ld MB)\n",
+					drive->name, capacity, 
+					(capacity - capacity/625 + 974)/1950,
+					set_max, (set_max - set_max/625 + 974)/1950);
+#ifdef CONFIG_IDEDISK_STROKE
+				set_max = idedisk_set_max_address(drive, set_max);
+				if (set_max) {
+					drive->capacity = drive->id->lba_capacity = set_max;
+					drive->cyl = set_max / (drive->head * drive->sect);
+					drive->select.b.lba = 1;
+					printk(KERN_INFO "%s: Host Protected Area Disabled\n", drive->name);
+				}
+#endif /* CONFIG_IDEDISK_STROKE */
+			}
 
-	if ((id->command_set_2 & 0x0400) && (id->cfs_enable_2 & 0x0400)) {
-		drive->capacity48 = id->lba_capacity_2;
-		drive->head = 255;
-		drive->sect = 63;
-		drive->cyl = (unsigned long)(drive->capacity48) / (drive->head * drive->sect);
+		}
 	}
 }
 

Re: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

[Andries Brouwer]

On Sat, Aug 02, 2003 at 05:34:38PM -0600, Erik Andersen wrote:

> I have rewritten the init_idedisk_capacity() function and taught
> it to behave itself.  It is now much cleaner IMHO

Yes, nice cleanup.

Some comments for later - the patch can be applied as it is:

The assignment
	drive->select.b.lba = 0/1;
is done in the first half of init_idedisk_capacity().
I don't think the presence or disabling of HPA has any effect
on b.lba, so there should not be such assignments in the
second, HPA, half.

My standard muttering: id->... should not be modified.

In my source I test drive->head * drive->sect for being nonzero
before dividing.

Andries

Re: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

[Erik Andersen]

On Sun Aug 03, 2003 at 03:26:59AM +0200, Andries Brouwer wrote:
> On Sat, Aug 02, 2003 at 05:34:38PM -0600, Erik Andersen wrote:
> 
> > I have rewritten the init_idedisk_capacity() function and taught
> > it to behave itself.  It is now much cleaner IMHO
> 
> Yes, nice cleanup.

Thanks.  :-)

> Some comments for later - the patch can be applied as it is:
> 
> The assignment
> 	drive->select.b.lba = 0/1;
> is done in the first half of init_idedisk_capacity().
> I don't think the presence or disabling of HPA has any effect
> on b.lba, so there should not be such assignments in the
> second, HPA, half.

Yes, you are right.  This is garbage leftover from the previous
implementation.  It should not change anything, but that would 
be a nice additional cleanup.

> My standard muttering: id->... should not be modified.

Agreed, but that is best left for another patch, since I did not
want to walk through the whole ide stack checking for things that
depends on this behavior.  Hopefully nothing, but you never know.

> In my source I test drive->head * drive->sect for being nonzero
> before dividing.

That would also be a nice additional cleanup.

 -Erik

--
Erik B. Andersen             http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

Re: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

[Jens Axboe]

On Sat, Aug 02 2003, Erik Andersen wrote:
> On Sat Aug 02, 2003 at 10:06:19PM +0100, Alan Cox wrote:
> > On Sad, 2003-08-02 at 18:42, Andries Brouwer wrote:
> > > OK, so we have to investigate. This strange test was inserted
> > > in 2.4 and 2.5 via Alan, and google gives me Alan's changelog:
> > > 
> > > Linux 2.5.66-ac1
> > > o Don't issue WIN_SET_MAX on older drivers (Jens Axboe)
> > >   (Breaks some Samsung)
> > 
> > Some older Samsung drives don't abort WIN_SET_MAX but the firmware
> > hangs hence the check.
> 
> Ok, I think I can actually test that one.
> 
>     <rummages in ye olde box of hardware>
> 
> Cool, found it, I have an ancient Samsung SHD-3212A (426MB)
> drive that will hopefully show the problem.
> 
>     <sound of testing in the distance>
> 
> Ok, found the problem.  The current code (in addition to being
> badly written) does not even bother to test if the drive supports
> the HPA feature set before issuing a WIN_SET_MAX call.  In my
> case, it didn't crash my Samsung drive, but it certainly did make
> it complain rather loudly.
> 
> I have rewritten the init_idedisk_capacity() function and taught
> it to behave itself.  It is now much cleaner IMHO, and will only
> issues SET_MAX* calls to drives that claim they support such
> things.  I've tested this patch with a 200GB drive, a 120GB
> drive, an 80GB drive and my ancient Samsung drive and in each
> case (48bit LBA, 28bit LBA, 28bit CHS w/o support for HPA), my
> new version appears to the Right Thing(tm).
> 
> Attached is a patch vs 2.4.22-pre10, and a patch vs 2.6.0-pre2. 
> Please apply,

Very nice Erik, looks good!

-- 
Jens Axboe