Re: [uClinux-dev] [PATCH] NOMMU: use copy_*_user_page() in access_process_vm()

[Jie Zhang <jie.zhang@analog.com>]

On 11/25/2009 07:49 PM, Jamie Lokier wrote:
> Jie Zhang wrote:
>> On 11/25/2009 02:16 PM, Jamie Lokier wrote:
>>> Mike Frysinger wrote:
>>>> From: Jie Zhang<jie.zhang@analog.com>
>>>>
>>>> The mmu code uses the copy_*_user_page() variants in access_process_vm()
>>>> rather than copy_*_user() as the former includes an icache flush.  This is
>>>> important when doing things like setting software breakpoints with gdb.
>>>> So switch the nommu code over to do the same.
>>>
>>> Reasonable, but it's a bit subtle don't you think?
>>> How about a one-line comment saying why it's using copy_*_user_page()?
>>>
>>> (If it was called copy_*_user_flush_icache() I wouldn't say anything,
>>> but it isn't).
>>>
>> But I think it's well known in Linux kernel developers that
>> copy_to_user_page and copy_from_user_page should do cache flushing. It's
>> documented in Documentation/cachetlb.txt. I don't think it's necessary
>> to replicate it here.
>
> You're right, however I now think the commit message is misleading.
>
> Since this is the *only place in the entire kernel* where these
> functions are used (plus the mmu equivalent), I'm not sure I'd agree
> about well known, and the name could be better (copy_*_user_ptrace()),
> but I agree now, it doesn't need a comment.
>
> It was the talk of icache flush which bothered me, as I (wrongly)
> assumed copy_*_user_page() was used elsewhere, without knowledge of
> icache vs non-icache differences - which are often the responsibility
> of userspace to get right, so often the kernel does not care.
>
> In fact, it's not just icache.  copy_*_user_page() has to do some
> *data* cache flushing too, on some architecures.  For example, see

You are right. We needs dcache flushing here, too. However, for harvard 
architecture, flushing icache implies you have to flush dcache. So in 
implementation, there is dcache flushing in flush_icache_range, as we do 
for Blackfin, as well as they do for ARM. So when we say icache 
flushing, dcache flushing is implied.

> I'm not sure why I don't see the same dcache flushing on ARM, so I
> wonder if the ARM implementation of these buggy.
>
I'm not familiar with ARM. But I believe they do dcache flushing after 
some grepping in arch/arm/*.

> Which is why, given they are only used for ptrace (have just grepped),
> I'm inclined to think it'd be clearer to rename the functions to
> copy_*_user_ptrace().  And make your no-mmu change of course :-)
> Any thoughts on the rename?
>
I have no opinion on renaming things. I can live with the current naming.


Jie