lkml.org 
[lkml]   [2005]   [Dec]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2.6.15-rc4 1/1] cpia: use vm_insert_page() instead of remap_pfn_range()
On Tue, Dec 06, 2005 at 06:31:26PM +0000, Hugh Dickins wrote:
> On Mon, 5 Dec 2005, Nick Holloway wrote:
> > Although the cpia driver functioned correctly after printing out the
> > "incomplete pfn remapping" message, I thought I would have a go at the
> > trivial conversion'' as I have access to the hardware.
>
> A couple of minor points which you may reasonably feel go beyond
> what you were attempting:
>
> rvfree becomes totally pointless (since vfree checks for NULL itself),
> so it would be better to delete rvfree and replace the rvfree calls
> by vfree calls (removing the size argument).

I could see that would be the next step in the cleanup, but I wanted to
perform the minumum changes, so it was clear what I had done.

> pos would be better off as a u8* matching frame_buf, than an unsigned
> long that has to be cast this way and that.

I agree. I couldn't see any logical reason for dealing with it as
"unsigned long", and wondered about switching to "void*". On the other
hand, the machine this was being tested on was remote, and the scope
for a bad change that locked up would have halted development.

> And a third point which makes me hesitate. It used to set PAGE_SHARED
> (read-write access) on all the page table entries, whether the mmap
> was MAP_PRIVATE or MAP_SHARED, PROT_WRITE or not. That was wrong, and
> Linus intentionally does not permit that mistake with vm_insert_page.
>
> And the change _probably_ causes no trouble for anyone; but it _might_
> cause trouble somewhere: it could be that userspace needs correcting
> (to ask for shared write access where it wasn't asking before).
> I've no idea whether write access makes sense with this driver.

I did wonder about that too. The application I tested with does:

mmap(..., PROT_READ|PROT_WRITE, MAP_SHARED, ... );

This seems to be a common incantation for video4linux clients. It would
also seem to be the wrong thing for the mmap to not be MAP_SHARED.

> So personally I'm rather reluctant to recommend a change of this kind
> late in a release cycle (and I'd prefer that you didn't have to endure
> the noisy warnings at this stage too, but Linus put them in,
> so I think he wants them to stay).

I'm not concerned with the warnings -- I just fancied a quick kernel hack,
and had the hardware to test.

--
`O O' | Nick.Holloway@pyrites.org.uk
// ^ \\ | http://www.pyrites.org.uk/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-12-06 21:40    [W:0.758 / U:1.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site