Patchwork ps3disk: passing wrong variable to bvec_kunmap_irq()

login
register
mail settings
Submitter Dan Carpenter
Date Oct. 11, 2010, 7:13 p.m.
Message ID <20101011191335.GH5851@bicker>
Download mbox | patch
Permalink /patch/67474/
State Not Applicable
Headers show

Comments

Dan Carpenter - Oct. 11, 2010, 7:13 p.m.
This should pass "buf" to bvec_kunmap_irq() instead of "bv".  The api is
like kmap_atomic() instead of kmap().

Signed-off-by: Dan Carpenter <error27@gmail.com>
Jens Axboe - Oct. 11, 2010, 7:42 p.m.
On 2010-10-11 21:13, Dan Carpenter wrote:
> This should pass "buf" to bvec_kunmap_irq() instead of "bv".  The api is
> like kmap_atomic() instead of kmap().
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> 
> diff --git a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c
> index e9da874..03688c2 100644
> --- a/drivers/block/ps3disk.c
> +++ b/drivers/block/ps3disk.c
> @@ -113,7 +113,7 @@ static void ps3disk_scatter_gather(struct ps3_storage_device *dev,
>  			memcpy(buf, dev->bounce_buf+offset, size);
>  		offset += size;
>  		flush_kernel_dcache_page(bvec->bv_page);
> -		bvec_kunmap_irq(bvec, &flags);
> +		bvec_kunmap_irq(buf, &flags);
>  		i++;
>  	}
>  }

Thanks applied, that bug is all too common.
Geoff Levand - Oct. 12, 2010, 4:01 p.m.
On 10/11/2010 12:13 PM, Dan Carpenter wrote:
> This should pass "buf" to bvec_kunmap_irq() instead of "bv".  The api is
> like kmap_atomic() instead of kmap().
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>

Looks good.  Thanks.

Acked-by: Geoff Levand <geoff@infradead.org>
Geert Uytterhoeven - Oct. 16, 2010, 7:39 p.m.
On Mon, Oct 11, 2010 at 21:42, Jens Axboe <jaxboe@fusionio.com> wrote:
> On 2010-10-11 21:13, Dan Carpenter wrote:
>> This should pass "buf" to bvec_kunmap_irq() instead of "bv".  The api is
>> like kmap_atomic() instead of kmap().
>>
>> Signed-off-by: Dan Carpenter <error27@gmail.com>
>>
>> diff --git a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c
>> index e9da874..03688c2 100644
>> --- a/drivers/block/ps3disk.c
>> +++ b/drivers/block/ps3disk.c
>> @@ -113,7 +113,7 @@ static void ps3disk_scatter_gather(struct ps3_storage_device *dev,
>>                       memcpy(buf, dev->bounce_buf+offset, size);
>>               offset += size;
>>               flush_kernel_dcache_page(bvec->bv_page);
>> -             bvec_kunmap_irq(bvec, &flags);
>> +             bvec_kunmap_irq(buf, &flags);
>>               i++;
>>       }
>>  }
>
> Thanks applied, that bug is all too common.

Because  bvec_kunmap_irq() is a macro if !CONFIG_HIGHMEM, and thus there's no
argument type checking on e.g. pp64, which doesn't support HIGHMEM?

What about turning it into an inline function?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Jens Axboe - Oct. 17, 2010, 6:49 a.m.
On 2010-10-16 21:39, Geert Uytterhoeven wrote:
> On Mon, Oct 11, 2010 at 21:42, Jens Axboe <jaxboe@fusionio.com> wrote:
>> On 2010-10-11 21:13, Dan Carpenter wrote:
>>> This should pass "buf" to bvec_kunmap_irq() instead of "bv".  The api is
>>> like kmap_atomic() instead of kmap().
>>>
>>> Signed-off-by: Dan Carpenter <error27@gmail.com>
>>>
>>> diff --git a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c
>>> index e9da874..03688c2 100644
>>> --- a/drivers/block/ps3disk.c
>>> +++ b/drivers/block/ps3disk.c
>>> @@ -113,7 +113,7 @@ static void ps3disk_scatter_gather(struct ps3_storage_device *dev,
>>>                       memcpy(buf, dev->bounce_buf+offset, size);
>>>               offset += size;
>>>               flush_kernel_dcache_page(bvec->bv_page);
>>> -             bvec_kunmap_irq(bvec, &flags);
>>> +             bvec_kunmap_irq(buf, &flags);
>>>               i++;
>>>       }
>>>  }
>>
>> Thanks applied, that bug is all too common.
> 
> Because  bvec_kunmap_irq() is a macro if !CONFIG_HIGHMEM, and thus there's no
> argument type checking on e.g. pp64, which doesn't support HIGHMEM?

It's a generic problem, not isolated to this case. The problem is that
the API isn't symmetric, and the unmap parts take a void * pointer.
Geert Uytterhoeven - Oct. 17, 2010, 11:36 a.m.
On Sun, Oct 17, 2010 at 08:49, Jens Axboe <jaxboe@fusionio.com> wrote:
> On 2010-10-16 21:39, Geert Uytterhoeven wrote:
>> On Mon, Oct 11, 2010 at 21:42, Jens Axboe <jaxboe@fusionio.com> wrote:
>>> On 2010-10-11 21:13, Dan Carpenter wrote:
>>>> This should pass "buf" to bvec_kunmap_irq() instead of "bv".  The api is
>>>> like kmap_atomic() instead of kmap().
>>>>
>>>> Signed-off-by: Dan Carpenter <error27@gmail.com>
>>>>
>>>> diff --git a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c
>>>> index e9da874..03688c2 100644
>>>> --- a/drivers/block/ps3disk.c
>>>> +++ b/drivers/block/ps3disk.c
>>>> @@ -113,7 +113,7 @@ static void ps3disk_scatter_gather(struct ps3_storage_device *dev,
>>>>                       memcpy(buf, dev->bounce_buf+offset, size);
>>>>               offset += size;
>>>>               flush_kernel_dcache_page(bvec->bv_page);
>>>> -             bvec_kunmap_irq(bvec, &flags);
>>>> +             bvec_kunmap_irq(buf, &flags);
>>>>               i++;
>>>>       }
>>>>  }
>>>
>>> Thanks applied, that bug is all too common.
>>
>> Because  bvec_kunmap_irq() is a macro if !CONFIG_HIGHMEM, and thus there's no
>> argument type checking on e.g. pp64, which doesn't support HIGHMEM?
>
> It's a generic problem, not isolated to this case. The problem is that
> the API isn't symmetric, and the unmap parts take a void * pointer.

No, the unmap takes a char *:

static inline void bvec_kunmap_irq(char *buffer, unsigned long *flags)

So it could be detected. Patch will follow.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Patch

diff --git a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c
index e9da874..03688c2 100644
--- a/drivers/block/ps3disk.c
+++ b/drivers/block/ps3disk.c
@@ -113,7 +113,7 @@  static void ps3disk_scatter_gather(struct ps3_storage_device *dev,
 			memcpy(buf, dev->bounce_buf+offset, size);
 		offset += size;
 		flush_kernel_dcache_page(bvec->bv_page);
-		bvec_kunmap_irq(bvec, &flags);
+		bvec_kunmap_irq(buf, &flags);
 		i++;
 	}
 }