Re: Device driver memory 'mmap()' function helper cleanup

[Takashi Iwai <tiwai@suse.de>]

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 <tiwai@suse.de>


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