[PATCH 1/2] ARM: remove unneeded check of the cache_is_vipt_nonaliasing()

[PATCH 1/2] ARM: remove unneeded check of the cache_is_vipt_nonaliasing()

[Saeed Bishara]

Signed-off-by: Saeed Bishara <saeed@marvell.com>
---
 arch/arm/mm/flush.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
index 2b269c9..f1b7998 100644
--- a/arch/arm/mm/flush.c
+++ b/arch/arm/mm/flush.c
@@ -253,8 +253,8 @@ void __sync_icache_dcache(pte_t pteval)
 
 	if (!test_and_set_bit(PG_dcache_clean, &page->flags))
 		__flush_dcache_page(mapping, page);
-	/* pte_exec() already checked above for non-aliasing VIPT cache */
-	if (cache_is_vipt_nonaliasing() || pte_exec(pteval))
+
+	if (pte_exec(pteval))
 		__flush_icache_all();
 }
 #endif
-- 
1.7.1

[PATCH 2/2] ARM: fix the note about dcache lazy flushing for SMP systems

[Saeed Bishara]

Signed-off-by: Saeed Bishara <saeed@marvell.com>
---
 arch/arm/mm/flush.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
index f1b7998..e632363 100644
--- a/arch/arm/mm/flush.c
+++ b/arch/arm/mm/flush.c
@@ -275,7 +275,8 @@ void __sync_icache_dcache(pte_t pteval)
  *  kernel cache lines for later.  Otherwise, we assume we have
  *  aliasing mappings.
  *
