[U-Boot-Users] [PATCH] 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 --- a/common/cmd_ide.c 2007-04-12 09:12:49.000000000 -0500 +++ b/common/cmd_ide.c 2007-04-12 09:14:40.843750000 -0500 @@ -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); }
On 4/12/07, Greg Lopp lopp@pobox.com wrote:
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.....
<snip>
The following patch fixes the pointer manipulation in ide_read(), ide_write() and atapi_read().
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.
blocksize in ulong */
buffer+=(cnt*ATAPI_READ_BLOCK_SIZE); /* ulongblocksize in ulong */
3. Now that buffer is incremented by bytes, the comment no longer applies. Remove it in your patch.
Thanks again, g.
On 4/12/07, Grant Likely grant.likely@secretlab.ca wrote:
A few comments:
- you need to add a Signed-of-by:" line at the bottom of your comment
block
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
spaces and long lines have been wrapped. The patch doesn't apply. Your mail client probably did this to you.
Which is why I've changed to an attachment.
blocksize in ulong */
buffer+=(cnt*ATAPI_READ_BLOCK_SIZE); /* ulongblocksize in ulong */
- Now that buffer is incremented by bytes, the comment no longer
applies. Remove it in your patch.
Fixed.......resubmitting
On 4/12/07, Greg Lopp lopp@pobox.com wrote:
On 4/12/07, Grant Likely grant.likely@secretlab.ca wrote:
- 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.
Which is why I've changed to an attachment.
Looks better now.
blocksize in ulong */
buffer+=(cnt*ATAPI_READ_BLOCK_SIZE); /*ulong
blocksize in ulong */
- Now that buffer is incremented by bytes, the comment no longer
applies. Remove it in your patch.
Fixed.......resubmitting
buffer+=cnt*(ATAPI_READ_BLOCK_SIZE/4); /* ulong blocksize in ulong */
buffer+=(cnt*ATAPI_READ_BLOCK_SIZE); /* ulong blocksize in ulong */
The comment at the end of the line still needs to be removed as it is no longer accurate.
Signed-of-by: Greg Lopp lopp@pobox.com
'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.
On 4/12/07, Grant Likely grant.likely@secretlab.ca wrote:
On 4/12/07, Greg Lopp lopp@pobox.com wrote:
On 4/12/07, Grant Likely grant.likely@secretlab.ca wrote:
- 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.
Which is why I've changed to an attachment.
Looks better now.
blocksize in ulong */
buffer+=(cnt*ATAPI_READ_BLOCK_SIZE); /*ulong
blocksize in ulong */
- Now that buffer is incremented by bytes, the comment no longer
applies. Remove it in your patch.
Fixed.......resubmitting
buffer+=cnt*(ATAPI_READ_BLOCK_SIZE/4); /* ulong blocksize in ulong */
buffer+=(cnt*ATAPI_READ_BLOCK_SIZE); /* ulong blocksize in ulong */The comment at the end of the line still needs to be removed as it is no longer accurate.
Lost in the shuffle when wrestling with the mail client.
Signed-of-by: Greg Lopp lopp@pobox.com
'off' is spelt with 2 f's. :-P
Out and out stupidity.
On 4/12/07, Greg Lopp lopp@pobox.com wrote:
Signed-off-by: Greg Lopp lopp@pobox.com
Acked-by: Grant Likely grant.likely@secretlab.ca
Stefan, can you pick this one up please?
Thanks, g.
Hi Denis,
On Thursday 12 April 2007 22:27, Grant Likely wrote:
On 4/12/07, Greg Lopp lopp@pobox.com wrote:
Signed-off-by: Greg Lopp lopp@pobox.com
Acked-by: Grant Likely grant.likely@secretlab.ca
Stefan, can you pick this one up please?
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,
On Friday 13 April 2007 07:56, Stefan Roese wrote:
Hi Denis,
On Thursday 12 April 2007 22:27, Grant Likely wrote:
On 4/12/07, Greg Lopp lopp@pobox.com wrote:
Signed-off-by: Greg Lopp lopp@pobox.com
Acked-by: Grant Likely grant.likely@secretlab.ca
Stefan, can you pick this one up please?
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?
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
On 4/13/07, Denis Peter d.peter@mpl.ch wrote:
Hello Stefan,
On Friday 13 April 2007 07:56, Stefan Roese wrote:
Hi Denis,
On Thursday 12 April 2007 22:27, Grant Likely wrote:
On 4/12/07, Greg Lopp lopp@pobox.com wrote:
Signed-off-by: Greg Lopp lopp@pobox.com
Acked-by: Grant Likely grant.likely@secretlab.ca
Stefan, can you pick this one up please?
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?
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 *.
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@secretlab.ca (403) 399-0195
Hi All,
On Friday 13 April 2007 08:54, Grant Likely wrote:
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?
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 *.
Incrementing a void* is safe.
Personally, I prefer the solution in Greg's patch.
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,
On Friday 13 April 2007 09:18, Stefan Roese wrote:
Hi All,
On Friday 13 April 2007 08:54, Grant Likely wrote:
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?
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 *.
Incrementing a void* is safe.
Personally, I prefer the solution in Greg's patch.
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:
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
Denis Peter wrote:
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 *.
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);
participants (5)
-
Denis Peter -
Grant Likely -
Greg Lopp -
Stefan Roese -
Timur Tabi