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

[Bartlomiej Zolnierkiewicz <B.Zolnierkiewicz@elka.pw.edu.pl>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 454 bytes --]


On Wed, 6 Aug 2003, Erik Andersen wrote:

> > Can you please resend me the patch once its accepted in 2.6 ?
> >
> > Maybe we want to test it a while in -ac, also?
>
> Ok.  Though it is being incorporated as part of a much larger
> patch in 2.6.  I suppose that will filter back into 2.4 when
> it is ready,

Hi Erik,

I made init_idedisk_capacity() cleanup.
Then I ported your patch and reworked it a bit.

Could you take a look?

Thanks,
--
Bartlomiej

[-- Attachment #2: Type: TEXT/PLAIN, Size: 3973 bytes --]


ide: init_idedisk_capacity() cleanup()

- no need to zero drive->capacity48 and drive->select.b.lba
- don't call idedisk_read_native_max_address_ext() twice
  if drive uses LBA-48 and CONFIG_IDEDISK_STROKE is defined
- remove uneccessary setup of drive->capacity48/cyl/select.b.lba
  if drive uses LBA-48, maximum virtual LBA address needs to be set
  and CONFIG_IDEDISK_STROKE is defined
- set drive->cyl only once if drive uses LBA-48
- don't call idedisk_read_native_max_address() if drive uses LBA-48
  and don't call it twice if CONFIG_IDEDISK_STROKE is defined
- don't check for Host Protected Area if drive uses CHS addressing
  (such combination is not supported by a driver)
- remove duplicated code (at the end of the function) that is never executed

 drivers/ide/ide-disk.c |   32 +++++++++-----------------------
 1 files changed, 9 insertions(+), 23 deletions(-)

diff -puN drivers/ide/ide-disk.c~ide-disk-capacity-init-cleanup drivers/ide/ide-disk.c
--- linux-2.6.0-test2-bk5/drivers/ide/ide-disk.c~ide-disk-capacity-init-cleanup	2003-08-06 02:48:33.000000000 +0200
+++ linux-2.6.0-test2-bk5-root/drivers/ide/ide-disk.c	2003-08-06 16:23:27.000000000 +0200
@@ -1095,13 +1095,10 @@ static inline int idedisk_supports_host_
 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;
+	unsigned long capacity, set_max;
+	unsigned long long capacity_2, set_max_ext;
 
-	drive->capacity48 = 0;
-	drive->select.b.lba = 0;
+	capacity_2 = capacity = drive->cyl * drive->head * drive->sect;
 
 	(void) idedisk_supports_host_protected_area(drive);
 
@@ -1109,19 +1106,13 @@ static void init_idedisk_capacity (ide_d
 		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;
-                        }
+			if (set_max_ext)
+				id->lba_capacity_2 = capacity_2 = set_max_ext;
 #else /* !CONFIG_IDEDISK_STROKE */
 			printk(KERN_INFO "%s: setmax_ext LBA %llu, native  %llu\n",
 				drive->name, set_max_ext, capacity_2);
@@ -1137,11 +1128,14 @@ static void init_idedisk_capacity (ide_d
 		capacity = id->lba_capacity;
 		drive->cyl = capacity / (drive->head * drive->sect);
 		drive->select.b.lba = 1;
+	} else {
+		drive->capacity = capacity;
+		return;
 	}
 
+	set_max = idedisk_read_native_max_address(drive);
 	if (set_max > capacity && capacity > IDE_STROKE_LIMIT) {
 #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;
@@ -1154,15 +1148,7 @@ static void init_idedisk_capacity (ide_d
 			drive->name, set_max, capacity);
 #endif /* CONFIG_IDEDISK_STROKE */
 	}
-
 	drive->capacity = capacity;
-
-	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);
-	}
 }
 
 static unsigned long idedisk_capacity (ide_drive_t *drive)

_

