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

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

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

Signed-off-by: Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]>

   No cookie this time. :-)

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

   I thought it all started with web. 8-)

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

[...]

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

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

[...]

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

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

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.

Signed-off-by: Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]>

Acked-by: Sergei Shtylyov <[EMAIL PROTECTED]>

MBR, Sergei
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html