Fix use of "void *" for block dev read/write buffer pointers

A couple months ago, a patch was submitted which changed the prototypes for block_write and block_read in the block_dev_desc_t structure (include/part.h). For both, the buffer parameter was changed from a ulong* to a void*. Seven functions were changed as a result of this. Four of those functions take that parameter and pass it to a local variable of a different type, thereby insulating them from this change. The other three were not so lucky..... Using the 2006-06-16 version of common/cmd_ide.c as the reference, lets look at ide_read(). The function begins on line 1230 and a while loop begins on line 1279. At the bottom of this loop, we see
1339 ++blknr;
1340 buffer += ATA_SECTORWORDS;
1341 }
Back when buffer was a ulong*, this pointer addition would result in the pointer moving 0x200 bytes or 0x80 ulongs (ATA_SECTORWORDS is defined as 512/sizeof(unsigned long) in include/ata.h). Now that buffer is a void*, this pointer addition results in the pointer moving by only 0x80 bytes.
In my debugging, I used the diskboot command to read an image into address 0x200000 and tftp to place the same image at 0x400000. When I compared these two locations, I discovered that the data was different starting at address 0x200280. The data at that location was the same as that found at 0x400400. Then 0x200300-0x200380 == 0x400600-0x400680, 0x200380-0x200400 == 0x400800-0x400880 and so on and so on.
The first 0x200 is correct because that is done in a separate call to ide_read that only requests a single block. The second read copied 0x200 bytes to 0x200200, the third read is copied 0x200 bytes to 0x200280, etc.
The following patch fixes the pointer manipulation in ide_read(), ide_write() and atapi_read().

diff -up a/common/cmd_ide.c b/common/cmd_ide.c
@@ -1344,7 +1344,7 @@ ulong ide_read (int device, lbaint_t blk
 
                ++n;
                ++blknr;
-               buffer += ATA_SECTORWORDS;
+               buffer += ATA_BLOCKSIZE;
        }
 IDE_READ_E:
        ide_led (DEVICE_LED(device), 0);        /* LED off      */

@@ -1428,7 +1428,7 @@ ulong ide_write (int device, lbaint_t bl
                c = ide_inb (device, ATA_STATUS);       /* clear IRQ */
                ++n;
                ++blknr;
-               buffer += ATA_SECTORWORDS;
+               buffer += ATA_BLOCKSIZE;
        }
 WR_OUT:
        ide_led (DEVICE_LED(device), 0);        /* LED off      */

@@ -2052,7 +2052,7 @@ ulong atapi_read (int device, lbaint_t b
                n+=cnt;
                blkcnt-=cnt;
                blknr+=cnt;
-               buffer+=cnt*(ATAPI_READ_BLOCK_SIZE/4); /* ulong

blocksize in ulong */
+ buffer+=(cnt*ATAPI_READ_BLOCK_SIZE); /* ulong
blocksize in ulong */

        } while (blkcnt > 0);
        return (n);

}

<snip>

Yup, this is the correct fix. Good catch.

A few comments:

1. you need to add a Signed-of-by:" line at the bottom of your comment block
2. Looks like whitespace has been mangled in this patch. Tabs are now spaces and long lines have been wrapped. The patch doesn't apply. Your mail client probably did this to you.

3. Now that buffer is incremented by bytes, the comment no longer applies. Remove it in your patch.

Thanks again,
g.

Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely at secretlab.ca
(403) 399-0195

Oops. I started reading Documentation/SubmittingPatches. I just didn't get all the way through.

2. Looks like whitespace has been mangled in this patch. Tabs are now

Which is why I've changed to an attachment.

Fixed.......resubmitting
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.denx.de/pipermail/u-boot/attachments/20070412/ccf8b24a/attachment.htm
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cmd_ide.patch
Type: application/octet-stream
Size: 961 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20070412/ccf8b24a/attachment.obj

Looks better now.

'off' is spelt with 2 f's. :-P

Otherwise, looks good. Fix those two minor points and resubmit, and I'll ack it and poke Stefan to pick it up.

Thanks again.
g.

Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely at secretlab.ca
(403) 399-0195

Lost in the shuffle when wrestling with the mail client.

Out and out stupidity.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cmd_ide.patch
Type: application/octet-stream
Size: 934 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20070412/c667de59/attachment.obj

heeheehee
g.

Stefan, can you pick this one up please?

Thanks,
g.

Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely at secretlab.ca
(403) 399-0195

Hi Denis,

Yes I will. Just checking with an older patch that tried to fix this issue:

Denis, could you please take a look at this patch and let me know if this patch supersedes your patch:

"Re: [U-Boot-Users] [PATCH] Fix bugs in cmd_ide.c and cmd_scsi.c"

from April, 2nd?

Thanks.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
=====================================================================

Hello Stefan,

My patch from April, 2nd, fixes this issue already. In my patch I decided to just cast the pointer back to ulong *, since I don't know if it is a good idea to increment a pointer to void *.
But the same bug is also in cmd_scsi.c which also fixed by my patch. The other bug which prevented to use the command "diskboot" and "scsiboot" from other devices than device 0, is also fixed by my patch.
It seems that the function atapi_read slipped my attention, this function is not fixed by my patch.

With best regards,

Denis

Incrementing a void* is safe.

Personally, I prefer the solution in Greg's patch.

Cheers,
g.

--
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely at secretlab.ca
(403) 399-0195

Hi All,

I appied Greg's patch and the device# part of Denis's patch to my u-boot-ppc4xx repository. Please give it a try and let me know if the issues are fixed now:

git://www.denx.de/git/u-boot-ppc4xx.git

Then I'll ask Wolfgang to pull from my repository.

Thanks.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
=====================================================================

Hello Stefan,

I currently don't have the hardware to test the scsiboot, but I tested "ide_read", "ide_write" and "diskboot". It works, so I assume "scsiboot" is also Ok.

Thank you.

Best regards,
Denis

It's not part of the normal C standard, but gcc allows you to perform pointer math on a void *. It acts as is it were a "u8 *".

Remember, this code:

ulong *p;
p++;

Has the same effect as:

void *p
p += sizeof(ulong);

Timur Tabi
Linux Kernel Developer @ Freescale