- * Note that we disable the lazy flush for SMP.
+ * Note that we disable the lazy flush for SMP configurations where
+ * the cache maintenance operations are not automatically broadcasted
  */
 void flush_dcache_page(struct page *page)
 {
-- 
1.7.1

[PATCH 2/2] ARM: fix the note about dcache lazy flushing for SMP systems

[saeed bishara]

On Thu, May 12, 2011 at 9:33 PM, Saeed Bishara <saeed@marvell.com> wrote:
>
> Signed-off-by: Saeed Bishara <saeed@marvell.com>
> ---
> ?arch/arm/mm/flush.c | ? ?3 ++-
> ?1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
> index f1b7998..e632363 100644
> --- a/arch/arm/mm/flush.c
> +++ b/arch/arm/mm/flush.c
> @@ -275,7 +275,8 @@ void __sync_icache_dcache(pte_t pteval)
> ?* ?kernel cache lines for later. ?Otherwise, we assume we have
> ?* ?aliasing mappings.
> ?*
> - * Note that we disable the lazy flush for SMP.
> + * Note that we disable the lazy flush for SMP configurations where
> + * the cache maintenance operations are not automatically broadcasted
> ?*/
> ?void flush_dcache_page(struct page *page)
> ?{
can you please review this one?
also, the rest of this function documentation is not coherent with the
code itself any more, for example, this function flushes the dcache to
prevent I cache incoherency.
can you please updated the comments to reflect what this function actually does?

saeed

[PATCH 2/2] ARM: fix the note about dcache lazy flushing for SMP systems

[Catalin Marinas]

On 12 May 2011 19:33, Saeed Bishara <saeed@marvell.com> wrote:
> diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
> index f1b7998..e632363 100644
> --- a/arch/arm/mm/flush.c
> +++ b/arch/arm/mm/flush.c
> @@ -275,7 +275,8 @@ void __sync_icache_dcache(pte_t pteval)
> ?* ?kernel cache lines for later. ?Otherwise, we assume we have
> ?* ?aliasing mappings.
> ?*
> - * Note that we disable the lazy flush for SMP.
> + * Note that we disable the lazy flush for SMP configurations where
> + * the cache maintenance operations are not automatically broadcasted

(I think "broadcast" rather than "broadcasted" would be better, though
I'm not a native English speaker. And a full stop at the end)

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH 1/2] ARM: remove unneeded check of the cache_is_vipt_nonaliasing()

[saeed bishara]

On Thu, May 12, 2011 at 9:33 PM, Saeed Bishara <saeed@marvell.com> wrote:
>
> Signed-off-by: Saeed Bishara <saeed@marvell.com>
> ---
> ?arch/arm/mm/flush.c | ? ?4 ++--
> ?1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
> index 2b269c9..f1b7998 100644
> --- a/arch/arm/mm/flush.c
> +++ b/arch/arm/mm/flush.c
> @@ -253,8 +253,8 @@ void __sync_icache_dcache(pte_t pteval)
>
> ? ? ? ?if (!test_and_set_bit(PG_dcache_clean, &page->flags))
> ? ? ? ? ? ? ? ?__flush_dcache_page(mapping, page);
> - ? ? ? /* pte_exec() already checked above for non-aliasing VIPT cache */
> - ? ? ? if (cache_is_vipt_nonaliasing() || pte_exec(pteval))
> +
> + ? ? ? if (pte_exec(pteval))
> ? ? ? ? ? ? ? ?__flush_icache_all();
> ?}
> ?#endif
can you have I a look at this patch?
the __sync_icache_dcache() returns if (cache_is_vipt_nonaliasing() &&
!pte_exec(pteval)), so later, the if (cache_is_vipt_nonaliasing() ||
pte_exec(pteval)) should be equivalent to  if (pte_exec(pteval))

saeed

[PATCH 1/2] ARM: remove unneeded check of the cache_is_vipt_nonaliasing()

[Catalin Marinas]

On 15 May 2011 15:48, saeed bishara <saeed.bishara@gmail.com> wrote:
> On Thu, May 12, 2011 at 9:33 PM, Saeed Bishara <saeed@marvell.com> wrote:
>>
>> Signed-off-by: Saeed Bishara <saeed@marvell.com>
>> ---
>> ?arch/arm/mm/flush.c | ? ?4 ++--
>> ?1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
>> index 2b269c9..f1b7998 100644
>> --- a/arch/arm/mm/flush.c
>> +++ b/arch/arm/mm/flush.c
>> @@ -253,8 +253,8 @@ void __sync_icache_dcache(pte_t pteval)
>>
>> ? ? ? ?if (!test_and_set_bit(PG_dcache_clean, &page->flags))
>> ? ? ? ? ? ? ? ?__flush_dcache_page(mapping, page);
>> - ? ? ? /* pte_exec() already checked above for non-aliasing VIPT cache */
>> - ? ? ? if (cache_is_vipt_nonaliasing() || pte_exec(pteval))
>> +
>> + ? ? ? if (pte_exec(pteval))
>> ? ? ? ? ? ? ? ?__flush_icache_all();
>> ?}
>> ?#endif
> can you have I a look at this patch?
> the __sync_icache_dcache() returns if (cache_is_vipt_nonaliasing() &&
> !pte_exec(pteval)), so later, the if (cache_is_vipt_nonaliasing() ||
> pte_exec(pteval)) should be equivalent to ?if (pte_exec(pteval))

Your patch looks fine - when cache_is_vipt_nonaliasing(), we always
have pte_exec() true at the end of this function, so no need for the
additional check.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH 1/2] ARM: remove unneeded check of the cache_is_vipt_nonaliasing()

[saeed bishara]

>>> Signed-off-by: Saeed Bishara <saeed@marvell.com>
>>> ---
>>> ?arch/arm/mm/flush.c | ? ?4 ++--
>>> ?1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
>>> index 2b269c9..f1b7998 100644
>>> --- a/arch/arm/mm/flush.c
>>> +++ b/arch/arm/mm/flush.c
>>> @@ -253,8 +253,8 @@ void __sync_icache_dcache(pte_t pteval)
>>>
>>> ? ? ? ?if (!test_and_set_bit(PG_dcache_clean, &page->flags))
>>> ? ? ? ? ? ? ? ?__flush_dcache_page(mapping, page);
>>> - ? ? ? /* pte_exec() already checked above for non-aliasing VIPT cache */
>>> - ? ? ? if (cache_is_vipt_nonaliasing() || pte_exec(pteval))
>>> +
>>> + ? ? ? if (pte_exec(pteval))
>>> ? ? ? ? ? ? ? ?__flush_icache_all();
>>> ?}
>>> ?#endif
>> can you have I a look at this patch?
>> the __sync_icache_dcache() returns if (cache_is_vipt_nonaliasing() &&
>> !pte_exec(pteval)), so later, the if (cache_is_vipt_nonaliasing() ||
>> pte_exec(pteval)) should be equivalent to ?if (pte_exec(pteval))
>
> Your patch looks fine - when cache_is_vipt_nonaliasing(), we always
> have pte_exec() true at the end of this function, so no need for the
> additional check.
>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
thanks, can you please merge it to your tree.
saeed

