| Message ID | 20101011191335.GH5851@bicker (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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.
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>
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
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.
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
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++; } }
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>