[U-Boot-Users] [PATCH] Fix use of "void *" for block dev read/write buffer pointers

[U-Boot-Users] [PATCH] Fix use of "void *" for block dev read/write buffer pointers

[Greg Lopp]

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

[U-Boot-Users] [PATCH] Fix use of "void *" for block dev read/write buffer pointers

[Grant Likely]

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); /* ulong
> blocksize in ulong */

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

[U-Boot-Users] [PATCH] Fix use of "void *" for block dev read/write buffer pointers

[Greg Lopp]

On 4/12/07, Grant Likely <grant.likely@secretlab.ca> wrote:

>
> A few comments:
>
> 1. 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); /* ulong
> > blocksize in ulong */
>
> 3. Now that buffer is incremented by bytes, the comment no longer
> applies.  Remove it in your patch.


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 

[U-Boot-Users] [PATCH] Fix use of "void *" for block dev read/write buffer pointers

[Grant Likely]

On 4/12/07, Greg Lopp <lopp@pobox.com> wrote:
> On 4/12/07, Grant Likely <grant.likely@secretlab.ca> wrote:
> > 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.

Looks better now.

> > > blocksize in ulong */
> > > +               buffer+=(cnt*ATAPI_READ_BLOCK_SIZE); /*
> ulong
> > > blocksize in ulong */
> >
> > 3. 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.

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

[U-Boot-Users] [PATCH] Fix use of "void *" for block dev read/write buffer pointers

[Greg Lopp]

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:
> > > 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.
>
> Looks better now.
>
> > > > blocksize in ulong */
> > > > +               buffer+=(cnt*ATAPI_READ_BLOCK_SIZE); /*
> > ulong
> > > > blocksize in ulong */
> > >
> > > 3. 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.
-------------- 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 

[U-Boot-Users] [PATCH] Fix use of "void *" for block dev read/write buffer pointers

[Grant Likely]

On 4/12/07, Ben Warren <bwarren@qstreams.com> wrote:
>
> I'm by no means a git expert, but a lot of the git commands have a
> '--signoff' switch that fills this stuff in for you.  Very helpful for
> pople like me that leve a leter out here and ther.

heeheehee
g.

[U-Boot-Users] [PATCH] Fix use of "void *" for block dev read/write buffer pointers

[Grant Likely]

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.

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

[U-Boot-Users] [PATCH] Fix use of "void *" for block dev read/write buffer pointers

[Stefan Roese]

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

[U-Boot-Users] [PATCH] Fix use of "void *" for block dev read/write buffer pointers

[Denis Peter]

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

[U-Boot-Users] [PATCH] Fix use of "void *" for block dev read/write buffer pointers

[Grant Likely]

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 at secretlab.ca
(403) 399-0195

[U-Boot-Users] [PATCH] Fix use of "void *" for block dev read/write buffer pointers

[Stefan Roese]

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

[U-Boot-Users] [PATCH] Fix use of "void *" for block dev read/write buffer pointers

[Denis Peter]

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

[U-Boot-Users] [PATCH] Fix use of "void *" for block dev read/write buffer pointers

[Timur Tabi]

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

-- 
Timur Tabi
Linux Kernel Developer @ Freescale