[PATCH 1/2] ARM: remove unneeded check of the cache_is_vipt_nonaliasing()

[Catalin Marinas]

On 16 May 2011 08:39, saeed bishara <saeed.bishara@gmail.com> wrote:
>>>> Signed-off-by: Saeed Bishara <saeed@marvell.com>
>>>> ---
>>>> ?arch/arm/mm/flush.c | ? ?4 ++--
>>>> ?1 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
>>>> index 2b269c9..f1b7998 100644
>>>> --- a/arch/arm/mm/flush.c
>>>> +++ b/arch/arm/mm/flush.c
>>>> @@ -253,8 +253,8 @@ void __sync_icache_dcache(pte_t pteval)
>>>>
>>>> ? ? ? ?if (!test_and_set_bit(PG_dcache_clean, &page->flags))
>>>> ? ? ? ? ? ? ? ?__flush_dcache_page(mapping, page);
>>>> - ? ? ? /* pte_exec() already checked above for non-aliasing VIPT cache */
>>>> - ? ? ? if (cache_is_vipt_nonaliasing() || pte_exec(pteval))
>>>> +
>>>> + ? ? ? if (pte_exec(pteval))
>>>> ? ? ? ? ? ? ? ?__flush_icache_all();
>>>> ?}
>>>> ?#endif
>>> can you have I a look at this patch?
>>> the __sync_icache_dcache() returns if (cache_is_vipt_nonaliasing() &&
>>> !pte_exec(pteval)), so later, the if (cache_is_vipt_nonaliasing() ||
>>> pte_exec(pteval)) should be equivalent to ?if (pte_exec(pteval))
>>
>> Your patch looks fine - when cache_is_vipt_nonaliasing(), we always
>> have pte_exec() true at the end of this function, so no need for the
>> additional check.
>>
>> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>
> thanks, can you please merge it to your tree.

I could but it may be better for you to just upload it to Russell's
patch system (I'm not planning to send any pull requests to Russell,
apart from LPAE).

-- 
Catalin

[PATCH 1/2] ARM: remove unneeded check of the cache_is_vipt_nonaliasing()

[saeed bishara]

