Skip to contentSkip to conversation

ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

PATCH In review Erik Andersen 5 participants 9 messages Aug 2, 2003 – Aug 3, 2003

lore
Share & tools
lore

Erik Andersen

author
#0 lore raw

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/[email protected]?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--

drivers/ide/ide-disk.c renamed from drivers/ide/ide-disk.c.orig +24 -24
@@ -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 */
 	}
#1 lore raw
Quoted 4 lines · Erik Andersen

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.

Quoted 2 lines

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?

Quoted 1 line

#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?

Review

Quoted 1 line
(quoted diff) +1 -0
+	    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

#2 lore raw
Quoted 2 lines · Andries Brouwer

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

#3 lore raw
Quoted 7 lines · Bartlomiej Zolnierkiewicz
Quoted 2 lines · Andries Brouwer

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.]

#4 lore raw
Quoted 6 lines · Andries Brouwer

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.

Quoted 3 lines

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

author
#5 lore raw

PATCH 1/1 ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

Quoted 10 lines · Alan Cox
Quoted 6 lines · Andries Brouwer

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--

drivers/ide/ide-disk.c renamed from drivers/ide/ide-disk.c.orig +87 -72
@@ -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);
+		}
 	}
 }
drivers/ide/ide-disk.c renamed from drivers/ide/ide-disk.c.orig +87 -72

188 lines not on this page · raw message

#6 lore raw
Quoted 2 lines · Erik Andersen

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

author
#7 lore raw
Quoted 6 lines · Andries Brouwer
Quoted 2 lines · Erik Andersen

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

Yes, nice cleanup.

Thanks. :-)

Quoted 8 lines

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.

Quoted 1 line

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.

Quoted 2 lines

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--

#8 lore raw
In reply to Erik Andersen
Quoted 35 lines · Erik Andersen
Quoted 10 lines · Alan Cox
Quoted 6 lines · Andries Brouwer

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).

Quoted 2 lines

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

Very nice Erik, looks good!

Signature · 1 line
Jens Axboe