[-- Attachment #3: Type: TEXT/PLAIN, Size: 6668 bytes --]


ide: fix CONFIG_IDEDISK_STROKE support in ide-disk.c

Original patch from Erik Andersen <andersen@codepoet.org>:
- fix CONFIG_IDEDISK_STROKE by adding proper detection of HPA feature set
  and removing IDE_STROKE_LIMIT
- remove irrelevant idedisk_supports_host_protected_area()
- make the HPA detection actually display useful information

I've reworked it a bit:
- detect HPA before calculating drive's geometry
- move HPA detection outside init_idedisk_capacity()
  to idedisk_check_hpa_lba28() and idedisk_check_hpa_lba48()
- respect 80-column limit

 drivers/ide/ide-disk.c |  128 +++++++++++++++++++++++++++++--------------------
 1 files changed, 78 insertions(+), 50 deletions(-)

diff -puN drivers/ide/ide-disk.c~ide-disk-hpa-fixup drivers/ide/ide-disk.c
--- linux-2.6.0-test2-bk5/drivers/ide/ide-disk.c~ide-disk-hpa-fixup	2003-08-06 17:58:02.000000000 +0200
+++ linux-2.6.0-test2-bk5-root/drivers/ide/ide-disk.c	2003-08-06 19:47:52.000000000 +0200
@@ -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! */
 
@@ -1065,16 +1066,64 @@ static unsigned long long idedisk_set_ma
 
 #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)
+static inline void idedisk_check_hpa_lba28(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;
+	unsigned long capacity, set_max;
+
+	capacity = drive->id->lba_capacity;
+	set_max = idedisk_read_native_max_address(drive);
+
+	if (set_max <= capacity)
+		return;
+
+	printk(KERN_INFO "%s: Host Protected Area detected.\n"
+			 "\tcurrent capacity is %ld sectors (%ld MB)\n"
+			 "\tnative  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->id->lba_capacity = set_max;
+		printk(KERN_INFO "%s: Host Protected Area disabled.\n",
+				 drive->name);
+	}
+#endif
+}
+
+static inline void idedisk_check_hpa_lba48(ide_drive_t *drive)
+{
+	unsigned long long capacity_2, set_max_ext, nativeMb, currentMb;
+
+	capacity_2 = drive->id->lba_capacity_2;
+	set_max_ext = idedisk_read_native_max_address_ext(drive);
+
+	if (set_max_ext <= capacity_2)
+		return;
+
+	/*
+	 * Sigh.  We have to jump through extra hoops to
+	 * divide unsigned long longs within the kernel.
+	 */
+	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"
+			 "\tcurrent capacity is %lld sectors (%lld MB)\n"
+			 "\tnative  capacity is %lld sectors (%lld MB)\n",
+			 drive->name,
+			 capacity_2, currentMb, set_max_ext, nativeMb);
+#ifdef CONFIG_IDEDISK_STROKE
+	set_max_ext = idedisk_set_max_address_ext(drive, set_max_ext);
+	if (set_max_ext) {
+		drive->id->lba_capacity_2 = set_max_ext;
+		printk(KERN_INFO "%s: Host Protected Area disabled.\n",
+				 drive->name);
+	}
+#endif
 }
 
 /*
@@ -1091,64 +1140,43 @@ static inline int idedisk_supports_host_
  * 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, set_max;
-	unsigned long long capacity_2, set_max_ext;
-
-	capacity_2 = capacity = drive->cyl * drive->head * drive->sect;
+	/*
+	 * If this drive supports the Host Protected Area feature set,
+	 * then we may need to change our opinion about the drive's capacity.
+	 */
+	int hpa = (id->command_set_1 & 0x0400) && (id->cfs_enable_1 & 0x0400);
 
-	(void) idedisk_supports_host_protected_area(drive);
+	if ((id->command_set_2 & 0x0400) && (id->cfs_enable_2 & 0x0400)) {
+		/* drive speaks 48-bit LBA */
+		unsigned long long capacity_2;
 
-	if (id->cfs_enable_2 & 0x0400) {
+		drive->select.b.lba = 1;
+		if (hpa)
+			idedisk_check_hpa_lba48(drive);
 		capacity_2 = id->lba_capacity_2;
 		drive->head		= drive->bios_head = 255;
 		drive->sect		= drive->bios_sect = 63;
-		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_set_max_address_ext(drive, set_max_ext);
-			if (set_max_ext)
-				id->lba_capacity_2 = capacity_2 = set_max_ext;
-#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 */
 	} else if ((id->capability & 2) && lba_capacity_is_ok(id)) {
+		/* drive speaks 28-bit LBA */
+		unsigned long capacity;
+
+		drive->select.b.lba = 1;
+		if (hpa)
+			idedisk_check_hpa_lba28(drive);
 		capacity = id->lba_capacity;
 		drive->cyl = capacity / (drive->head * drive->sect);
-		drive->select.b.lba = 1;
-	} else {
 		drive->capacity = capacity;
-		return;
-	}
-
-	set_max = idedisk_read_native_max_address(drive);
-	if (set_max > capacity && capacity > IDE_STROKE_LIMIT) {
-#ifdef CONFIG_IDEDISK_STROKE
-		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);
-#endif /* CONFIG_IDEDISK_STROKE */
+	} else {
+		/* drive speaks boring old 28-bit CHS */
+		drive->capacity = drive->cyl * drive->head * drive->sect;
 	}
-	drive->capacity = capacity;
 }
 
 static unsigned long idedisk_capacity (ide_drive_t *drive)

_