On Mon, May 16, 2011 at 11:38 AM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On 16 May 2011 08:39, saeed bishara <saeed.bishara@gmail.com> wrote:
>>>>> Signed-off-by: Saeed Bishara <saeed@marvell.com>
>>>>> ---
>>>>> ?arch/arm/mm/flush.c | ? ?4 ++--
>>>>> ?1 files changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
>>>>> index 2b269c9..f1b7998 100644
>>>>> --- a/arch/arm/mm/flush.c
>>>>> +++ b/arch/arm/mm/flush.c
>>>>> @@ -253,8 +253,8 @@ void __sync_icache_dcache(pte_t pteval)
>>>>>
>>>>> ? ? ? ?if (!test_and_set_bit(PG_dcache_clean, &page->flags))
>>>>> ? ? ? ? ? ? ? ?__flush_dcache_page(mapping, page);
>>>>> - ? ? ? /* pte_exec() already checked above for non-aliasing VIPT cache */
>>>>> - ? ? ? if (cache_is_vipt_nonaliasing() || pte_exec(pteval))
>>>>> +
>>>>> + ? ? ? if (pte_exec(pteval))
>>>>> ? ? ? ? ? ? ? ?__flush_icache_all();
>>>>> ?}
>>>>> ?#endif
>>>> can you have I a look at this patch?
>>>> the __sync_icache_dcache() returns if (cache_is_vipt_nonaliasing() &&
>>>> !pte_exec(pteval)), so later, the if (cache_is_vipt_nonaliasing() ||
>>>> pte_exec(pteval)) should be equivalent to ?if (pte_exec(pteval))
>>>
>>> Your patch looks fine - when cache_is_vipt_nonaliasing(), we always
>>> have pte_exec() true at the end of this function, so no need for the
>>> additional check.
>>>
>>> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>>
>> thanks, can you please merge it to your tree.
>
> I could but it may be better for you to just upload it to Russell's
> patch system (I'm not planning to send any pull requests to Russell,
> apart from LPAE).
ok.
BTW, there is something concerns me about ARM11MP core, this system
doesn't support cache maintenance broadcasting, but when looking at
do_cache_op(), it doesn't seem to flush the cache of other cpus, so if
we have a multithreaded applications like java that uses self
modifying code, how can you make sure that all cpus will have their I
cache flushed when one cpu requests to do do_cache_op?

saeed

[PATCH 1/2] ARM: remove unneeded check of the cache_is_vipt_nonaliasing()

[Catalin Marinas]

On Mon, 2011-05-16 at 11:00 +0100, saeed bishara wrote:
> BTW, there is something concerns me about ARM11MP core, this system
> doesn't support cache maintenance broadcasting, but when looking at
> do_cache_op(), it doesn't seem to flush the cache of other cpus, so if
> we have a multithreaded applications like java that uses self
> modifying code, how can you make sure that all cpus will have their I
> cache flushed when one cpu requests to do do_cache_op?

We don't :). If you are unlucky enough, your thread may migrate to
another CPU between the moment it wrote the data and the do_cache_op()
call so you'll miss the D-cache flushing.

I had a simple fix for a while, see below (I posted it in the past in a
slightly different form, maybe I should post it again).


ARM: Allow lazy cache flushing on ARM11MPCore

From: Catalin Marinas <catalin.marinas@arm.com>

The ARM11MPCore doesn't broadcast the cache maintenance operations in
hardware, therefore the flush_dcache_page() currently performs the cache
flushing non-lazily. But since not all drivers call this function after
writing to a page cache page, the kernel needs a different approach like
using read-for-ownership on the CPU flushing the cache to force the
dirty cache lines migration from other CPUs. This way the cache flushing
operation can be done lazily via __sync_icache_dcache().

