Guys, I just pushed out a new helper function intended for cleaning up
various device driver mmap functions, because they are rather messy,
and at least part of the problem was the bad impedance between what a
driver author would want to have, and the VM interfaces to map a
memory range into user space with mmap.
Some drivers would end up doing extensive checks on the length of the
mappings and the page offset within the mapping, while other drivers
would end up doing no checks at all.
The new helper is in commit b4cbb197c7e7 ("vm: add vm_iomap_memory()
helper function"), but I didn't actually commit any *users* of it,
because I just have this untested patch-collection for a few random
drivers (picked across a few different driver subsystems, just to make
it interesting). I did that largely just to check the different use
cases, but I don't actually tend to *use* all that many fancy drivers,
so I don't have much of a way of testing it.
The media layer has a few users of [io_]remap_pfn_range() that look
like they could do with some tender loving too, but they don't match
this particular pattern of "allow users to map a part of a fixed range
of memory". In fact, the media pattern seems to be single-page
mappings, which probably should use "vm_insert_page()" instead, but
that's a whole separate thing. But I didn't check all the media cases
(and there's a lot of remap_pfn_range use outside of media drivers I
didn't check either), so there might be code that could use the new
helper.
Anyway, I'm attaching the *untested* patch to several drivers. Guys,
mind taking a look? The point here is to simplify the interface,
avoiding bugs, but also:
5 files changed, 21 insertions(+), 87 deletions(-)
it needs current -git for the new helper function.
NOTE! The driver subsystem .mmap functions seem to almost universally do
if (io_remap_pfn_range(..))
return -EAGAIN;
return 0;
and I didn't make the new helper function do that "turn all
remap_pfn_range errors into EAGAIN". My *suspicion* is that this is
just really old copy-pasta and makes no sense, but maybe there is some
actual reasoning behind EAGAIN vs ENOMEM, for example. EAGAIN is
documented to be about file/memory locking, which means that it really
doesn't make any sense, but obviously there might be some binary that
actally depends on this, so I'm perfectly willing to make the helper
do that odd error case, I'd just like to know (and a add a comment)
WHY.
My personal guess is that nobody actually cares (we return other error
codes for other cases, notably EINVAL for various out-of-mapping-range
issues), and the whole EAGAIN return value is just a completely
historical oddity.
(And yes, I know the mtdchar code is actually disabled right now. But
that was a good example of a driver that had a bug in this area and
that I touched myself not too long ago, and recent stable noise
reminded me of it, so I did that one despite it not being active).
Linus
At Tue, 16 Apr 2013 20:12:32 -0700,
Linus Torvalds wrote:
>
> Guys, I just pushed out a new helper function intended for cleaning up
> various device driver mmap functions, because they are rather messy,
> and at least part of the problem was the bad impedance between what a
> driver author would want to have, and the VM interfaces to map a
> memory range into user space with mmap.
>
> Some drivers would end up doing extensive checks on the length of the
> mappings and the page offset within the mapping, while other drivers
> would end up doing no checks at all.
>
> The new helper is in commit b4cbb197c7e7 ("vm: add vm_iomap_memory()
> helper function"), but I didn't actually commit any *users* of it,
> because I just have this untested patch-collection for a few random
> drivers (picked across a few different driver subsystems, just to make
> it interesting). I did that largely just to check the different use
> cases, but I don't actually tend to *use* all that many fancy drivers,
> so I don't have much of a way of testing it.
>
> The media layer has a few users of [io_]remap_pfn_range() that look
> like they could do with some tender loving too, but they don't match
> this particular pattern of "allow users to map a part of a fixed range
> of memory". In fact, the media pattern seems to be single-page
> mappings, which probably should use "vm_insert_page()" instead, but
> that's a whole separate thing. But I didn't check all the media cases
> (and there's a lot of remap_pfn_range use outside of media drivers I
> didn't check either), so there might be code that could use the new
> helper.
>
> Anyway, I'm attaching the *untested* patch to several drivers. Guys,
> mind taking a look? The point here is to simplify the interface,
> avoiding bugs, but also:
>
> 5 files changed, 21 insertions(+), 87 deletions(-)
>
> it needs current -git for the new helper function.
A nice cleanup, and the change in sound/core/pcm_native.c looks good.
Unfortunately I can't test this code path since it's very specific to
some hardware, though.
Feel free to take
Acked-by: Takashi Iwai <[email protected]>
> NOTE! The driver subsystem .mmap functions seem to almost universally do
>
> if (io_remap_pfn_range(..))
> return -EAGAIN;
> return 0;
>
> and I didn't make the new helper function do that "turn all
> remap_pfn_range errors into EAGAIN". My *suspicion* is that this is
> just really old copy-pasta and makes no sense, but maybe there is some
> actual reasoning behind EAGAIN vs ENOMEM, for example. EAGAIN is
> documented to be about file/memory locking, which means that it really
> doesn't make any sense, but obviously there might be some binary that
> actally depends on this, so I'm perfectly willing to make the helper
> do that odd error case, I'd just like to know (and a add a comment)
> WHY.
>
> My personal guess is that nobody actually cares (we return other error
> codes for other cases, notably EINVAL for various out-of-mapping-range
> issues), and the whole EAGAIN return value is just a completely
> historical oddity.
I took a quick look through 2.4 kernel code, and almost all
remap_page_range() and io_remap_page_range() calls return -EAGAIN for
errors. And we follow majority.
thanks,
Takashi
>
> (And yes, I know the mtdchar code is actually disabled right now. But
> that was a good example of a driver that had a bug in this area and
> that I touched myself not too long ago, and recent stable noise
> reminded me of it, so I did that one despite it not being active).
On Wednesday 17 April 2013, Linus Torvalds wrote:
> Anyway, I'm attaching the untested patch to several drivers. Guys,
> mind taking a look? The point here is to simplify the interface,
> avoiding bugs, but also:
>
> 5 files changed, 21 insertions(+), 87 deletions(-)
>
> it needs current -git for the new helper function.
>
> NOTE! The driver subsystem .mmap functions seem to almost universally do
>
> if (io_remap_pfn_range(..))
> return -EAGAIN;
> return 0;
I took a look at the hpet_mmap function, which still contains this check:
if (((vma->vm_end - vma->vm_start) != PAGE_SIZE) || vma->vm_pgoff)
return -EINVAL;
As far as I can tell, this check is implied by the new code in
vm_iomap_memory as the len argument passed here is PAGE_SIZE, so you
can remove another three lines in hpet_mmap.
Arnd
Arnd Bergmann wrote:
> On Wednesday 17 April 2013, Linus Torvalds wrote:
>> Anyway, I'm attaching the untested patch to several drivers. Guys,
>> mind taking a look?
>
> I took a look at the hpet_mmap function, which still contains this check:
>
> if (((vma->vm_end - vma->vm_start) != PAGE_SIZE) || vma->vm_pgoff)
> return -EINVAL;
>
> As far as I can tell, this check is implied by the new code in
> vm_iomap_memory as the len argument passed here is PAGE_SIZE, so you
> can remove another three lines in hpet_mmap.
Yes indeed.
> > [...] I just have this untested patch-collection for a few random
> > drivers (picked across a few different driver subsystems, just to make
> > it interesting). I did that largely just to check the different use
> > cases, but I don't actually tend to *use* all that many fancy drivers,
> > so I don't have much of a way of testing it.
Any more-or-less recent x86 machine has HPET, so you could enable
CONFIG_HPET(_MMAP) and try the (completely untested) program below.
Regards,
Clemens
--8<---------------------------------------------------------------->8--
#include <stdio.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/mman.h>
int main(void)
{
int fd = open("/dev/hpet", O_RDONLY);
if (fd == -1) {
perror("/dev/hpet");
return 1;
}
const volatile unsigned int *ptr = mmap(NULL, 4096, PROT_READ, MAP_SHARED, fd, 0);
if (ptr == MAP_FAILED) {
perror("mmap");
return 1;
}
printf("frequency: %.5f MHz\n", 1e9 / ptr[1]);
for (;;) {
printf("\rcounter: %08x", ptr[60]);
fflush(stdout);
usleep(123456);
}
}
Em Tue, 16 Apr 2013 20:12:32 -0700
Linus Torvalds <[email protected]> escreveu:
> Guys, I just pushed out a new helper function intended for cleaning up
> various device driver mmap functions, because they are rather messy,
> and at least part of the problem was the bad impedance between what a
> driver author would want to have, and the VM interfaces to map a
> memory range into user space with mmap.
>
> Some drivers would end up doing extensive checks on the length of the
> mappings and the page offset within the mapping, while other drivers
> would end up doing no checks at all.
>
> The new helper is in commit b4cbb197c7e7 ("vm: add vm_iomap_memory()
> helper function"), but I didn't actually commit any *users* of it,
> because I just have this untested patch-collection for a few random
> drivers (picked across a few different driver subsystems, just to make
> it interesting). I did that largely just to check the different use
> cases, but I don't actually tend to *use* all that many fancy drivers,
> so I don't have much of a way of testing it.
>
> The media layer has a few users of [io_]remap_pfn_range() that look
> like they could do with some tender loving too, but they don't match
> this particular pattern of "allow users to map a part of a fixed range
> of memory". In fact, the media pattern seems to be single-page
> mappings, which probably should use "vm_insert_page()" instead, but
> that's a whole separate thing. But I didn't check all the media cases
> (and there's a lot of remap_pfn_range use outside of media drivers I
> didn't check either), so there might be code that could use the new
> helper.
I think that [io_]remap_pfn_range() calls are used by the oldest drivers
and a few newer ones that based on some old cut-and-paste code.
Let me see what drivers use it on media...
$ git grep -l remap_pfn_range drivers/media/
drivers/media/pci/meye/meye.c
drivers/media/pci/zoran/zoran_driver.c
drivers/media/platform/omap/omap_vout.c
drivers/media/platform/omap24xxcam.c
drivers/media/platform/vino.c
drivers/media/usb/cpia2/cpia2_core.c
drivers/media/v4l2-core/videobuf-dma-contig.c
Yes, meye, vino, cpia2 and zoran are very old drivers. I only have
here somewhere zoran cards. I'll see if they still work, and write
a patch for it.
The platform drivers that use remap_pfn_range() are omap2 and vino.
Vino is for SGI Indy machines. I dunno anyone with those hardware
and a camera anymore. The OMAP2 were used on some Nokia phones.
They used to maintain that code, but now that they moved to the dark
side of the moon, they lost their interests on it. So, it may not
be easily find testers for patches there.
The videobuf-dma-contig code actually uses two implementations there:
one using vm_insert_page() for cached memory, and another one using
remap_pfn_range() for uncached memory.
All places where cached memory is used got already moved to
videobuf2-dma-contig. We can simply drop that unused code from it,
and remap_pfn_range() by vm_iomap_memory().
I can write the patches doing it, but I don't any hardware here
using videobuf-dma-contig for testing. So, I'll post it
asking people to test.
>
> Anyway, I'm attaching the *untested* patch to several drivers. Guys,
> mind taking a look? The point here is to simplify the interface,
> avoiding bugs, but also:
>
> 5 files changed, 21 insertions(+), 87 deletions(-)
>
> it needs current -git for the new helper function.
>
> NOTE! The driver subsystem .mmap functions seem to almost universally do
>
> if (io_remap_pfn_range(..))
> return -EAGAIN;
> return 0;
>
> and I didn't make the new helper function do that "turn all
> remap_pfn_range errors into EAGAIN". My *suspicion* is that this is
> just really old copy-pasta and makes no sense, but maybe there is some
> actual reasoning behind EAGAIN vs ENOMEM, for example. EAGAIN is
> documented to be about file/memory locking, which means that it really
> doesn't make any sense, but obviously there might be some binary that
> actally depends on this, so I'm perfectly willing to make the helper
> do that odd error case, I'd just like to know (and a add a comment)
> WHY.
>
> My personal guess is that nobody actually cares (we return other error
> codes for other cases, notably EINVAL for various out-of-mapping-range
> issues), and the whole EAGAIN return value is just a completely
> historical oddity.
>
> (And yes, I know the mtdchar code is actually disabled right now. But
> that was a good example of a driver that had a bug in this area and
> that I touched myself not too long ago, and recent stable noise
> reminded me of it, so I did that one despite it not being active).
>
> Linus
Regards,
Mauro
On 2013-04-17 06:12, Linus Torvalds wrote:
> Guys, I just pushed out a new helper function intended for cleaning up
> various device driver mmap functions, because they are rather messy,
> and at least part of the problem was the bad impedance between what a
> driver author would want to have, and the VM interfaces to map a
> memory range into user space with mmap.
>
> Some drivers would end up doing extensive checks on the length of the
> mappings and the page offset within the mapping, while other drivers
> would end up doing no checks at all.
>
> The new helper is in commit b4cbb197c7e7 ("vm: add vm_iomap_memory()
> helper function"), but I didn't actually commit any *users* of it,
> because I just have this untested patch-collection for a few random
> drivers (picked across a few different driver subsystems, just to make
> it interesting). I did that largely just to check the different use
> cases, but I don't actually tend to *use* all that many fancy drivers,
> so I don't have much of a way of testing it.
Should there be a similar helper that uses remap_pfn_range() instead of
io_remap_pfn_range()?
Some fb drivers use remap_pfn_range(), and I'm not sure if there could
be some side effects on some platforms if they are changed to use
io_remap_pfn_range(). At least mips and sparc seem to have their own
versions for io_remap_pfn_range().
Tomi
vm_iomap_memory() provides a better end user interface than
remap_pfn_range(), as it does the needed tests before doing
mmap.
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
drivers/media/v4l2-core/videobuf-dma-contig.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c b/drivers/media/v4l2-core/videobuf-dma-contig.c
index 67f572c..7e6b209 100644
--- a/drivers/media/v4l2-core/videobuf-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf-dma-contig.c
@@ -303,14 +303,9 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
goto error;
/* Try to remap memory */
-
size = vma->vm_end - vma->vm_start;
- size = (size < mem->size) ? size : mem->size;
-
vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
- retval = remap_pfn_range(vma, vma->vm_start,
- mem->dma_handle >> PAGE_SHIFT,
- size, vma->vm_page_prot);
+ retval = vm_iomap_memory(vma, vma->vm_start, size);
if (retval) {
dev_err(q->dev, "mmap: remap failed with error %d. ",
retval);
--
1.8.1.4
videobuf_queue_dma_contig_init_cached() is not used anywhere.
Drop support for it, cleaning up the code a little bit.
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
drivers/media/v4l2-core/videobuf-dma-contig.c | 130 +++-----------------------
include/media/videobuf-dma-contig.h | 10 --
2 files changed, 14 insertions(+), 126 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c b/drivers/media/v4l2-core/videobuf-dma-contig.c
index 3a43ba0..67f572c 100644
--- a/drivers/media/v4l2-core/videobuf-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf-dma-contig.c
@@ -27,7 +27,6 @@ struct videobuf_dma_contig_memory {
u32 magic;
void *vaddr;
dma_addr_t dma_handle;
- bool cached;
unsigned long size;
};
@@ -43,26 +42,8 @@ static int __videobuf_dc_alloc(struct device *dev,
unsigned long size, gfp_t flags)
{
mem->size = size;
- if (mem->cached) {
- mem->vaddr = alloc_pages_exact(mem->size, flags | GFP_DMA);
- if (mem->vaddr) {
- int err;
-
- mem->dma_handle = dma_map_single(dev, mem->vaddr,
- mem->size,
- DMA_FROM_DEVICE);
- err = dma_mapping_error(dev, mem->dma_handle);
- if (err) {
- dev_err(dev, "dma_map_single failed\n");
-
- free_pages_exact(mem->vaddr, mem->size);
- mem->vaddr = NULL;
- return err;
- }
- }
- } else
- mem->vaddr = dma_alloc_coherent(dev, mem->size,
- &mem->dma_handle, flags);
+ mem->vaddr = dma_alloc_coherent(dev, mem->size,
+ &mem->dma_handle, flags);
if (!mem->vaddr) {
dev_err(dev, "memory alloc size %ld failed\n", mem->size);
@@ -77,14 +58,7 @@ static int __videobuf_dc_alloc(struct device *dev,
static void __videobuf_dc_free(struct device *dev,
struct videobuf_dma_contig_memory *mem)
{
- if (mem->cached) {
- if (!mem->vaddr)
- return;
- dma_unmap_single(dev, mem->dma_handle, mem->size,
- DMA_FROM_DEVICE);
- free_pages_exact(mem->vaddr, mem->size);
- } else
- dma_free_coherent(dev, mem->size, mem->vaddr, mem->dma_handle);
+ dma_free_coherent(dev, mem->size, mem->vaddr, mem->dma_handle);
mem->vaddr = NULL;
}
@@ -234,7 +208,7 @@ out_up:
return ret;
}
-static struct videobuf_buffer *__videobuf_alloc_vb(size_t size, bool cached)
+static struct videobuf_buffer *__videobuf_alloc(size_t size)
{
struct videobuf_dma_contig_memory *mem;
struct videobuf_buffer *vb;
@@ -244,22 +218,11 @@ static struct videobuf_buffer *__videobuf_alloc_vb(size_t size, bool cached)
vb->priv = ((char *)vb) + size;
mem = vb->priv;
mem->magic = MAGIC_DC_MEM;
- mem->cached = cached;
}
return vb;
}
-static struct videobuf_buffer *__videobuf_alloc_uncached(size_t size)
-{
- return __videobuf_alloc_vb(size, false);
-}
-
-static struct videobuf_buffer *__videobuf_alloc_cached(size_t size)
-{
- return __videobuf_alloc_vb(size, true);
-}
-
static void *__videobuf_to_vaddr(struct videobuf_buffer *buf)
{
struct videobuf_dma_contig_memory *mem = buf->priv;
@@ -310,19 +273,6 @@ static int __videobuf_iolock(struct videobuf_queue *q,
return 0;
}
-static int __videobuf_sync(struct videobuf_queue *q,
- struct videobuf_buffer *buf)
-{
- struct videobuf_dma_contig_memory *mem = buf->priv;
- BUG_ON(!mem);
- MAGIC_CHECK(mem->magic, MAGIC_DC_MEM);
-
- dma_sync_single_for_cpu(q->dev, mem->dma_handle, mem->size,
- DMA_FROM_DEVICE);
-
- return 0;
-}
-
static int __videobuf_mmap_mapper(struct videobuf_queue *q,
struct videobuf_buffer *buf,
struct vm_area_struct *vma)
@@ -331,8 +281,6 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
struct videobuf_mapping *map;
int retval;
unsigned long size;
- unsigned long pos, start = vma->vm_start;
- struct page *page;
dev_dbg(q->dev, "%s\n", __func__);
@@ -359,43 +307,16 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
size = vma->vm_end - vma->vm_start;
size = (size < mem->size) ? size : mem->size;
- if (!mem->cached) {
- vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
- retval = remap_pfn_range(vma, vma->vm_start,
- mem->dma_handle >> PAGE_SHIFT,
+ vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+ retval = remap_pfn_range(vma, vma->vm_start,
+ mem->dma_handle >> PAGE_SHIFT,
size, vma->vm_page_prot);
- if (retval) {
- dev_err(q->dev, "mmap: remap failed with error %d. ",
- retval);
- dma_free_coherent(q->dev, mem->size,
- mem->vaddr, mem->dma_handle);
- goto error;
- }
- } else {
- pos = (unsigned long)mem->vaddr;
-
- while (size > 0) {
- page = virt_to_page((void *)pos);
- if (NULL == page) {
- dev_err(q->dev, "mmap: virt_to_page failed\n");
- __videobuf_dc_free(q->dev, mem);
- goto error;
- }
- retval = vm_insert_page(vma, start, page);
- if (retval) {
- dev_err(q->dev, "mmap: insert failed with error %d\n",
- retval);
- __videobuf_dc_free(q->dev, mem);
- goto error;
- }
- start += PAGE_SIZE;
- pos += PAGE_SIZE;
-
- if (size > PAGE_SIZE)
- size -= PAGE_SIZE;
- else
- size = 0;
- }
+ if (retval) {
+ dev_err(q->dev, "mmap: remap failed with error %d. ",
+ retval);
+ dma_free_coherent(q->dev, mem->size,
+ mem->vaddr, mem->dma_handle);
+ goto error;
}
vma->vm_ops = &videobuf_vm_ops;
@@ -417,17 +338,8 @@ error:
static struct videobuf_qtype_ops qops = {
.magic = MAGIC_QTYPE_OPS,
- .alloc_vb = __videobuf_alloc_uncached,
- .iolock = __videobuf_iolock,
- .mmap_mapper = __videobuf_mmap_mapper,
- .vaddr = __videobuf_to_vaddr,
-};
-
-static struct videobuf_qtype_ops qops_cached = {
- .magic = MAGIC_QTYPE_OPS,
- .alloc_vb = __videobuf_alloc_cached,
+ .alloc_vb = __videobuf_alloc,
.iolock = __videobuf_iolock,
- .sync = __videobuf_sync,
.mmap_mapper = __videobuf_mmap_mapper,
.vaddr = __videobuf_to_vaddr,
};
@@ -447,20 +359,6 @@ void videobuf_queue_dma_contig_init(struct videobuf_queue *q,
}
EXPORT_SYMBOL_GPL(videobuf_queue_dma_contig_init);
-void videobuf_queue_dma_contig_init_cached(struct videobuf_queue *q,
- const struct videobuf_queue_ops *ops,
- struct device *dev,
- spinlock_t *irqlock,
- enum v4l2_buf_type type,
- enum v4l2_field field,
- unsigned int msize,
- void *priv, struct mutex *ext_lock)
-{
- videobuf_queue_core_init(q, ops, dev, irqlock, type, field, msize,
- priv, &qops_cached, ext_lock);
-}
-EXPORT_SYMBOL_GPL(videobuf_queue_dma_contig_init_cached);
-
dma_addr_t videobuf_to_dma_contig(struct videobuf_buffer *buf)
{
struct videobuf_dma_contig_memory *mem = buf->priv;
diff --git a/include/media/videobuf-dma-contig.h b/include/media/videobuf-dma-contig.h
index f473aeb..f0ed825 100644
--- a/include/media/videobuf-dma-contig.h
+++ b/include/media/videobuf-dma-contig.h
@@ -26,16 +26,6 @@ void videobuf_queue_dma_contig_init(struct videobuf_queue *q,
void *priv,
struct mutex *ext_lock);
-void videobuf_queue_dma_contig_init_cached(struct videobuf_queue *q,
- const struct videobuf_queue_ops *ops,
- struct device *dev,
- spinlock_t *irqlock,
- enum v4l2_buf_type type,
- enum v4l2_field field,
- unsigned int msize,
- void *priv,
- struct mutex *ext_lock);
-
dma_addr_t videobuf_to_dma_contig(struct videobuf_buffer *buf);
void videobuf_dma_contig_free(struct videobuf_queue *q,
struct videobuf_buffer *buf);
--
1.8.1.4
On Wed 17 April 2013 14:22:15 Mauro Carvalho Chehab wrote:
> videobuf_queue_dma_contig_init_cached() is not used anywhere.
> Drop support for it, cleaning up the code a little bit.
>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
Nice!
Acked-by: Hans Verkuil <[email protected]>
> ---
> drivers/media/v4l2-core/videobuf-dma-contig.c | 130 +++-----------------------
> include/media/videobuf-dma-contig.h | 10 --
> 2 files changed, 14 insertions(+), 126 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c b/drivers/media/v4l2-core/videobuf-dma-contig.c
> index 3a43ba0..67f572c 100644
> --- a/drivers/media/v4l2-core/videobuf-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf-dma-contig.c
> @@ -27,7 +27,6 @@ struct videobuf_dma_contig_memory {
> u32 magic;
> void *vaddr;
> dma_addr_t dma_handle;
> - bool cached;
> unsigned long size;
> };
>
> @@ -43,26 +42,8 @@ static int __videobuf_dc_alloc(struct device *dev,
> unsigned long size, gfp_t flags)
> {
> mem->size = size;
> - if (mem->cached) {
> - mem->vaddr = alloc_pages_exact(mem->size, flags | GFP_DMA);
> - if (mem->vaddr) {
> - int err;
> -
> - mem->dma_handle = dma_map_single(dev, mem->vaddr,
> - mem->size,
> - DMA_FROM_DEVICE);
> - err = dma_mapping_error(dev, mem->dma_handle);
> - if (err) {
> - dev_err(dev, "dma_map_single failed\n");
> -
> - free_pages_exact(mem->vaddr, mem->size);
> - mem->vaddr = NULL;
> - return err;
> - }
> - }
> - } else
> - mem->vaddr = dma_alloc_coherent(dev, mem->size,
> - &mem->dma_handle, flags);
> + mem->vaddr = dma_alloc_coherent(dev, mem->size,
> + &mem->dma_handle, flags);
>
> if (!mem->vaddr) {
> dev_err(dev, "memory alloc size %ld failed\n", mem->size);
> @@ -77,14 +58,7 @@ static int __videobuf_dc_alloc(struct device *dev,
> static void __videobuf_dc_free(struct device *dev,
> struct videobuf_dma_contig_memory *mem)
> {
> - if (mem->cached) {
> - if (!mem->vaddr)
> - return;
> - dma_unmap_single(dev, mem->dma_handle, mem->size,
> - DMA_FROM_DEVICE);
> - free_pages_exact(mem->vaddr, mem->size);
> - } else
> - dma_free_coherent(dev, mem->size, mem->vaddr, mem->dma_handle);
> + dma_free_coherent(dev, mem->size, mem->vaddr, mem->dma_handle);
>
> mem->vaddr = NULL;
> }
> @@ -234,7 +208,7 @@ out_up:
> return ret;
> }
>
> -static struct videobuf_buffer *__videobuf_alloc_vb(size_t size, bool cached)
> +static struct videobuf_buffer *__videobuf_alloc(size_t size)
> {
> struct videobuf_dma_contig_memory *mem;
> struct videobuf_buffer *vb;
> @@ -244,22 +218,11 @@ static struct videobuf_buffer *__videobuf_alloc_vb(size_t size, bool cached)
> vb->priv = ((char *)vb) + size;
> mem = vb->priv;
> mem->magic = MAGIC_DC_MEM;
> - mem->cached = cached;
> }
>
> return vb;
> }
>
> -static struct videobuf_buffer *__videobuf_alloc_uncached(size_t size)
> -{
> - return __videobuf_alloc_vb(size, false);
> -}
> -
> -static struct videobuf_buffer *__videobuf_alloc_cached(size_t size)
> -{
> - return __videobuf_alloc_vb(size, true);
> -}
> -
> static void *__videobuf_to_vaddr(struct videobuf_buffer *buf)
> {
> struct videobuf_dma_contig_memory *mem = buf->priv;
> @@ -310,19 +273,6 @@ static int __videobuf_iolock(struct videobuf_queue *q,
> return 0;
> }
>
> -static int __videobuf_sync(struct videobuf_queue *q,
> - struct videobuf_buffer *buf)
> -{
> - struct videobuf_dma_contig_memory *mem = buf->priv;
> - BUG_ON(!mem);
> - MAGIC_CHECK(mem->magic, MAGIC_DC_MEM);
> -
> - dma_sync_single_for_cpu(q->dev, mem->dma_handle, mem->size,
> - DMA_FROM_DEVICE);
> -
> - return 0;
> -}
> -
> static int __videobuf_mmap_mapper(struct videobuf_queue *q,
> struct videobuf_buffer *buf,
> struct vm_area_struct *vma)
> @@ -331,8 +281,6 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
> struct videobuf_mapping *map;
> int retval;
> unsigned long size;
> - unsigned long pos, start = vma->vm_start;
> - struct page *page;
>
> dev_dbg(q->dev, "%s\n", __func__);
>
> @@ -359,43 +307,16 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
> size = vma->vm_end - vma->vm_start;
> size = (size < mem->size) ? size : mem->size;
>
> - if (!mem->cached) {
> - vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> - retval = remap_pfn_range(vma, vma->vm_start,
> - mem->dma_handle >> PAGE_SHIFT,
> + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> + retval = remap_pfn_range(vma, vma->vm_start,
> + mem->dma_handle >> PAGE_SHIFT,
> size, vma->vm_page_prot);
> - if (retval) {
> - dev_err(q->dev, "mmap: remap failed with error %d. ",
> - retval);
> - dma_free_coherent(q->dev, mem->size,
> - mem->vaddr, mem->dma_handle);
> - goto error;
> - }
> - } else {
> - pos = (unsigned long)mem->vaddr;
> -
> - while (size > 0) {
> - page = virt_to_page((void *)pos);
> - if (NULL == page) {
> - dev_err(q->dev, "mmap: virt_to_page failed\n");
> - __videobuf_dc_free(q->dev, mem);
> - goto error;
> - }
> - retval = vm_insert_page(vma, start, page);
> - if (retval) {
> - dev_err(q->dev, "mmap: insert failed with error %d\n",
> - retval);
> - __videobuf_dc_free(q->dev, mem);
> - goto error;
> - }
> - start += PAGE_SIZE;
> - pos += PAGE_SIZE;
> -
> - if (size > PAGE_SIZE)
> - size -= PAGE_SIZE;
> - else
> - size = 0;
> - }
> + if (retval) {
> + dev_err(q->dev, "mmap: remap failed with error %d. ",
> + retval);
> + dma_free_coherent(q->dev, mem->size,
> + mem->vaddr, mem->dma_handle);
> + goto error;
> }
>
> vma->vm_ops = &videobuf_vm_ops;
> @@ -417,17 +338,8 @@ error:
>
> static struct videobuf_qtype_ops qops = {
> .magic = MAGIC_QTYPE_OPS,
> - .alloc_vb = __videobuf_alloc_uncached,
> - .iolock = __videobuf_iolock,
> - .mmap_mapper = __videobuf_mmap_mapper,
> - .vaddr = __videobuf_to_vaddr,
> -};
> -
> -static struct videobuf_qtype_ops qops_cached = {
> - .magic = MAGIC_QTYPE_OPS,
> - .alloc_vb = __videobuf_alloc_cached,
> + .alloc_vb = __videobuf_alloc,
> .iolock = __videobuf_iolock,
> - .sync = __videobuf_sync,
> .mmap_mapper = __videobuf_mmap_mapper,
> .vaddr = __videobuf_to_vaddr,
> };
> @@ -447,20 +359,6 @@ void videobuf_queue_dma_contig_init(struct videobuf_queue *q,
> }
> EXPORT_SYMBOL_GPL(videobuf_queue_dma_contig_init);
>
> -void videobuf_queue_dma_contig_init_cached(struct videobuf_queue *q,
> - const struct videobuf_queue_ops *ops,
> - struct device *dev,
> - spinlock_t *irqlock,
> - enum v4l2_buf_type type,
> - enum v4l2_field field,
> - unsigned int msize,
> - void *priv, struct mutex *ext_lock)
> -{
> - videobuf_queue_core_init(q, ops, dev, irqlock, type, field, msize,
> - priv, &qops_cached, ext_lock);
> -}
> -EXPORT_SYMBOL_GPL(videobuf_queue_dma_contig_init_cached);
> -
> dma_addr_t videobuf_to_dma_contig(struct videobuf_buffer *buf)
> {
> struct videobuf_dma_contig_memory *mem = buf->priv;
> diff --git a/include/media/videobuf-dma-contig.h b/include/media/videobuf-dma-contig.h
> index f473aeb..f0ed825 100644
> --- a/include/media/videobuf-dma-contig.h
> +++ b/include/media/videobuf-dma-contig.h
> @@ -26,16 +26,6 @@ void videobuf_queue_dma_contig_init(struct videobuf_queue *q,
> void *priv,
> struct mutex *ext_lock);
>
> -void videobuf_queue_dma_contig_init_cached(struct videobuf_queue *q,
> - const struct videobuf_queue_ops *ops,
> - struct device *dev,
> - spinlock_t *irqlock,
> - enum v4l2_buf_type type,
> - enum v4l2_field field,
> - unsigned int msize,
> - void *priv,
> - struct mutex *ext_lock);
> -
> dma_addr_t videobuf_to_dma_contig(struct videobuf_buffer *buf);
> void videobuf_dma_contig_free(struct videobuf_queue *q,
> struct videobuf_buffer *buf);
>
Em Wed, 17 Apr 2013 09:22:16 -0300
Mauro Carvalho Chehab <[email protected]> escreveu:
> vm_iomap_memory() provides a better end user interface than
> remap_pfn_range(), as it does the needed tests before doing
> mmap.
>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> ---
> drivers/media/v4l2-core/videobuf-dma-contig.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c b/drivers/media/v4l2-core/videobuf-dma-contig.c
> index 67f572c..7e6b209 100644
> --- a/drivers/media/v4l2-core/videobuf-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf-dma-contig.c
> @@ -303,14 +303,9 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
> goto error;
>
> /* Try to remap memory */
> -
> size = vma->vm_end - vma->vm_start;
> - size = (size < mem->size) ? size : mem->size;
> -
> vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> - retval = remap_pfn_range(vma, vma->vm_start,
> - mem->dma_handle >> PAGE_SHIFT,
> - size, vma->vm_page_prot);
> + retval = vm_iomap_memory(vma, vma->vm_start, size);
Just to be sure, that changing from remap_pfn_range() to io_remap_pfn_range()
won't cause any side-effects, I double-checked that, for all drivers using
this code, that remap_pfn_range is equal to io_remap_pfn_range.
The Timberdale driver was a little trickier to check, as VIDEO_TIMBERDALE
doesn't depend on any architecture. However, this driver was submitted and
was known to work only on the Intel in-Vehicle Infotainment reference board
russelville. According with http://wiki.meego.com/In-vehicle, the
architecture for it is x86 (Intel Atom Z5xx).
In summary, those are the archs where this core driver is used, with the
corresponding drivers that make such usage:
powerpc:
drivers/media/platform/fsl-viu.c
arm:
drivers/media/platform/omap/omap_vout.c
drivers/media/platform/omap/omap_vout_vrfb.c
drivers/media/platform/soc_camera/mx1_camera.c
drivers/media/platform/soc_camera/omap1_camera.c
sh:
drivers/media/platform/sh_vou.c
x86:
drivers/media/platform/timblogiw.c
--
Cheers,
Mauro
On Wed, Apr 17, 2013 at 4:34 AM, Tomi Valkeinen <[email protected]> wrote:
>
> Should there be a similar helper that uses remap_pfn_range() instead of
> io_remap_pfn_range()?
The two are practically identical (*). I went back-and-forth over
which one to use, and ended up using io_remap_pfn_range() because that
ends up being the fancier one in a few special cases.
And it's actually about 50/50 whether people use that function on RAM
or on MMIO memory. Some people use it for DMA buffers, some people use
it for memory-mapped PCI memory, so the whole confusion about naming
ends up being double. We probably should get rid of
"io_remap_pfn_range()" entirely, since the real issue ends up being
what page protection bits to use, and drivers do that outside of this
function.
So you can use the new helper function to convert things that use
remap_pfn_range() too. The differences end up mattering only for
either highmem RAM pages (which get used for /dev/mem, but not for the
normal drivers) or for actual memory-mapped IO, in which case the
io_remap_pfn_range() function does a bit more.
Linus
(*) io_remap_pfn_page() is the "extended" version that takes care of
some special magical details on a couple of odd architectures, notably
sparc (but also one special case of MIPS PAE that have some magic bit
tricks). But even for MIPS and Sparc, it ends up devolving to be the
same as the "regular" remap_pfn_page() for normal memory. I'm adding
David and Ralf to the cc just to verify that I read it right, but
everybody else just defines "io_remap_pfn_range" to be
"remap_pfn_range".
From: Linus Torvalds <[email protected]>
Date: Wed, 17 Apr 2013 07:44:33 -0700
> (*) io_remap_pfn_page() is the "extended" version that takes care of
> some special magical details on a couple of odd architectures, notably
> sparc (but also one special case of MIPS PAE that have some magic bit
> tricks). But even for MIPS and Sparc, it ends up devolving to be the
> same as the "regular" remap_pfn_page() for normal memory. I'm adding
> David and Ralf to the cc just to verify that I read it right, but
> everybody else just defines "io_remap_pfn_range" to be
> "remap_pfn_range".
Yeah, the only thing special we do on sparc is interpret the PFN
specially. We munge it into the real physical address and then
pass it all down to remap_pfn_range() to do the real work.
We used to have a crazy special version of this entire routine
on sparc64 that tried to create huge TLB mappings, but that got
killed quite some time ago:
commit 3e37fd3153ac95088a74f5e7c569f7567e9f993a
Author: David S. Miller <[email protected]>
Date: Thu Nov 17 18:17:59 2011 -0800
sparc: Kill custom io_remap_pfn_range().
To handle the large physical addresses, just make a simple wrapper
around remap_pfn_range() like MIPS does.
Signed-off-by: David S. Miller <[email protected]>
On Wed, Apr 17, 2013 at 10:11 AM, David Miller <[email protected]> wrote:
>
> Yeah, the only thing special we do on sparc is interpret the PFN
> specially. We munge it into the real physical address and then
> pass it all down to remap_pfn_range() to do the real work.
So the main thing I want to check is that *if* it's given a regular
RAM physical address, it still works?
Some drivers basically allocate DMA memory and then pass on the
resulting physical address to this. Others pass in the PCI BAR
addresses etc. And some try to use "remap_pfn_range()", and others try
to use "io_remap_pfn_range()", and quite frankly, from what I can tell
we can just always use the "io_" version because it ends up being a
proper superset.
I'm pretty sure it works fine the way I read it, but I'm just verifying..
Linus
From: Linus Torvalds <[email protected]>
Date: Wed, 17 Apr 2013 10:20:43 -0700
> On Wed, Apr 17, 2013 at 10:11 AM, David Miller <[email protected]> wrote:
>>
>> Yeah, the only thing special we do on sparc is interpret the PFN
>> specially. We munge it into the real physical address and then
>> pass it all down to remap_pfn_range() to do the real work.
>
> So the main thing I want to check is that *if* it's given a regular
> RAM physical address, it still works?
>
> Some drivers basically allocate DMA memory and then pass on the
> resulting physical address to this. Others pass in the PCI BAR
> addresses etc. And some try to use "remap_pfn_range()", and others try
> to use "io_remap_pfn_range()", and quite frankly, from what I can tell
> we can just always use the "io_" version because it ends up being a
> proper superset.
>
> I'm pretty sure it works fine the way I read it, but I'm just verifying..
All we do is take the top 4 bits and shift them down into the
resulting physical address.
For normal RAM these bits will be clear, so it should all work out.
Passing PCI BARs into these routines is illegal, we have proper
abstractions for mmap()'ing PCI resources via pci_mmap_page_range()
et al.
So, any code doing that needs to be fixed.
On Wed, Apr 17, 2013 at 10:27 AM, David Miller <[email protected]> wrote:
>
> Passing PCI BARs into these routines is illegal, we have proper
> abstractions for mmap()'ing PCI resources via pci_mmap_page_range()
> et al.
>
> So, any code doing that needs to be fixed.
Hmm. I've definitely seen code that does that (well, it's not PCI, but
HPET, which is the same kind of "device mapped into memory rather than
RAM"), but the one example I definitely know of is x86-specific, where
it doesn't matter if it's IO memory or RAM.
I suspect there may be other drivers our there that do it, for the
simple reason that "it works on x86". And it's almost certainly
hardware that anybody with a sparc machine will never ever care about,
so I wouldn't worry too much about it ;)
(Same goes for things like s390 etc, which has a large comment about
"io_remap_pfn_range()" not being something they can support).
So in practice I suspect it's moot. The normal uses are about DMA
kernel allocations, and the cases where that isn't the case they might
not work on some architectures.
Linus
On Wed, Apr 17, 2013 at 2:15 AM, Arnd Bergmann <[email protected]> wrote:
>
> I took a look at the hpet_mmap function, which still contains this check:
>
> if (((vma->vm_end - vma->vm_start) != PAGE_SIZE) || vma->vm_pgoff)
> return -EINVAL;
>
> As far as I can tell, this check is implied by the new code in
> vm_iomap_memory as the len argument passed here is PAGE_SIZE, so you
> can remove another three lines in hpet_mmap.
Not the way things are now.
vm_iomap_memory() actually allows non-page-aligned things to be
mapped, with the assumption that the user will then know about the
internal offsets.
The *reason* for that is questionable, but that's how pretty much
every single user I've seen has worked, throwing the low bits of the
physical away (after adding them to the length of the area).
Now, I sincerely *hope* that there are no users of "let's mmap this
non-page-aligned thing and let people access the data around it", but
I didn't want to break things that I didn't know about, and that I
couldn't test.
The HPET case was the only one (admittedly of the very limited cases I
looked at and converted) that actually checked alignment, so I left
that part in.
It may be that I should have done things differently: make the normal
helper function verify page alignment, and warn if it's missing. Then,
we could have a "vm_unaligned_iomap_memory()" that would just do the
"extend to aligned pages" that people could convert any odd users for.
That would probably be a good thing to do, but it would be separate
"phase two" from the "let's start using the sane helper".
Linus
On Wednesday 17 April 2013, Linus Torvalds wrote:
> Not the way things are now.
>
> vm_iomap_memory() actually allows non-page-aligned things to be
> mapped, with the assumption that the user will then know about the
> internal offsets.
>
> The reason for that is questionable, but that's how pretty much
> every single user I've seen has worked, throwing the low bits of the
> physical away (after adding them to the length of the area).
There is a separate check for the physical address that gets
mapped in hpet_mmap:
if (addr & (PAGE_SIZE - 1))
return -ENOSYS;
We cannot remove that without changing the semantics of this function,
but the check that I mentioned:
if (((vma->vm_end - vma->vm_start) != PAGE_SIZE) || vma->vm_pgoff)
return -EINVAL;
is for the virtual address. All of vm_start, vm_end and vm_pgoff
are guaranteed to be page-aligned through previous checks or
shifts, and we have also checked that the size is non-zero.
Since we pass a hardcoded len=PAGE_SIZE into vm_iomap_memory, that will
return -EINVAL for any non-zero vma->vm_pgoff. Testing ((vma->vm_end -
vma->vm_start) != PAGE_SIZE) is redundant as well, because we know it
is a positive multiple of PAGE_SIZE because of the call chain leading
up to this function, and vm_iomap_memory() ensures that it can not
be more than len, which leaves PAGE_SIZE as the only possible value
not resulting in -EINVAL without the extra check.
> It may be that I should have done things differently: make the normal
> helper function verify page alignment, and warn if it's missing. Then,
> we could have a "vm_unaligned_iomap_memory()" that would just do the
> "extend to aligned pages" that people could convert any odd users for.
> That would probably be a good thing to do, but it would be separate
> "phase two" from the "let's start using the sane helper".
Makes sense, but I think this is independent of the observation I made
regarding the checks for the vma.
Arnd
On Wed, Apr 17, 2013 at 2:28 PM, Arnd Bergmann <[email protected]> wrote:
>
> There is a separate check for the physical address that gets
> mapped in hpet_mmap:
Ahh, right you are. The earlier check for PAGE_SIZE's mapping and zero
offset is indeed superfluous.
Linus
On Tue, Apr 16, 2013 at 8:12 PM, Linus Torvalds
<[email protected]> wrote:
> Guys, I just pushed out a new helper function intended for cleaning up
> various device driver mmap functions, because they are rather messy,
> and at least part of the problem was the bad impedance between what a
> driver author would want to have, and the VM interfaces to map a
> memory range into user space with mmap.
I have not had a detailed look yet (am at LSF/MM workshop right now).
Just a suggestion: when file->f_op->mmap returns an error code,
mmap_region() currently has to call unmap_region() to undo any partial
mappings that might have been created by the device driver. Would it
make more sense to ask that the few drivers that create such messes to
clean up after themselves on their error paths ?
--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
On Fri, Apr 19, 2013 at 8:43 AM, Michel Lespinasse <[email protected]> wrote:
>
> Just a suggestion: when file->f_op->mmap returns an error code,
> mmap_region() currently has to call unmap_region() to undo any partial
> mappings that might have been created by the device driver. Would it
> make more sense to ask that the few drivers that create such messes to
> clean up after themselves on their error paths ?
No. We basically want to assume the least competence humanly
(simianly?) possible from driver writers. The problem the helper is
trying to solve is exactly the fact that driver writers shouldn't have
to even know about all the subtleties with page offsets etc.
So if a driver returns an error code, we should assume they screwed up
potentially half-way and clean up. We should *not* assume that we
don't need to.
Linus
Hi Mauro,
On Wed, Apr 17, 2013 at 07:43:00AM -0300, Mauro Carvalho Chehab wrote:
> and a camera anymore. The OMAP2 were used on some Nokia phones.
> They used to maintain that code, but now that they moved to the dark
> side of the moon, they lost their interests on it. So, it may not
> be easily find testers for patches there.
There's one more underlying issue there than potentially having both no-one
with the device and time to test it: the driver does not function as-is in
mainline (nor any recent non-mainline kernel either). Quite some work would
be required to update it (both to figure out why the whole system crashes
when trying to capture images and change the driver use modern APIs). A
small while back we decided to still keep the driver in the tree:
<URL:http://www.spinics.net/lists/linux-media/msg56237.html>
(The rest of the discussion took place in #v4l AFAIR.)
So, what could be done now is either 1) write a patch that changes the
driver to use the right API and take a risk of adding one more bug to the
driver; or 2) remove the driver now and bring it back only if someone really
has time to make it work first.
--
Kind regards,
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]