Messages in this thread | | | Date | Tue, 6 Dec 2005 20:35:02 +0000 | From | Nick Holloway <> | Subject | Re: [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/
| |