Since we cannot force read-for-ownership on the I-cache, the patch
invalidates the whole I-cache when a thread migrates to another CPU.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm/include/asm/mmu_context.h |    3 ++-
 arch/arm/mm/cache-v6.S             |    4 ++++
 arch/arm/mm/flush.c                |    3 +--
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/mmu_context.h b/arch/arm/include/asm/mmu_context.h
index 71605d9..d116a23 100644
--- a/arch/arm/include/asm/mmu_context.h
+++ b/arch/arm/include/asm/mmu_context.h
@@ -114,7 +114,8 @@ switch_mm(struct mm_struct *prev, struct mm_struct *next,
 #ifdef CONFIG_SMP
 	/* check for possible thread migration */
 	if (!cpumask_empty(mm_cpumask(next)) &&
-	    !cpumask_test_cpu(cpu, mm_cpumask(next)))
+	    (cache_ops_need_broadcast() ||
+	     !cpumask_test_cpu(cpu, mm_cpumask(next))))
 		__flush_icache_all();
 #endif
 	if (!cpumask_test_and_set_cpu(cpu, mm_cpumask(next)) || prev != next) {
diff --git a/arch/arm/mm/cache-v6.S b/arch/arm/mm/cache-v6.S
index c96fa1b..59bd8b94 100644
--- a/arch/arm/mm/cache-v6.S
+++ b/arch/arm/mm/cache-v6.S
@@ -177,6 +177,10 @@ ENDPROC(v6_coherent_kern_range)
 ENTRY(v6_flush_kern_dcache_area)
 	add	r1, r0, r1
 1:
+#ifdef CONFIG_SMP
+	/* no cache maintenance broadcasting on ARM11MPCore */
+	ldr	r2, [r0]			@ read for ownership
+#endif
 #ifdef HARVARD_CACHE
 	mcr	p15, 0, r0, c7, c14, 1		@ clean & invalidate D line
 #else
diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
index 2b269c9..7c98287 100644
--- a/arch/arm/mm/flush.c
+++ b/arch/arm/mm/flush.c
@@ -290,8 +290,7 @@ void flush_dcache_page(struct page *page)
 
 	mapping = page_mapping(page);
 
-	if (!cache_ops_need_broadcast() &&
-	    mapping && !mapping_mapped(mapping))
+	if (mapping && !mapping_mapped(mapping))
 		clear_bit(PG_dcache_clean, &page->flags);
 	else {
 		__flush_dcache_page(mapping, page);


-- 
Catalin

[PATCH 1/2] ARM: remove unneeded check of the cache_is_vipt_nonaliasing()

[Saeed Bishara]

 

>> BTW, there is something concerns me about ARM11MP core, this system
>> doesn't support cache maintenance broadcasting, but when looking at
>> do_cache_op(), it doesn't seem to flush the cache of other 
>cpus, so if
>> we have a multithreaded applications like java that uses self
>> modifying code, how can you make sure that all cpus will have their I
>> cache flushed when one cpu requests to do do_cache_op?
>
>We don't :). If you are unlucky enough, your thread may migrate to
>another CPU between the moment it wrote the data and the do_cache_op()
>call so you'll miss the D-cache flushing.
>
>I had a simple fix for a while, see below (I posted it in the past in a
>slightly different form, maybe I should post it again).
Well, this patch seems to handle the case were the thread migrated to another CPU.
But what about this case:
1. Thread 0 runs on CPU0
2. Thread 1 runs on CPU1 and run a code from A.
3. Thread 0 modifies the code at A
4. Thread 0 calls do_cache_op()
5. Thread 1 tries to run new code from A.
I'm assuming that thread 1 continued to run on CPU1 and no context switch occurred on that CPU.


saeed

[PATCH 1/2] ARM: remove unneeded check of the cache_is_vipt_nonaliasing()

[Catalin Marinas]

On Mon, 2011-05-16 at 13:13 +0100, Saeed Bishara wrote:
> >> BTW, there is something concerns me about ARM11MP core, this system
> >> doesn't support cache maintenance broadcasting, but when looking at
> >> do_cache_op(), it doesn't seem to flush the cache of other
> >cpus, so if
> >> we have a multithreaded applications like java that uses self
> >> modifying code, how can you make sure that all cpus will have their I
> >> cache flushed when one cpu requests to do do_cache_op?
> >
> >We don't :). If you are unlucky enough, your thread may migrate to
> >another CPU between the moment it wrote the data and the do_cache_op()
> >call so you'll miss the D-cache flushing.
> >
> >I had a simple fix for a while, see below (I posted it in the past in a
> >slightly different form, maybe I should post it again).
> 
> Well, this patch seems to handle the case were the thread migrated to another CPU.
> But what about this case:
> 1. Thread 0 runs on CPU0
> 2. Thread 1 runs on CPU1 and run a code from A.
> 3. Thread 0 modifies the code at A
> 4. Thread 0 calls do_cache_op()
> 5. Thread 1 tries to run new code from A.
> I'm assuming that thread 1 continued to run on CPU1 and no context
> switch occurred on that CPU.

First my patch needs another read-for-ownership in the
v6_coherent_user_range() function to handle the D-cache in case of
thread migrating just before do_cache_op().

But in the above scenario, D-cache would be handled however, the I-cache
won't be. So some kind of smp_call_function() is be needed if you have a
multi-threaded application that writes instructions in a thread and
executes them in another. If you do the cross calling, you don't even
need my initial patch (though that solves other things).

-- 
Catalin

[PATCH 1/2] ARM: remove unneeded check of the cache_is_vipt_nonaliasing()

[Russell King - ARM Linux]

On Mon, May 16, 2011 at 09:38:00AM +0100, Catalin Marinas wrote:
> On 16 May 2011 08:39, saeed bishara <saeed.bishara@gmail.com> wrote:
> >>>> Signed-off-by: Saeed Bishara <saeed@marvell.com>
> >>>> ---
> >>>> ?arch/arm/mm/flush.c | ? ?4 ++--
> >>>> ?1 files changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
> >>>> index 2b269c9..f1b7998 100644
> >>>> --- a/arch/arm/mm/flush.c
> >>>> +++ b/arch/arm/mm/flush.c
> >>>> @@ -253,8 +253,8 @@ void __sync_icache_dcache(pte_t pteval)
> >>>>
> >>>> ? ? ? ?if (!test_and_set_bit(PG_dcache_clean, &page->flags))
> >>>> ? ? ? ? ? ? ? ?__flush_dcache_page(mapping, page);
> >>>> - ? ? ? /* pte_exec() already checked above for non-aliasing VIPT cache */
> >>>> - ? ? ? if (cache_is_vipt_nonaliasing() || pte_exec(pteval))
> >>>> +
> >>>> + ? ? ? if (pte_exec(pteval))
> >>>> ? ? ? ? ? ? ? ?__flush_icache_all();
> >>>> ?}
> >>>> ?#endif
> >>> can you have I a look at this patch?
> >>> the __sync_icache_dcache() returns if (cache_is_vipt_nonaliasing() &&
> >>> !pte_exec(pteval)), so later, the if (cache_is_vipt_nonaliasing() ||
> >>> pte_exec(pteval)) should be equivalent to ?if (pte_exec(pteval))
> >>
> >> Your patch looks fine - when cache_is_vipt_nonaliasing(), we always
> >> have pte_exec() true at the end of this function, so no need for the
> >> additional check.
> >>
> >> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> >
> > thanks, can you please merge it to your tree.
> 
> I could but it may be better for you to just upload it to Russell's
> patch system (I'm not planning to send any pull requests to Russell,
> apart from LPAE).

It may also be a good idea to put some description into the commit comment
so people know why the change is being made.  Eg, the fact you had to
explain why in a follow up message before you got a response suggests that
omitting it meant that people had to read the code to work out why the
patch was being proposed.

[PATCH 1/2] ARM: remove unneeded check of the cache_is_vipt_nonaliasing()

[Saeed Bishara]

>> >>> can you have I a look at this patch?
>> >>> the __sync_icache_dcache() returns if 
>(cache_is_vipt_nonaliasing() &&
>> >>> !pte_exec(pteval)), so later, the if 
>(cache_is_vipt_nonaliasing() ||
>> >>> pte_exec(pteval)) should be equivalent to ?if (pte_exec(pteval))
>> >>
>> >> Your patch looks fine - when cache_is_vipt_nonaliasing(), 
>we always
>> >> have pte_exec() true at the end of this function, so no 
>need for the
>> >> additional check.
>> >>
>> >> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>> >
>> > thanks, can you please merge it to your tree.
>> 
>> I could but it may be better for you to just upload it to Russell's
>> patch system (I'm not planning to send any pull requests to Russell,
>> apart from LPAE).
>
>It may also be a good idea to put some description into the 
>commit comment
>so people know why the change is being made.  Eg, the fact you had to
>explain why in a follow up message before you got a response 
>suggests that
>omitting it meant that people had to read the code to work out why the
>patch was being proposed.
Ok what about this (added Catalin's explanation):
commit 1ae2a4434959b5c6b234fa5f6eb5ad9a8e0fba0c
Author: Saeed Bishara <saeed@marvell.com>
Date:   Thu May 12 19:45:25 2011 +0300

    ARM: remove unneeded check of the cache_is_vipt_nonaliasing()

    when cache_is_vipt_nonaliasing(), we always have pte_exec() true at
    the end of this function, so no need for the additional check.

    Acked-by: Catalin Marinas <catalin.marinas@arm.com>

diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
index 2b269c9..f1b7998 100644
--- a/arch/arm/mm/flush.c
+++ b/arch/arm/mm/flush.c
@@ -253,8 +253,8 @@ void __sync_icache_dcache(pte_t pteval)

        if (!test_and_set_bit(PG_dcache_clean, &page->flags))
                __flush_dcache_page(mapping, page);
-       /* pte_exec() already checked above for non-aliasing VIPT cache */
-       if (cache_is_vipt_nonaliasing() || pte_exec(pteval))
+
+       if (pte_exec(pteval))
                __flush_icache_all();
 }
 #endif
>

[PATCH 1/2] ARM: remove unneeded check of the cache_is_vipt_nonaliasing()

[Catalin Marinas]

On Mon, 2011-05-16 at 12:10 +0100, Saeed Bishara wrote:
> commit 1ae2a4434959b5c6b234fa5f6eb5ad9a8e0fba0c
> Author: Saeed Bishara <saeed@marvell.com>
> Date:   Thu May 12 19:45:25 2011 +0300
> 
>     ARM: remove unneeded check of the cache_is_vipt_nonaliasing()
> 
>     when cache_is_vipt_nonaliasing(), we always have pte_exec() true at
>     the end of this function, so no need for the additional check.
> 
>     Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Please add your Signed-off-by as well.

-- 
Catalin

[PATCH 1/2] ARM: remove unneeded check of the cache_is_vipt_nonaliasing()

[Saeed Bishara]

 

>-----Original Message-----
>From: Catalin Marinas [mailto:catalin.marinas at arm.com] 
>Sent: Monday, May 16, 2011 2:12 PM
>To: Saeed Bishara
>Cc: Russell King - ARM Linux; saeed bishara; 
>linux-arm-kernel at lists.infradead.org
>Subject: RE: [PATCH 1/2] ARM: remove unneeded check of the 
>cache_is_vipt_nonaliasing()
>
>On Mon, 2011-05-16 at 12:10 +0100, Saeed Bishara wrote:
>> commit 1ae2a4434959b5c6b234fa5f6eb5ad9a8e0fba0c
>> Author: Saeed Bishara <saeed@marvell.com>
>> Date:   Thu May 12 19:45:25 2011 +0300
>> 
>>     ARM: remove unneeded check of the cache_is_vipt_nonaliasing()
>> 
>>     when cache_is_vipt_nonaliasing(), we always have 
>pte_exec() true at
>>     the end of this function, so no need for the additional check.
>> 
>>     Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>
>Please add your Signed-off-by as well.
Sure.
Russle, I submitted the previous patch to patch system, how should change its status to discarded then upload new one?

saeed

[PATCH 1/2] ARM: remove unneeded check of the cache_is_vipt_nonaliasing()

[Russell King - ARM Linux]

On Mon, May 16, 2011 at 02:15:35PM +0300, Saeed Bishara wrote:
> >Please add your Signed-off-by as well.
> Sure.
> Russle, I submitted the previous patch to patch system, how should
> change its status to discarded then upload new one?

Submit the new one in the standard way.  I'll sort out the old one once
I see the new appear.

[PATCH 1/2] ARM: remove unneeded check of the cache_is_vipt_nonaliasing()

[saeed bishara]

On Mon, May 16, 2011 at 4:42 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, May 16, 2011 at 02:15:35PM +0300, Saeed Bishara wrote:
>> >Please add your Signed-off-by as well.
>> Sure.
>> Russle, I submitted the previous patch to patch system, how should
>> change its status to discarded then upload new one?
>
> Submit the new one in the standard way. ?I'll sort out the old one once
> I see the new appear.
done.
btw, the first attempt fail with the following error:
Your request to update the patch failed because:

    An internal state error was detected.

the second attempt succeeded.
saeed