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' | [email protected]
// ^ \\ | http://www.pyrites.org.uk/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
