Message-ID: <CA+55aFyK2EEPuBPrqu3AGRbW+8TdP=kLLz4opvynNRcrSWC2ww@mail.gmail.com>
Date:	Tue, 16 Apr 2013 20:12:32 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Clemens Ladisch <clemens@...isch.de>,
	Arnd Bergmann <arnd@...db.de>, Takashi Iwai <tiwai@...e.de>,
	Mauro Carvalho Chehab <mchehab@...hat.com>
Subject: Device driver memory 'mmap()' function helper cleanup

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

Download attachment "patch.diff" of type "application/octet-stream" (6437 bytes)

Powered by blists - more mailing lists