[PATCH] ARM: l2x0: Check the correct address range

[PATCH] ARM: l2x0: Check the correct address range

[Kyungmin Park]

From: Boojin Kim <boojin.kim@samsung.com>

When flush or clean the 1MiB, it doesn't flush or clean all since it doesn't check the correct address. So Check the correct address range.

Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 170c9bb..50599d9 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -195,7 +195,7 @@ static void l2x0_clean_range(unsigned long start, unsigned long end)
 	void __iomem *base = l2x0_base;
 	unsigned long flags;
 
-	if ((end - start) >= l2x0_size) {
+	if ((end - start + 1) >= l2x0_size) {
 		l2x0_clean_all();
 		return;
 	}
@@ -225,7 +225,7 @@ static void l2x0_flush_range(unsigned long start, unsigned long end)
 	void __iomem *base = l2x0_base;
 	unsigned long flags;
 
-	if ((end - start) >= l2x0_size) {
+	if ((end - start + 1) >= l2x0_size) {
 		l2x0_flush_all();
 		return;
 	}

[PATCH] ARM: l2x0: Check the correct address range

[Kyungmin Park]

From: Boojin Kim <boojin.kim@samsung.com>

When flush or clean the 1MiB, it doesn't flush or clean all since it doesn't check the correct address. So Check the correct address range.

Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 170c9bb..50599d9 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -195,7 +195,7 @@ static void l2x0_clean_range(unsigned long start, unsigned long end)
 	void __iomem *base = l2x0_base;
 	unsigned long flags;
 
-	if ((end - start) >= l2x0_size) {
+	if ((end - start + 1) >= l2x0_size) {
 		l2x0_clean_all();
 		return;
 	}
@@ -225,7 +225,7 @@ static void l2x0_flush_range(unsigned long start, unsigned long end)
 	void __iomem *base = l2x0_base;
 	unsigned long flags;
 
-	if ((end - start) >= l2x0_size) {
+	if ((end - start + 1) >= l2x0_size) {
 		l2x0_flush_all();
 		return;
 	}

[PATCH] ARM: l2x0: Check the correct address range

[Catalin Marinas]

On 19 November 2010 01:40, Kyungmin Park <kmpark@infradead.org> wrote:
> From: Boojin Kim <boojin.kim@samsung.com>
>
> When flush or clean the 1MiB, it doesn't flush or clean all since it doesn't check the correct address. So Check the correct address range.

This line is very long.

The patch looks fine otherwise. I think the optimal value would be
smaller than the whole cache size but it depends on many things.

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

Re: [PATCH] ARM: l2x0: Check the correct address range

[Catalin Marinas]

On 19 November 2010 01:40, Kyungmin Park <kmpark@infradead.org> wrote:
> From: Boojin Kim <boojin.kim@samsung.com>
>
> When flush or clean the 1MiB, it doesn't flush or clean all since it doesn't check the correct address. So Check the correct address range.

This line is very long.

The patch looks fine otherwise. I think the optimal value would be
smaller than the whole cache size but it depends on many things.

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

[PATCH] ARM: l2x0: Check the correct address range

[Linus WALLEIJ]

On Fri, 2010-11-19 at 11:08 +0000, Catalin Marinas wrote:
> On 19 November 2010 01:40, Kyungmin Park <kmpark@infradead.org> wrote:
> > From: Boojin Kim <boojin.kim@samsung.com>
> >
> > When flush or clean the 1MiB, it doesn't flush or clean all since it doesn't check the correct address. So Check the correct address range.
> 
> This line is very long.
> 
> The patch looks fine otherwise. I think the optimal value would be
> smaller than the whole cache size but it depends on many things.

Would it be a good idea to kick in a per-SoC threshold value into
the l2x0 range clean function, such that if the range exceed this
specific threshold it cleans all of it? That way it'd be possible
to optimize for each SoC quite easily.

I've heard that these things may even depend on OPPs etc so it may
even be some dynamic value...

Yours,
Linus Walleij
-------------- next part --------------
A non-text attachment was scrubbed...
Name: winmail.dat
Type: application/ms-tnef
Size: 3547 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101119/0e0c8bc6/attachment-0001.bin>

Re: Re: [PATCH] ARM: l2x0: Check the correct address range

[Linus WALLEIJ]

[-- Attachment #1: Type: text/plain, Size: 851 bytes --]

On Fri, 2010-11-19 at 11:08 +0000, Catalin Marinas wrote:
> On 19 November 2010 01:40, Kyungmin Park <kmpark@infradead.org> wrote:
> > From: Boojin Kim <boojin.kim@samsung.com>
> >
> > When flush or clean the 1MiB, it doesn't flush or clean all since it doesn't check the correct address. So Check the correct address range.
> 
> This line is very long.
> 
> The patch looks fine otherwise. I think the optimal value would be
> smaller than the whole cache size but it depends on many things.

Would it be a good idea to kick in a per-SoC threshold value into
the l2x0 range clean function, such that if the range exceed this
specific threshold it cleans all of it? That way it'd be possible
to optimize for each SoC quite easily.

I've heard that these things may even depend on OPPs etc so it may
even be some dynamic value...

Yours,
Linus Walleij

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 3547 bytes --]

[PATCH] ARM: l2x0: Check the correct address range

[Kanigeri, Hari]

> From: Boojin Kim <boojin.kim@samsung.com>
>
> When flush or clean the 1MiB, it doesn't flush or clean all since it doesn't check the correct address. So Check the correct address range.

I saw this before but then I thought that the individual callers of
cache functions have to call with  +1 to the end address.

>
> Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> index 170c9bb..50599d9 100644
> --- a/arch/arm/mm/cache-l2x0.c
> +++ b/arch/arm/mm/cache-l2x0.c
> @@ -195,7 +195,7 @@ static void l2x0_clean_range(unsigned long start, unsigned long end)
> ? ? ? ?void __iomem *base = l2x0_base;
> ? ? ? ?unsigned long flags;
>
> - ? ? ? if ((end - start) >= l2x0_size) {
> + ? ? ? if ((end - start + 1) >= l2x0_size) {

nitpick: probably will look more obvious if it is coded as  if ((end +
1) - start) >= l2x0_size)

> ? ? ? ? ? ? ? ?l2x0_clean_all();
> ? ? ? ? ? ? ? ?return;
> ? ? ? ?}
> @@ -225,7 +225,7 @@ static void l2x0_flush_range(unsigned long start, unsigned long end)
> ? ? ? ?void __iomem *base = l2x0_base;
> ? ? ? ?unsigned long flags;
>
> - ? ? ? if ((end - start) >= l2x0_size) {
> + ? ? ? if ((end - start + 1) >= l2x0_size) {

same as above.


Thank you,
Best regards,
Hari Kanigeri

Re: [PATCH] ARM: l2x0: Check the correct address range

[Kanigeri, Hari]

> From: Boojin Kim <boojin.kim@samsung.com>
>
> When flush or clean the 1MiB, it doesn't flush or clean all since it doesn't check the correct address. So Check the correct address range.

I saw this before but then I thought that the individual callers of
cache functions have to call with  +1 to the end address.

>
> Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> index 170c9bb..50599d9 100644
> --- a/arch/arm/mm/cache-l2x0.c
> +++ b/arch/arm/mm/cache-l2x0.c
> @@ -195,7 +195,7 @@ static void l2x0_clean_range(unsigned long start, unsigned long end)
>        void __iomem *base = l2x0_base;
>        unsigned long flags;
>
> -       if ((end - start) >= l2x0_size) {
> +       if ((end - start + 1) >= l2x0_size) {

nitpick: probably will look more obvious if it is coded as  if ((end +
1) - start) >= l2x0_size)

>                l2x0_clean_all();
>                return;
>        }
> @@ -225,7 +225,7 @@ static void l2x0_flush_range(unsigned long start, unsigned long end)
>        void __iomem *base = l2x0_base;
>        unsigned long flags;
>
> -       if ((end - start) >= l2x0_size) {
> +       if ((end - start + 1) >= l2x0_size) {

same as above.


Thank you,
Best regards,
Hari Kanigeri

[PATCH] ARM: l2x0: Check the correct address range

[Russell King - ARM Linux]

On Fri, Nov 19, 2010 at 05:30:14AM -0600, Kanigeri, Hari wrote:
> > diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> > index 170c9bb..50599d9 100644
> > --- a/arch/arm/mm/cache-l2x0.c
> > +++ b/arch/arm/mm/cache-l2x0.c
> > @@ -195,7 +195,7 @@ static void l2x0_clean_range(unsigned long start, unsigned long end)
> > ? ? ? ?void __iomem *base = l2x0_base;
> > ? ? ? ?unsigned long flags;
> >
> > - ? ? ? if ((end - start) >= l2x0_size) {
> > + ? ? ? if ((end - start + 1) >= l2x0_size) {
> 
> nitpick: probably will look more obvious if it is coded as  if ((end +
> 1) - start) >= l2x0_size)

Start defines the first byte of the object to perform the cache
maintainence on.  End defines the first subsequent byte not in the
object.  So, (end - start) is the number of bytes in the object,
and therefore the original code _is_ correct.

If you have an object which is 1MB in size, then (end - start) will
be 1048576 bytes.  If your cache is 1MB, then we'll use the xxx_all()
version of the function.

However, things get more murky when you consider that we have the
granularity of cache lines to deal with.

If start is mid-way in a cache line, then that cache line has to be
operated on.  If end is mid-way or at the end of a cache line, that
cache line also has to be operated on.  However, if end is at the
very first byte of the cache line, it must not be operated on.

So, it is possible for start = mid-cache line, end = start + 1048575,
that you will end up not doing the xxx_all() optimization, despite
operating on over 1MB of cached data - but this is only true for the
corner case up to 1MB + cache line size - 1.

Re: [PATCH] ARM: l2x0: Check the correct address range

[Russell King - ARM Linux]

On Fri, Nov 19, 2010 at 05:30:14AM -0600, Kanigeri, Hari wrote:
> > diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> > index 170c9bb..50599d9 100644
> > --- a/arch/arm/mm/cache-l2x0.c
> > +++ b/arch/arm/mm/cache-l2x0.c
> > @@ -195,7 +195,7 @@ static void l2x0_clean_range(unsigned long start, unsigned long end)
> >        void __iomem *base = l2x0_base;
> >        unsigned long flags;
> >
> > -       if ((end - start) >= l2x0_size) {
> > +       if ((end - start + 1) >= l2x0_size) {
> 
> nitpick: probably will look more obvious if it is coded as  if ((end +
> 1) - start) >= l2x0_size)

Start defines the first byte of the object to perform the cache
maintainence on.  End defines the first subsequent byte not in the
object.  So, (end - start) is the number of bytes in the object,
and therefore the original code _is_ correct.

If you have an object which is 1MB in size, then (end - start) will
be 1048576 bytes.  If your cache is 1MB, then we'll use the xxx_all()
version of the function.

However, things get more murky when you consider that we have the
granularity of cache lines to deal with.

If start is mid-way in a cache line, then that cache line has to be
operated on.  If end is mid-way or at the end of a cache line, that
cache line also has to be operated on.  However, if end is at the
very first byte of the cache line, it must not be operated on.

So, it is possible for start = mid-cache line, end = start + 1048575,
that you will end up not doing the xxx_all() optimization, despite
operating on over 1MB of cached data - but this is only true for the
corner case up to 1MB + cache line size - 1.