Skip to contentSkip to conversation

ide-disk: merge LBA28 and LBA48 Host Protected Area support code

PATCH In review Bartlomiej Zolnierkiewicz 2 participants 4 messages Oct 8, 2007 – Oct 25, 2007 libata

lore
Share & tools
lore
#0 lore raw

PATCH 7/12 ide-disk: merge LBA28 and LBA48 Host Protected Area support code

* Merge idedisk_{read_native,set}_max_address_ext() into idedisk_{read_native,set}_max_address().

There should be no functionality changes caused by this patch.

---
 drivers/ide/ide-disk.c |  102 +++++++++++++++----------------------------------
 1 file changed, 32 insertions(+), 70 deletions(-)
drivers/ide/ide-disk.c +32 -70
Index: b/drivers/ide/ide-disk.c
===================================================================
@@ -333,16 +333,19 @@ static ide_startstop_t ide_do_rw_disk (i
  * Queries for true maximum capacity of the drive.
  * Returns maximum LBA address (> 0) of the drive, 0 if failed.
  */
-static unsigned long idedisk_read_native_max_address(ide_drive_t *drive)
+static u64 idedisk_read_native_max_address(ide_drive_t *drive, int lba48)
 {
 	ide_task_t args;
 	struct ide_taskfile *tf = &args.tf;
-	unsigned long addr = 0;
+	u64 addr = 0;
 
 	/* Create IDE/ATA command request structure */
 	memset(&args, 0, sizeof(ide_task_t));
+	if (lba48)
+		tf->command = WIN_READ_NATIVE_MAX_EXT;
+	else
+		tf->command = WIN_READ_NATIVE_MAX;
 	tf->device  = ATA_LBA;
-	tf->command = WIN_READ_NATIVE_MAX;
 	args.command_type			= IDE_DRIVE_TASK_NO_DATA;
 	args.handler				= &task_no_data_intr;
 	/* submit command request */
@@ -350,31 +353,12 @@ static unsigned long idedisk_read_native
 
 	/* if OK, compute maximum address value */
 	if ((tf->command & 0x01) == 0) {
-		addr = ((tf->device & 0xf) << 24) |
-		       (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
-		addr++;	/* since the return value is (maxlba - 1), we add 1 */
-	}
-	return addr;
-}
-
-static unsigned long long idedisk_read_native_max_address_ext(ide_drive_t *drive)
-{
-	ide_task_t args;
-	struct ide_taskfile *tf = &args.tf;
-	unsigned long long addr = 0;
-
-	/* Create IDE/ATA command request structure */
-	memset(&args, 0, sizeof(ide_task_t));
-	tf->device  = ATA_LBA;
-	tf->command = WIN_READ_NATIVE_MAX_EXT;
-	args.command_type			= IDE_DRIVE_TASK_NO_DATA;
-	args.handler				= &task_no_data_intr;
-        /* submit command request */
-        ide_raw_taskfile(drive, &args, NULL);
-
-	/* if OK, compute maximum address value */
-	if ((tf->command & 0x01) == 0) {
 		u32 high, low;
+		if (lba48)
+			high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) |
+				tf->hob_lbal;
+		else
+			high = tf->device & 0xf;
 		high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) | tf->hob_lbal;
 		low  = (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
 		addr = ((__u64)high << 24) | low;
@@ -387,38 +371,11 @@ static unsigned long long idedisk_read_n
  * Sets maximum virtual LBA address of the drive.
  * Returns new maximum virtual LBA address (> 0) or 0 on failure.
  */
-static unsigned long idedisk_set_max_address(ide_drive_t *drive, unsigned long addr_req)
-{
-	ide_task_t args;
-	struct ide_taskfile *tf = &args.tf;
-	unsigned long addr_set = 0;
-	
-	addr_req--;
-	/* Create IDE/ATA command request structure */
-	memset(&args, 0, sizeof(ide_task_t));
-	tf->lbal    =  (addr_req >>  0) & 0xff;
-	tf->lbam    =  (addr_req >>  8) & 0xff;
-	tf->lbah    =  (addr_req >> 16) & 0xff;
-	tf->device  = ((addr_req >> 24) & 0x0f) | ATA_LBA;
-	tf->command = WIN_SET_MAX;
-	args.command_type			= IDE_DRIVE_TASK_NO_DATA;
-	args.handler				= &task_no_data_intr;
-	/* submit command request */
-	ide_raw_taskfile(drive, &args, NULL);
-	/* if OK, read new maximum address value */
-	if ((tf->command & 0x01) == 0) {
-		addr_set = ((tf->device & 0xf) << 24) |
-		           (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
-		addr_set++;
-	}
-	return addr_set;
-}
-
-static unsigned long long idedisk_set_max_address_ext(ide_drive_t *drive, unsigned long long addr_req)
+static u64 idedisk_set_max_address(ide_drive_t *drive, u64 addr_req, int lba48)
 {
 	ide_task_t args;
 	struct ide_taskfile *tf = &args.tf;
-	unsigned long long addr_set = 0;
+	u64 addr_set = 0;
 
 	addr_req--;
 	/* Create IDE/ATA command request structure */
@@ -426,11 +383,16 @@ static unsigned long long idedisk_set_ma
 	tf->lbal     = (addr_req >>  0) & 0xff;
 	tf->lbam     = (addr_req >>= 8) & 0xff;
 	tf->lbah     = (addr_req >>= 8) & 0xff;
-	tf->device   = ATA_LBA;
-	tf->command  = WIN_SET_MAX_EXT;
-	tf->hob_lbal = (addr_req >>= 8) & 0xff;
-	tf->hob_lbam = (addr_req >>= 8) & 0xff;
-	tf->hob_lbah = (addr_req >>= 8) & 0xff;
+	if (lba48) {
+		tf->hob_lbal = (addr_req >>= 8) & 0xff;
+		tf->hob_lbam = (addr_req >>= 8) & 0xff;
+		tf->hob_lbah = (addr_req >>= 8) & 0xff;
+		tf->command  = WIN_SET_MAX_EXT;
+	} else {
+		tf->device   = (addr_req >>= 8) & 0x0f;
+		tf->command  = WIN_SET_MAX;
+	}
+	tf->device |= ATA_LBA;
 	args.command_type			= IDE_DRIVE_TASK_NO_DATA;
 	args.handler				= &task_no_data_intr;
 	/* submit command request */
@@ -438,7 +400,11 @@ static unsigned long long idedisk_set_ma
 	/* if OK, compute maximum address value */
 	if ((tf->command & 0x01) == 0) {
 		u32 high, low;
-		high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) | tf->hob_lbal;
+		if (lba48)
+			high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) |
+				tf->hob_lbal;
+		else
+			high = tf->device & 0xf;
 		low  = (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
 		addr_set = ((__u64)high << 24) | low;
 		addr_set++;
@@ -488,10 +454,8 @@ static void idedisk_check_hpa(ide_drive_
 	int lba48 = idedisk_supports_lba48(drive->id);
 
 	capacity = drive->capacity64;
-	if (lba48)
-		set_max = idedisk_read_native_max_address_ext(drive);
-	else
-		set_max = idedisk_read_native_max_address(drive);
+
+	set_max = idedisk_read_native_max_address(drive, lba48);
 
 	if (ide_in_drive_list(drive->id, hpa_list)) {
 		/*
@@ -512,10 +476,8 @@ static void idedisk_check_hpa(ide_drive_
 			 capacity, sectors_to_MB(capacity),
 			 set_max, sectors_to_MB(set_max));
 
-	if (lba48)
-		set_max = idedisk_set_max_address_ext(drive, set_max);
-	else
-		set_max = idedisk_set_max_address(drive, set_max);
+	set_max = idedisk_set_max_address(drive, set_max, lba48);
+
 	if (set_max) {
 		drive->capacity64 = set_max;
 		printk(KERN_INFO "%s: Host Protected Area disabled.\n",
#1 lore raw

Hello.

Quoted 4 lines · Bartlomiej Zolnierkiewicz

* Merge idedisk_{read_native,set}_max_address_ext() into idedisk_{read_native,set}_max_address().

There should be no functionality changes caused by this patch.

This is unfortunately not achieved...

Quoted 1 line

No cookie this time. :-)

Quoted 4 lines

Index: b/drivers/ide/ide-disk.c
===================================================================

drivers/ide/ide-disk.c +0 -0

[...]

drivers/ide/ide-disk.c static unsigned long idedisk_read_native · line 355

Quoted 7 lines
drivers/ide/ide-disk.c +0 -3
@@ -350,31 +353,12 @@ static unsigned long idedisk_read_native
 
 	/* if OK, compute maximum address value */
 	if ((tf->command & 0x01) == 0) {
-		addr = ((tf->device & 0xf) << 24) |
-		       (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
-		addr++;	/* since the return value is (maxlba - 1), we add 1 */

Actually, the return value is just maxlba (which is *not* the same as
capacity).

Quoted 12 lines
drivers/ide/ide-disk.c +0 -12
-	}
-	return addr;
-}
-
-static unsigned long long idedisk_read_native_max_address_ext(ide_drive_t *drive)
-{
-	ide_task_t args;
-	struct ide_taskfile *tf = &args.tf;
-	unsigned long long addr = 0;
-
-	/* Create IDE/ATA command request structure */
-	memset(&args, 0, sizeof(ide_task_t));

drivers/ide/ide-disk.c static unsigned long idedisk_read_native · line 356

Quoted 10 lines
drivers/ide/ide-disk.c +0 -9
-	tf->device  = ATA_LBA;
-	tf->command = WIN_READ_NATIVE_MAX_EXT;
-	args.command_type			= IDE_DRIVE_TASK_NO_DATA;
-	args.handler				= &task_no_data_intr;
-        /* submit command request */
-        ide_raw_taskfile(drive, &args, NULL);
-
-	/* if OK, compute maximum address value */
-	if ((tf->command & 0x01) == 0) {
 		u32 high, low;

No newline after declaration block. Well, I see that it's been this way
before the patch but it doesn't have to when this code has been already touched...

drivers/ide/ide-disk.c static unsigned long idedisk_read_native · line 362

Quoted 6 lines
drivers/ide/ide-disk.c +5 -0
+		if (lba48)
+			high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) |
+				tf->hob_lbal;
+		else
+			high = tf->device & 0xf;
 		high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) | tf->hob_lbal;

Wait, you've just properly calculated 'high'! And now LBA28 variant is
borken. :-|

Quoted 3 lines
drivers/ide/ide-disk.c +0 -0
 		low  = (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
 		addr = ((__u64)high << 24) | low;
@@ -387,38 +371,11 @@ static unsigned long long idedisk_read_n

[...]

drivers/ide/ide-disk.c static unsigned long long idedisk_set_ma · line 402

Quoted 4 lines
drivers/ide/ide-disk.c +0 -0
@@ -438,7 +400,11 @@ static unsigned long long idedisk_set_ma
 	/* if OK, compute maximum address value */
 	if ((tf->command & 0x01) == 0) {
 		u32 high, low;

Again no newline after declaration...

drivers/ide/ide-disk.c static unsigned long long idedisk_set_ma · line 410

Quoted 9 lines
drivers/ide/ide-disk.c +5 -1
-		high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) | tf->hob_lbal;
+		if (lba48)
+			high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) |
+				tf->hob_lbal;
+		else
+			high = tf->device & 0xf;
 		low  = (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
 		addr_set = ((__u64)high << 24) | low;
 		addr_set++;

BTW, haven't you noticed that LBA readback code is duplicate? It just asks
to be put into a separate function, like:

static u64 ide_read_lba(struct ide_taskfile *tf)

{
	if (!(tf->command & 0x01)) {
		u32 high, low;

		if (lba48)
			high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) |
				tf->hob_lbal;
		else
			high = tf->device & 0xf;
		low  = (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
		return (((__u64)high << 24) | low) + 1;
	} else
		return 0;
}

MBR, Sergei

#2 lore raw
Quoted 7 lines · Sergei Shtylyov

Hello.

Quoted 2 lines · Bartlomiej Zolnierkiewicz

* Merge idedisk_{read_native,set}_max_address_ext() into idedisk_{read_native,set}_max_address().

Quoted 7 lines
Quoted 1 line

There should be no functionality changes caused by this patch.

This is unfortunately not achieved...

Quoted 1 line

No cookie this time. :-)

"Oh, cookie cookie cookie starts with C!" :)

Quoted 7 lines
Quoted 4 lines

Index: b/drivers/ide/ide-disk.c
===================================================================

drivers/ide/ide-disk.c +0 -0

[...]

Quoted 2 lines
drivers/ide/ide-disk.c +0 -0
@@ -350,31 +353,12 @@ static unsigned long idedisk_read_native
 
Quoted 8 lines

drivers/ide/ide-disk.c static unsigned long idedisk_read_native · line 355

Quoted 5 lines
drivers/ide/ide-disk.c +0 -3
 	/* if OK, compute maximum address value */
 	if ((tf->command & 0x01) == 0) {
-		addr = ((tf->device & 0xf) << 24) |
-		       (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
-		addr++;	/* since the return value is (maxlba - 1), we add 1 */

Actually, the return value is just maxlba (which is *not* the same as
capacity).

Indeed, moreover idedisk_{read_native,set}_max_address() comments look also incorrect, ditto for the function names.

Probably if we handle 'addr++'/'addr_req--'/'addr_set++' in idedisk_check_hpa() all issues will fix themselves.

Care to look into it?

[...]

Quoted 9 lines

drivers/ide/ide-disk.c static unsigned long idedisk_read_native · line 361

Quoted 6 lines
drivers/ide/ide-disk.c +5 -0
+		if (lba48)
+			high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) |
+				tf->hob_lbal;
+		else
+			high = tf->device & 0xf;
 		high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) | tf->hob_lbal;

Wait, you've just properly calculated 'high'! And now LBA28 variant is
borken. :-|

I can swear that it was OK but I remember that I few times needed fix the rejects in this patch because other patches in the series were changed or reordered...

just another brown paper bag bug...

Quoted 14 lines

drivers/ide/ide-disk.c static unsigned long idedisk_read_native

Quoted 9 lines
drivers/ide/ide-disk.c +5 -1
-		high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) | tf->hob_lbal;
+		if (lba48)
+			high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) |
+				tf->hob_lbal;
+		else
+			high = tf->device & 0xf;
 		low  = (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
 		addr_set = ((__u64)high << 24) | low;
 		addr_set++;

BTW, haven't you noticed that LBA readback code is duplicate? It just asks
to be put into a separate function, like:

static u64 ide_read_lba(struct ide_taskfile *tf)

Quoted 14 lines
{
	if (!(tf->command & 0x01)) {
		u32 high, low;

		if (lba48)
			high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) |
				tf->hob_lbal;
		else
			high = tf->device & 0xf;
		low  = (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
		return (((__u64)high << 24) | low) + 1;
	} else
		return 0;
}

"back in the days" I even had a patch doing something similar but since it is still lost somewhere in the TODO folder + requires update for the recent changes (which makes it useless a starting point) it would be great if you could cook a patch adding the above helper.

Thanks for review (especially for catching problem with 'high' variable), revised patch:

[PATCH] ide-disk: merge LBA28 and LBA48 Host Protected Area support code (take 2)

* Merge idedisk_{read_native,set}_max_address_ext() into idedisk_{read_native,set}_max_address().

v2:
* Remove LBA48 code leftover from idedisk_read_native_max_address() ('high' variable initialization). (Noticed by Sergei).

There should be no functionality changes caused by this patch.

---
 drivers/ide/ide-disk.c |  103 +++++++++++++++----------------------------------
 1 file changed, 32 insertions(+), 71 deletions(-)
drivers/ide/ide-disk.c +32 -71
Index: b/drivers/ide/ide-disk.c
===================================================================
@@ -333,16 +333,19 @@ static ide_startstop_t ide_do_rw_disk (i
  * Queries for true maximum capacity of the drive.
  * Returns maximum LBA address (> 0) of the drive, 0 if failed.
  */
-static unsigned long idedisk_read_native_max_address(ide_drive_t *drive)
+static u64 idedisk_read_native_max_address(ide_drive_t *drive, int lba48)
 {
 	ide_task_t args;
 	struct ide_taskfile *tf = &args.tf;
-	unsigned long addr = 0;
+	u64 addr = 0;
 
 	/* Create IDE/ATA command request structure */
 	memset(&args, 0, sizeof(ide_task_t));
+	if (lba48)
+		tf->command = WIN_READ_NATIVE_MAX_EXT;
+	else
+		tf->command = WIN_READ_NATIVE_MAX;
 	tf->device  = ATA_LBA;
-	tf->command = WIN_READ_NATIVE_MAX;
 	args.command_type			= IDE_DRIVE_TASK_NO_DATA;
 	args.handler				= &task_no_data_intr;
 	/* submit command request */
@@ -350,33 +353,13 @@ static unsigned long idedisk_read_native
 
 	/* if OK, compute maximum address value */
 	if ((tf->status & 0x01) == 0) {
-		addr = ((tf->device & 0xf) << 24) |
-		       (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
-		addr++;	/* since the return value is (maxlba - 1), we add 1 */
-	}
-	return addr;
-}
-
-static unsigned long long idedisk_read_native_max_address_ext(ide_drive_t *drive)
-{
-	ide_task_t args;
-	struct ide_taskfile *tf = &args.tf;
-	unsigned long long addr = 0;
-
-	/* Create IDE/ATA command request structure */
-	memset(&args, 0, sizeof(ide_task_t));
-	tf->device  = ATA_LBA;
-	tf->command = WIN_READ_NATIVE_MAX_EXT;
-	args.command_type			= IDE_DRIVE_TASK_NO_DATA;
-	args.handler				= &task_no_data_intr;
-        /* submit command request */
-        ide_raw_taskfile(drive, &args, NULL);
-
-	/* if OK, compute maximum address value */
-	if ((tf->status & 0x01) == 0) {
 		u32 high, low;
 
-		high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) | tf->hob_lbal;
+		if (lba48)
+			high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) |
+				tf->hob_lbal;
+		else
+			high = tf->device & 0xf;
 		low  = (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
 		addr = ((__u64)high << 24) | low;
 		addr++;	/* since the return value is (maxlba - 1), we add 1 */
@@ -388,38 +371,11 @@ static unsigned long long idedisk_read_n
  * Sets maximum virtual LBA address of the drive.
  * Returns new maximum virtual LBA address (> 0) or 0 on failure.
  */
-static unsigned long idedisk_set_max_address(ide_drive_t *drive, unsigned long addr_req)
+static u64 idedisk_set_max_address(ide_drive_t *drive, u64 addr_req, int lba48)
 {
 	ide_task_t args;
 	struct ide_taskfile *tf = &args.tf;
-	unsigned long addr_set = 0;
-	
-	addr_req--;
-	/* Create IDE/ATA command request structure */
-	memset(&args, 0, sizeof(ide_task_t));
-	tf->lbal    =  (addr_req >>  0) & 0xff;
-	tf->lbam    =  (addr_req >>  8) & 0xff;
-	tf->lbah    =  (addr_req >> 16) & 0xff;
-	tf->device  = ((addr_req >> 24) & 0x0f) | ATA_LBA;
-	tf->command = WIN_SET_MAX;
-	args.command_type			= IDE_DRIVE_TASK_NO_DATA;
-	args.handler				= &task_no_data_intr;
-	/* submit command request */
-	ide_raw_taskfile(drive, &args, NULL);
-	/* if OK, read new maximum address value */
-	if ((tf->status & 0x01) == 0) {
-		addr_set = ((tf->device & 0xf) << 24) |
-			   (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
-		addr_set++;
-	}
-	return addr_set;
-}
-
-static unsigned long long idedisk_set_max_address_ext(ide_drive_t *drive, unsigned long long addr_req)
-{
-	ide_task_t args;
-	struct ide_taskfile *tf = &args.tf;
-	unsigned long long addr_set = 0;
+	u64 addr_set = 0;
 
 	addr_req--;
 	/* Create IDE/ATA command request structure */
@@ -427,11 +383,16 @@ static unsigned long long idedisk_set_ma
 	tf->lbal     = (addr_req >>  0) & 0xff;
 	tf->lbam     = (addr_req >>= 8) & 0xff;
 	tf->lbah     = (addr_req >>= 8) & 0xff;
-	tf->device   = ATA_LBA;
-	tf->command  = WIN_SET_MAX_EXT;
-	tf->hob_lbal = (addr_req >>= 8) & 0xff;
-	tf->hob_lbam = (addr_req >>= 8) & 0xff;
-	tf->hob_lbah = (addr_req >>= 8) & 0xff;
+	if (lba48) {
+		tf->hob_lbal = (addr_req >>= 8) & 0xff;
+		tf->hob_lbam = (addr_req >>= 8) & 0xff;
+		tf->hob_lbah = (addr_req >>= 8) & 0xff;
+		tf->command  = WIN_SET_MAX_EXT;
+	} else {
+		tf->device   = (addr_req >>= 8) & 0x0f;
+		tf->command  = WIN_SET_MAX;
+	}
+	tf->device |= ATA_LBA;
 	args.command_type			= IDE_DRIVE_TASK_NO_DATA;
 	args.handler				= &task_no_data_intr;
 	/* submit command request */
@@ -440,7 +401,11 @@ static unsigned long long idedisk_set_ma
 	if ((tf->status & 0x01) == 0) {
 		u32 high, low;
 
-		high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) | tf->hob_lbal;
+		if (lba48)
+			high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) |
+				tf->hob_lbal;
+		else
+			high = tf->device & 0xf;
 		low  = (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
 		addr_set = ((__u64)high << 24) | low;
 		addr_set++;
@@ -490,10 +455,8 @@ static void idedisk_check_hpa(ide_drive_
 	int lba48 = idedisk_supports_lba48(drive->id);
 
 	capacity = drive->capacity64;
-	if (lba48)
-		set_max = idedisk_read_native_max_address_ext(drive);
-	else
-		set_max = idedisk_read_native_max_address(drive);
+
+	set_max = idedisk_read_native_max_address(drive, lba48);
 
 	if (ide_in_drive_list(drive->id, hpa_list)) {
 		/*
@@ -514,10 +477,8 @@ static void idedisk_check_hpa(ide_drive_
 			 capacity, sectors_to_MB(capacity),
 			 set_max, sectors_to_MB(set_max));
 
-	if (lba48)
-		set_max = idedisk_set_max_address_ext(drive, set_max);
-	else
-		set_max = idedisk_set_max_address(drive, set_max);
+	set_max = idedisk_set_max_address(drive, set_max, lba48);
+
 	if (set_max) {
 		drive->capacity64 = set_max;
 		printk(KERN_INFO "%s: Host Protected Area disabled.\n",
#3 lore raw

Hello.

Quoted 5 lines · Bartlomiej Zolnierkiewicz
Quoted 4 lines
Quoted 4 lines

* Merge idedisk_{read_native,set}_max_address_ext() into idedisk_{read_native,set}_max_address().

There should be no functionality changes caused by this patch.

Quoted 7 lines
Quoted 5 lines

This is unfortunately not achieved...

Quoted 1 line

No cookie this time. :-)

"Oh, cookie cookie cookie starts with C!" :)

I thought it all started with web. 8-)

Quoted 18 lines
Quoted 9 lines
Quoted 4 lines

Index: b/drivers/ide/ide-disk.c
===================================================================

drivers/ide/ide-disk.c +0 -0

[...]

Quoted 2 lines
drivers/ide/ide-disk.c +0 -0
@@ -350,31 +353,12 @@ static unsigned long idedisk_read_native
 
Quoted 8 lines

drivers/ide/ide-disk.c static unsigned long idedisk_read_native · line 355

Quoted 5 lines
drivers/ide/ide-disk.c +0 -3
 	/* if OK, compute maximum address value */
 	if ((tf->command & 0x01) == 0) {
-		addr = ((tf->device & 0xf) << 24) |
-		       (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
-		addr++;	/* since the return value is (maxlba - 1), we add 1 */

Actually, the return value is just maxlba (which is *not* the same as
capacity).

Quoted 7 lines

Indeed, moreover idedisk_{read_native,set}_max_address() comments look also incorrect, ditto for the function names.

Probably if we handle 'addr++'/'addr_req--'/'addr_set++' in idedisk_check_hpa() all issues will fix themselves.

Care to look into it?

Still no time...

Quoted 31 lines

[...]

Quoted 14 lines
Quoted 9 lines
drivers/ide/ide-disk.c +5 -1
-		high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) | tf->hob_lbal;
+		if (lba48)
+			high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) |
+				tf->hob_lbal;
+		else
+			high = tf->device & 0xf;
		low  = (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
		addr_set = ((__u64)high << 24) | low;
		addr_set++;

BTW, haven't you noticed that LBA readback code is duplicate? It just asks
to be put into a separate function, like:

static u64 ide_read_lba(struct ide_taskfile *tf)

Quoted 14 lines
{
	if (!(tf->command & 0x01)) {
		u32 high, low;

		if (lba48)
			high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) |
				tf->hob_lbal;
		else
			high = tf->device & 0xf;
		low  = (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
		return (((__u64)high << 24) | low) + 1;
	} else
		return 0;
}
Quoted 4 lines

"back in the days" I even had a patch doing something similar but since it is still lost somewhere in the TODO folder + requires update for the recent changes (which makes it useless a starting point) it would be great if you could cook a patch adding the above helper.

I'll think about it... :-)

Quoted 8 lines

Thanks for review (especially for catching problem with 'high' variable), revised patch:

[PATCH] ide-disk: merge LBA28 and LBA48 Host Protected Area support code (take 2)

* Merge idedisk_{read_native,set}_max_address_ext() into idedisk_{read_native,set}_max_address().

Quoted 7 lines

v2:
* Remove LBA48 code leftover from idedisk_read_native_max_address() ('high' variable initialization). (Noticed by Sergei).

There should be no functionality changes caused by this patch.

MBR, Sergei