2006-10-24 16:28:11

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH 2/3] spufs: fix another off-by-one bug in mbox_read

Next try, the previous one did not do what I expected.

Signed-off-by: Arnd Bergmann <[email protected]>

Index: linux-2.6/arch/powerpc/platforms/cell/spufs/file.c
===================================================================
--- linux-2.6.orig/arch/powerpc/platforms/cell/spufs/file.c
+++ linux-2.6/arch/powerpc/platforms/cell/spufs/file.c
@@ -385,7 +385,7 @@ static ssize_t spufs_mbox_read(struct fi
udata = (void __user *)buf;

spu_acquire(ctx);
- for (count = 0; count <= len; count += 4, udata++) {
+ for (count = 0; (count + 4) <= len; count += 4, udata++) {
int ret;
ret = ctx->ops->mbox_read(ctx, &mbox_data);
if (ret == 0)

--


2006-10-24 18:42:32

by Pekka Enberg

[permalink] [raw]
Subject: Re: [PATCH 2/3] spufs: fix another off-by-one bug in mbox_read

Hi Arnd,

On 10/24/06, Arnd Bergmann <[email protected]> wrote:
> spu_acquire(ctx);
> - for (count = 0; count <= len; count += 4, udata++) {
> + for (count = 0; (count + 4) <= len; count += 4, udata++) {

Wouldn't this be more obvious as

for (count = 0, count < (len / 4); count++, udata++) {

And then do count * 4 if you need the actual index somewhere. Hmm?

2006-10-24 19:07:54

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 2/3] spufs: fix another off-by-one bug in mbox_read

On Tuesday 24 October 2006 20:42, Pekka Enberg wrote:
> On 10/24/06, Arnd Bergmann <[email protected]> wrote:
> > ? ? ? ? spu_acquire(ctx);
> > - ? ? ? for (count = 0; count <= len; count += 4, udata++) {
> > + ? ? ? for (count = 0; (count + 4) <= len; count += 4, udata++) {
>
> Wouldn't this be more obvious as
>
> ? for (count = 0, count < (len / 4); count++, udata++) {
>
> And then do count * 4 if you need the actual index somewhere. Hmm?

Count is the return value from a write() file operation. I find it
more readable to update that every time I do one put_user(), to
the exact value, than calculating the return code later.

Arnd <><

2006-10-24 19:53:36

by Will Schmidt

[permalink] [raw]
Subject: Re: [PATCH 2/3] spufs: fix another off-by-one bug in mbox_read

On Tue, 2006-24-10 at 21:07 +0200, Arnd Bergmann wrote:
> On Tuesday 24 October 2006 20:42, Pekka Enberg wrote:
> > On 10/24/06, Arnd Bergmann <[email protected]> wrote:
> > > spu_acquire(ctx);
> > > - for (count = 0; count <= len; count += 4, udata++) {
> > > + for (count = 0; (count + 4) <= len; count += 4, udata++) {
> >
> > Wouldn't this be more obvious as
> >
> > for (count = 0, count < (len / 4); count++, udata++) {
> >
> > And then do count * 4 if you need the actual index somewhere. Hmm?
>
> Count is the return value from a write() file operation. I find it
> more readable to update that every time I do one put_user(), to
> the exact value, than calculating the return code later.

Hey Arnd,
just curiosity.. What was the behavior before this patch? just
leaving a few (0 - 3) characters behind?


>
> Arnd <><
> _______________________________________________
> Linuxppc-dev mailing list
> [email protected]
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

2006-10-24 19:58:39

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 2/3] spufs: fix another off-by-one bug in mbox_read

On Tuesday 24 October 2006 21:53, Will Schmidt wrote:
> Hey Arnd,
>    just curiosity..   What was the behavior before this patch?   just
> leaving a few (0 - 3) characters behind?

It transfers more bytes than requested on a read. If you asked for
four bytes, you got eight.

Note: one nasty property of this file in spufs is that you can only
read multiples of four bytes in the first place, there is no way to
atomically put back a few bytes into the hardware register, so reading
less than four bytes returns -EINVAL. Asking for more than four
should return the largest possible multiple of four.

Arnd <><