ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

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

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

_

ide: fix CONFIG_IDEDISK_STROKE support in ide-disk.c

Original patch from Erik Andersen <[email protected]>:
- 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(-)

_

I like your improvements. The only concern I have is you retained my use of do_div() intact. That needs to change since it turns out that do_div() directly modifies the numerator (violating the principle of least surprise). See the recent thread on "do_div considered harmful". In that thread, Andries mentions he has made a generic sectors_to_MB() function to consolidate such things. which I think is a very good idea since then this only has to be correct once. That would also let you eliminate the somewhat ugly and not particularly obvious division plus rounding (X - X/625 + 974)/1950 sequence from idedisk_check_hpa_lba28(),

-Erik

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

Oh yes... I forgot about do_div() and sectors_to_MB(). I will correct this before submitting to Linus.

Thanks,
--
Bartlomiej

Ha - and you didnt even tell me you had this patch out.

Looks good.
You forgot to correct the do_div.
The part I most object to are things like

# ifdef __BIG_ENDIAN
        int i;
        u16 *shortcast;

        shortcast = (u16 *) id;
        for (i = 0; i < (sizeof(struct hd_driveid) / 2); i++)
                shortcast[i] = __le16_to_cpu(shortcast[i]);
# else
#  error "Please fix <asm/byteorder.h>"
# endif
#endif
}
--------------------------------------------------------------

:-)

Yep, Erik noticed this already.

What problems? This reflects real change in drive's identify and I think should be replaced by rereading drive->id from a drive.

I can't spot place when we get info from a BIOS.
If we get it from a user, user should know what she/he is doing.

We can deal with HPA before our geometry info is set (by moving code that is in idedisk_setup() right before call to init_idedisk_capacity()).

User gets correct report from HDIO_GET_IDENTIFY as drive's identify was really changed. Moreover HDIO_GET_IDENTIFY needs fixing to actually reread drive->id from a drive (similarly like /proc identify was fixed).

Even if disk's info changes? I don't think so.
--
Bartlomiej