[PATCH] mm: nobootmem: avoid type warning about alignment value

[PATCH] mm: nobootmem: avoid type warning about alignment value

[Santosh Shilimkar]

Building ARM with NO_BOOTMEM generates below warning. Using min_t
to find the correct alignment avoids the warning.

Cc: Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 mm/nobootmem.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/nobootmem.c b/mm/nobootmem.c
index 2c254d3..8954e43 100644
--- a/mm/nobootmem.c
+++ b/mm/nobootmem.c
@@ -85,7 +85,7 @@ static void __init __free_pages_memory(unsigned long start, unsigned long end)
 	int order;
 
 	while (start < end) {
-		order = min(MAX_ORDER - 1UL, __ffs(start));
+		order = min_t(size_t, MAX_ORDER - 1UL, __ffs(start));
 
 		while (start + (1UL << order) > end)
 			order--;
-- 
1.7.9.5

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: nobootmem: avoid type warning about alignment value

[Sergei Shtylyov]

Hello.

On 24-11-2013 3:28, Santosh Shilimkar wrote:

> Building ARM with NO_BOOTMEM generates below warning. Using min_t

    Where is that below? :-)

> to find the correct alignment avoids the warning.

> Cc: Tejun Heo <tj@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

WBR, Sergei


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: nobootmem: avoid type warning about alignment value

[Santosh Shilimkar]

On Sunday 24 November 2013 10:14 AM, Sergei Shtylyov wrote:
> Hello.
> 
> On 24-11-2013 3:28, Santosh Shilimkar wrote:
> 
>> Building ARM with NO_BOOTMEM generates below warning. Using min_t
> 
>    Where is that below? :-)
> 
Damn.. Posted a wrong version of the patch ;-(
Here is the one with warning message included.

Re: [PATCH] mm: nobootmem: avoid type warning about alignment value

[Tejun Heo]

On Mon, Nov 25, 2013 at 08:57:54AM -0500, Santosh Shilimkar wrote:
> On Sunday 24 November 2013 10:14 AM, Sergei Shtylyov wrote:
> > Hello.
> > 
> > On 24-11-2013 3:28, Santosh Shilimkar wrote:
> > 
> >> Building ARM with NO_BOOTMEM generates below warning. Using min_t
> > 
> >    Where is that below? :-)
> > 
> Damn.. Posted a wrong version of the patch ;-(
> Here is the one with warning message included.
> 
> From 571dfdf4cf8ac7dfd50bd9b7519717c42824f1c3 Mon Sep 17 00:00:00 2001
> From: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Date: Sat, 23 Nov 2013 18:16:50 -0500
> Subject: [PATCH] mm: nobootmem: avoid type warning about alignment value
> 
> Building ARM with NO_BOOTMEM generates below warning.
> 
> mm/nobootmem.c: In function a??__free_pages_memorya??:
> mm/nobootmem.c:88:11: warning: comparison of distinct pointer types lacks a cast
> 
> Using min_t to find the correct alignment avoids the warning.
> 
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: nobootmem: avoid type warning about alignment value

[Santosh Shilimkar]

Andrew,

On Monday 25 November 2013 10:56 AM, Tejun Heo wrote:
> On Mon, Nov 25, 2013 at 08:57:54AM -0500, Santosh Shilimkar wrote:
>> On Sunday 24 November 2013 10:14 AM, Sergei Shtylyov wrote:
>>> Hello.
>>>
>>> On 24-11-2013 3:28, Santosh Shilimkar wrote:
>>>
>>>> Building ARM with NO_BOOTMEM generates below warning. Using min_t
>>>
>>>    Where is that below? :-)
>>>
>> Damn.. Posted a wrong version of the patch ;-(
>> Here is the one with warning message included.
>>
>> From 571dfdf4cf8ac7dfd50bd9b7519717c42824f1c3 Mon Sep 17 00:00:00 2001
>> From: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Date: Sat, 23 Nov 2013 18:16:50 -0500
>> Subject: [PATCH] mm: nobootmem: avoid type warning about alignment value
>>
>> Building ARM with NO_BOOTMEM generates below warning.
>>
>> mm/nobootmem.c: In function a??__free_pages_memorya??:
>> mm/nobootmem.c:88:11: warning: comparison of distinct pointer types lacks a cast
>>
>> Using min_t to find the correct alignment avoids the warning.
>>
>> Cc: Tejun Heo <tj@kernel.org>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> 
Can you please this warning fix as well in your mm tree ?

Regards,
Santosh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: nobootmem: avoid type warning about alignment value

[Andrew Morton]

On Mon, 25 Nov 2013 08:57:54 -0500 Santosh Shilimkar <santosh.shilimkar@ti.com> wrote:

> On Sunday 24 November 2013 10:14 AM, Sergei Shtylyov wrote:
> > Hello.
> > 
> > On 24-11-2013 3:28, Santosh Shilimkar wrote:
> > 
> >> Building ARM with NO_BOOTMEM generates below warning. Using min_t
> > 
> >    Where is that below? :-)
> > 
> Damn.. Posted a wrong version of the patch ;-(
> Here is the one with warning message included.
> 
> >From 571dfdf4cf8ac7dfd50bd9b7519717c42824f1c3 Mon Sep 17 00:00:00 2001
> From: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Date: Sat, 23 Nov 2013 18:16:50 -0500
> Subject: [PATCH] mm: nobootmem: avoid type warning about alignment value
> 
> Building ARM with NO_BOOTMEM generates below warning.
> 
> mm/nobootmem.c: In function _____free_pages_memory___:
> mm/nobootmem.c:88:11: warning: comparison of distinct pointer types lacks a cast
> 
> Using min_t to find the correct alignment avoids the warning.
> 
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> ---
>  mm/nobootmem.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/nobootmem.c b/mm/nobootmem.c
> index 2c254d3..8954e43 100644
> --- a/mm/nobootmem.c
> +++ b/mm/nobootmem.c
> @@ -85,7 +85,7 @@ static void __init __free_pages_memory(unsigned long start, unsigned long end)
>  	int order;
>  
>  	while (start < end) {
> -		order = min(MAX_ORDER - 1UL, __ffs(start));
> +		order = min_t(size_t, MAX_ORDER - 1UL, __ffs(start));
>  

size_t makes no sense.  Neither `order', `MAX_ORDER', 1UL nor __ffs()
have that type.

min() warnings often indicate that the chosen types are inappropriate,
and suppressing them with min_t() should be a last resort.

MAX_ORDER-1UL has type `unsigned long' (yes?) and __ffs() should return
unsigned long (except arch/arc which decided to be different).

Why does it warn?  What's the underlying reason?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: nobootmem: avoid type warning about alignment value

[Russell King - ARM Linux]

On Mon, Dec 09, 2013 at 04:50:44PM -0800, Andrew Morton wrote:
> On Mon, 25 Nov 2013 08:57:54 -0500 Santosh Shilimkar <santosh.shilimkar@ti.com> wrote:
> 
> > On Sunday 24 November 2013 10:14 AM, Sergei Shtylyov wrote:
> > > Hello.
> > > 
> > > On 24-11-2013 3:28, Santosh Shilimkar wrote:
> > > 
> > >> Building ARM with NO_BOOTMEM generates below warning. Using min_t
> > > 
> > >    Where is that below? :-)
> > > 
> > Damn.. Posted a wrong version of the patch ;-(
> > Here is the one with warning message included.
> > 
> > >From 571dfdf4cf8ac7dfd50bd9b7519717c42824f1c3 Mon Sep 17 00:00:00 2001
> > From: Santosh Shilimkar <santosh.shilimkar@ti.com>
> > Date: Sat, 23 Nov 2013 18:16:50 -0500
> > Subject: [PATCH] mm: nobootmem: avoid type warning about alignment value
> > 
> > Building ARM with NO_BOOTMEM generates below warning.
> > 
> > mm/nobootmem.c: In function _____free_pages_memory___:
> > mm/nobootmem.c:88:11: warning: comparison of distinct pointer types lacks a cast
> > 
> > Using min_t to find the correct alignment avoids the warning.
> > 
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> > ---
> >  mm/nobootmem.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/mm/nobootmem.c b/mm/nobootmem.c
> > index 2c254d3..8954e43 100644
> > --- a/mm/nobootmem.c
> > +++ b/mm/nobootmem.c
> > @@ -85,7 +85,7 @@ static void __init __free_pages_memory(unsigned long start, unsigned long end)
> >  	int order;
> >  
> >  	while (start < end) {
> > -		order = min(MAX_ORDER - 1UL, __ffs(start));
> > +		order = min_t(size_t, MAX_ORDER - 1UL, __ffs(start));
> >  
> 
> size_t makes no sense.  Neither `order', `MAX_ORDER', 1UL nor __ffs()
> have that type.
> 
> min() warnings often indicate that the chosen types are inappropriate,
> and suppressing them with min_t() should be a last resort.
> 
> MAX_ORDER-1UL has type `unsigned long' (yes?) and __ffs() should return
> unsigned long (except arch/arc which decided to be different).
> 
> Why does it warn?  What's the underlying reason?

The underlying reason is that - as I've already explained - ARM's __ffs()
differs from other architectures in that it ends up being an int, whereas
almost everyone else is unsigned long.

The fix is to fix ARMs __ffs() to conform to other architectures.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: nobootmem: avoid type warning about alignment value

[Santosh Shilimkar]

On Monday 09 December 2013 07:54 PM, Russell King - ARM Linux wrote:
> On Mon, Dec 09, 2013 at 04:50:44PM -0800, Andrew Morton wrote:
>> On Mon, 25 Nov 2013 08:57:54 -0500 Santosh Shilimkar <santosh.shilimkar@ti.com> wrote:
>>
>>> On Sunday 24 November 2013 10:14 AM, Sergei Shtylyov wrote:
>>>> Hello.
>>>>
>>>> On 24-11-2013 3:28, Santosh Shilimkar wrote:
>>>>
>>>>> Building ARM with NO_BOOTMEM generates below warning. Using min_t
>>>>
>>>>    Where is that below? :-)
>>>>
>>> Damn.. Posted a wrong version of the patch ;-(
>>> Here is the one with warning message included.
>>>
>>> >From 571dfdf4cf8ac7dfd50bd9b7519717c42824f1c3 Mon Sep 17 00:00:00 2001
>>> From: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>> Date: Sat, 23 Nov 2013 18:16:50 -0500
>>> Subject: [PATCH] mm: nobootmem: avoid type warning about alignment value
>>>
>>> Building ARM with NO_BOOTMEM generates below warning.
>>>
>>> mm/nobootmem.c: In function _____free_pages_memory___:
>>> mm/nobootmem.c:88:11: warning: comparison of distinct pointer types lacks a cast
>>>
>>> Using min_t to find the correct alignment avoids the warning.
>>>
>>> Cc: Tejun Heo <tj@kernel.org>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>> ---
>>>  mm/nobootmem.c |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/mm/nobootmem.c b/mm/nobootmem.c
>>> index 2c254d3..8954e43 100644
>>> --- a/mm/nobootmem.c
>>> +++ b/mm/nobootmem.c
>>> @@ -85,7 +85,7 @@ static void __init __free_pages_memory(unsigned long start, unsigned long end)
>>>  	int order;
>>>  
>>>  	while (start < end) {
>>> -		order = min(MAX_ORDER - 1UL, __ffs(start));
>>> +		order = min_t(size_t, MAX_ORDER - 1UL, __ffs(start));
>>>  
>>
>> size_t makes no sense.  Neither `order', `MAX_ORDER', 1UL nor __ffs()
>> have that type.
>>
>> min() warnings often indicate that the chosen types are inappropriate,
>> and suppressing them with min_t() should be a last resort.
>>
>> MAX_ORDER-1UL has type `unsigned long' (yes?) and __ffs() should return
>> unsigned long (except arch/arc which decided to be different).
>>
>> Why does it warn?  What's the underlying reason?
> 
> The underlying reason is that - as I've already explained - ARM's __ffs()
> differs from other architectures in that it ends up being an int, whereas
> almost everyone else is unsigned long.
> 
> The fix is to fix ARMs __ffs() to conform to other architectures.
> 
I was just about to cross-post your reply here. Obviously I didn't think
this far when I made  $subject fix.

So lets ignore the $subject patch which is not correct. Sorry for noise

Regards,
Santosh




--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: nobootmem: avoid type warning about alignment value

[Russell King - ARM Linux]

On Mon, Dec 09, 2013 at 08:02:30PM -0500, Santosh Shilimkar wrote:
> On Monday 09 December 2013 07:54 PM, Russell King - ARM Linux wrote:
> > The underlying reason is that - as I've already explained - ARM's __ffs()
> > differs from other architectures in that it ends up being an int, whereas
> > almost everyone else is unsigned long.
> > 
> > The fix is to fix ARMs __ffs() to conform to other architectures.
> > 
> I was just about to cross-post your reply here. Obviously I didn't think
> this far when I made  $subject fix.
> 
> So lets ignore the $subject patch which is not correct. Sorry for noise

Well, here we are, a month on, and this still remains unfixed despite
my comments pointing to what the problem is.  So, here's a patch to fix
this problem the correct way.  I took the time to add some comments to
these functions as I find that I wonder about their return values, and
these comments make the patch a little larger than it otherwise would be.

This patch makes their types match exactly with x86's definitions of
the same, which is the basic problem: on ARM, they all took "int" values
and returned "int"s, which leads to min() in nobootmem.c complaining.

 arch/arm/include/asm/bitops.h | 54 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 44 insertions(+), 10 deletions(-)

diff --git a/arch/arm/include/asm/bitops.h b/arch/arm/include/asm/bitops.h
index e691ec91e4d3..b2e298a90d76 100644
--- a/arch/arm/include/asm/bitops.h
+++ b/arch/arm/include/asm/bitops.h
@@ -254,25 +254,59 @@ static inline int constant_fls(int x)
 }
 
 /*
- * On ARMv5 and above those functions can be implemented around
- * the clz instruction for much better code efficiency.
+ * On ARMv5 and above those functions can be implemented around the
+ * clz instruction for much better code efficiency.  __clz returns
+ * the number of leading zeros, zero input will return 32, and
+ * 0x80000000 will return 0.
  */
+static inline unsigned int __clz(unsigned int x)
+{
+	unsigned int ret;
+
+	asm("clz\t%0, %1" : "=r" (ret) : "r" (x));
 
+	return ret;
+}
+
+/*
+ * fls() returns zero if the input is zero, otherwise returns the bit
+ * position of the last set bit, where the LSB is 1 and MSB is 32.
+ */
 static inline int fls(int x)
 {
-	int ret;
-
 	if (__builtin_constant_p(x))
 	       return constant_fls(x);
 
-	asm("clz\t%0, %1" : "=r" (ret) : "r" (x));
-       	ret = 32 - ret;
-	return ret;
+	return 32 - __clz(x);
+}
+
+/*
+ * __fls() returns the bit position of the last bit set, where the
+ * LSB is 0 and MSB is 31.  Zero input is undefined.
+ */
+static inline unsigned long __fls(unsigned long x)
+{
+	return fls(x) - 1;
+}
+
+/*
+ * ffs() returns zero if the input was zero, otherwise returns the bit
+ * position of the first set bit, where the LSB is 1 and MSB is 32.
+ */
+static inline int ffs(int x)
+{
+	return fls(x & -x);
+}
+
+/*
+ * __ffs() returns the bit position of the first bit set, where the
+ * LSB is 0 and MSB is 31.  Zero input is undefined.
+ */
+static inline unsigned long __ffs(unsigned long x)
+{
+	return ffs(x) - 1;
 }
 
-#define __fls(x) (fls(x) - 1)
-#define ffs(x) ({ unsigned long __t = (x); fls(__t & -__t); })
-#define __ffs(x) (ffs(x) - 1)
 #define ffz(x) __ffs( ~(x) )
 
 #endif


-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: nobootmem: avoid type warning about alignment value

[Santosh Shilimkar]

On Sunday 12 January 2014 05:59 AM, Russell King - ARM Linux wrote:
> On Mon, Dec 09, 2013 at 08:02:30PM -0500, Santosh Shilimkar wrote:
>> On Monday 09 December 2013 07:54 PM, Russell King - ARM Linux wrote:
>>> The underlying reason is that - as I've already explained - ARM's __ffs()
>>> differs from other architectures in that it ends up being an int, whereas
>>> almost everyone else is unsigned long.
>>>
>>> The fix is to fix ARMs __ffs() to conform to other architectures.
>>>
>> I was just about to cross-post your reply here. Obviously I didn't think
>> this far when I made  $subject fix.
>>
>> So lets ignore the $subject patch which is not correct. Sorry for noise
> 
> Well, here we are, a month on, and this still remains unfixed despite
> my comments pointing to what the problem is.  So, here's a patch to fix
> this problem the correct way.  I took the time to add some comments to
> these functions as I find that I wonder about their return values, and
> these comments make the patch a little larger than it otherwise would be.
> 
The $subject warning fix [1] is already picked by Andrew with your ack
and its in his queue [2]

> This patch makes their types match exactly with x86's definitions of
> the same, which is the basic problem: on ARM, they all took "int" values
> and returned "int"s, which leads to min() in nobootmem.c complaining.
> 
Not sure if you missed the thread but the right fix was picked. Ofcourse
you do have additional clz optimisation in updated patch and some comments
on those functions.

Regards,
Santosh

[1] https://lkml.org/lkml/2013/12/9/811
[2] http://ozlabs.org/~akpm/mmotm/broken-out/mm-arm-fix-arms-__ffs-to-conform-to-avoid-warning-with-no_bootmem.patch

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: nobootmem: avoid type warning about alignment value

[Santosh Shilimkar]

On Sunday 12 January 2014 10:42 AM, Santosh Shilimkar wrote:
> On Sunday 12 January 2014 05:59 AM, Russell King - ARM Linux wrote:
>> On Mon, Dec 09, 2013 at 08:02:30PM -0500, Santosh Shilimkar wrote:
>>> On Monday 09 December 2013 07:54 PM, Russell King - ARM Linux wrote:
>>>> The underlying reason is that - as I've already explained - ARM's __ffs()
>>>> differs from other architectures in that it ends up being an int, whereas
>>>> almost everyone else is unsigned long.
>>>>
>>>> The fix is to fix ARMs __ffs() to conform to other architectures.
>>>>
>>> I was just about to cross-post your reply here. Obviously I didn't think
>>> this far when I made  $subject fix.
>>>
>>> So lets ignore the $subject patch which is not correct. Sorry for noise
>>
>> Well, here we are, a month on, and this still remains unfixed despite
>> my comments pointing to what the problem is.  So, here's a patch to fix
>> this problem the correct way.  I took the time to add some comments to
>> these functions as I find that I wonder about their return values, and
>> these comments make the patch a little larger than it otherwise would be.
>>
> The $subject warning fix [1] is already picked by Andrew with your ack
> and its in his queue [2]
> 
>> This patch makes their types match exactly with x86's definitions of
>> the same, which is the basic problem: on ARM, they all took "int" values
>> and returned "int"s, which leads to min() in nobootmem.c complaining.
>>
> Not sure if you missed the thread but the right fix was picked. Ofcourse
> you do have additional clz optimisation in updated patch and some comments
> on those functions.
> 
> Regards,
> Santosh
> 
> [1] https://lkml.org/lkml/2013/12/9/811
fixing the link since above was the link to the $subject thread and below is
the correct link for updated patch
[1] https://lkml.org/lkml/2013/12/20/497
> [2] http://ozlabs.org/~akpm/mmotm/broken-out/mm-arm-fix-arms-__ffs-to-conform-to-avoid-warning-with-no_bootmem.patch
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: nobootmem: avoid type warning about alignment value

[Russell King - ARM Linux]

On Sun, Jan 12, 2014 at 10:42:00AM -0500, Santosh Shilimkar wrote:
> On Sunday 12 January 2014 05:59 AM, Russell King - ARM Linux wrote:
> > On Mon, Dec 09, 2013 at 08:02:30PM -0500, Santosh Shilimkar wrote:
> >> On Monday 09 December 2013 07:54 PM, Russell King - ARM Linux wrote:
> >>> The underlying reason is that - as I've already explained - ARM's __ffs()
> >>> differs from other architectures in that it ends up being an int, whereas
> >>> almost everyone else is unsigned long.
> >>>
> >>> The fix is to fix ARMs __ffs() to conform to other architectures.
> >>>
> >> I was just about to cross-post your reply here. Obviously I didn't think
> >> this far when I made  $subject fix.
> >>
> >> So lets ignore the $subject patch which is not correct. Sorry for noise
> > 
> > Well, here we are, a month on, and this still remains unfixed despite
> > my comments pointing to what the problem is.  So, here's a patch to fix
> > this problem the correct way.  I took the time to add some comments to
> > these functions as I find that I wonder about their return values, and
> > these comments make the patch a little larger than it otherwise would be.
> > 
> The $subject warning fix [1] is already picked by Andrew with your ack
> and its in his queue [2]
> 
> > This patch makes their types match exactly with x86's definitions of
> > the same, which is the basic problem: on ARM, they all took "int" values
> > and returned "int"s, which leads to min() in nobootmem.c complaining.
> > 
> Not sure if you missed the thread but the right fix was picked. Ofcourse
> you do have additional clz optimisation in updated patch and some comments
> on those functions.

The problem here is that the patch fixing this is going via akpm's tree
(why?) yet you want the code which introduces the warning to be merged
via my tree.

It seems to me to be absolutely silly to have code introduce a warning
yet push the fix for the warning via a completely different tree...

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: nobootmem: avoid type warning about alignment value

[Santosh Shilimkar]

On Monday 13 January 2014 07:37 AM, Russell King - ARM Linux wrote:
> On Sun, Jan 12, 2014 at 10:42:00AM -0500, Santosh Shilimkar wrote:
>> On Sunday 12 January 2014 05:59 AM, Russell King - ARM Linux wrote:
>>> On Mon, Dec 09, 2013 at 08:02:30PM -0500, Santosh Shilimkar wrote:
>>>> On Monday 09 December 2013 07:54 PM, Russell King - ARM Linux wrote:
>>>>> The underlying reason is that - as I've already explained - ARM's __ffs()
>>>>> differs from other architectures in that it ends up being an int, whereas
>>>>> almost everyone else is unsigned long.
>>>>>
>>>>> The fix is to fix ARMs __ffs() to conform to other architectures.
>>>>>
>>>> I was just about to cross-post your reply here. Obviously I didn't think
>>>> this far when I made  $subject fix.
>>>>
>>>> So lets ignore the $subject patch which is not correct. Sorry for noise
>>>
>>> Well, here we are, a month on, and this still remains unfixed despite
>>> my comments pointing to what the problem is.  So, here's a patch to fix
>>> this problem the correct way.  I took the time to add some comments to
>>> these functions as I find that I wonder about their return values, and
>>> these comments make the patch a little larger than it otherwise would be.
>>>
>> The $subject warning fix [1] is already picked by Andrew with your ack
>> and its in his queue [2]
>>
>>> This patch makes their types match exactly with x86's definitions of
>>> the same, which is the basic problem: on ARM, they all took "int" values
>>> and returned "int"s, which leads to min() in nobootmem.c complaining.
>>>
>> Not sure if you missed the thread but the right fix was picked. Ofcourse
>> you do have additional clz optimisation in updated patch and some comments
>> on those functions.
> 
> The problem here is that the patch fixing this is going via akpm's tree
> (why?) yet you want the code which introduces the warning to be merged
> via my tree.
>
> It seems to me to be absolutely silly to have code introduce a warning
> yet push the fix for the warning via a completely different tree...
> 
I mixed it up. Sorry. Some how I thought there was some other build
configuration thrown the same warning with memblock series and hence
suggested the patch to go via Andrew's tree.

Regards,
Santosh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: nobootmem: avoid type warning about alignment value

[Andrew Morton]

On Mon, 13 Jan 2014 09:27:44 -0500 Santosh Shilimkar <santosh.shilimkar@ti.com> wrote:

> > It seems to me to be absolutely silly to have code introduce a warning
> > yet push the fix for the warning via a completely different tree...
> > 
> I mixed it up. Sorry. Some how I thought there was some other build
> configuration thrown the same warning with memblock series and hence
> suggested the patch to go via Andrew's tree.

Yes, I too had assumed that the warning was caused by the bootmem
patches in -mm.

But it in fact occurs in Linus's current tree.  I'll drop
mm-arm-fix-arms-__ffs-to-conform-to-avoid-warning-with-no_bootmem.patch
and I'll assume that rmk will fix this up at an appropriate time.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: nobootmem: avoid type warning about alignment value

[Russell King - ARM Linux]

On Mon, Jan 13, 2014 at 03:31:28PM -0800, Andrew Morton wrote:
> On Mon, 13 Jan 2014 09:27:44 -0500 Santosh Shilimkar <santosh.shilimkar@ti.com> wrote:
> 
> > > It seems to me to be absolutely silly to have code introduce a warning
> > > yet push the fix for the warning via a completely different tree...
> > > 
> > I mixed it up. Sorry. Some how I thought there was some other build
> > configuration thrown the same warning with memblock series and hence
> > suggested the patch to go via Andrew's tree.
> 
> Yes, I too had assumed that the warning was caused by the bootmem
> patches in -mm.
> 
> But it in fact occurs in Linus's current tree.  I'll drop
> mm-arm-fix-arms-__ffs-to-conform-to-avoid-warning-with-no_bootmem.patch
> and I'll assume that rmk will fix this up at an appropriate time.

Thanks.  I'll apply my version and then I can pull Santosh's nobootmem
changes (which I've had a couple of times already) without adding to
the warnings.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: nobootmem: avoid type warning about alignment value

[Santosh Shilimkar]

On Monday 13 January 2014 06:33 PM, Russell King - ARM Linux wrote:
> On Mon, Jan 13, 2014 at 03:31:28PM -0800, Andrew Morton wrote:
>> On Mon, 13 Jan 2014 09:27:44 -0500 Santosh Shilimkar <santosh.shilimkar@ti.com> wrote:
>>
>>>> It seems to me to be absolutely silly to have code introduce a warning
>>>> yet push the fix for the warning via a completely different tree...
>>>>
>>> I mixed it up. Sorry. Some how I thought there was some other build
>>> configuration thrown the same warning with memblock series and hence
>>> suggested the patch to go via Andrew's tree.
>>
>> Yes, I too had assumed that the warning was caused by the bootmem
>> patches in -mm.
>>
>> But it in fact occurs in Linus's current tree.  I'll drop
>> mm-arm-fix-arms-__ffs-to-conform-to-avoid-warning-with-no_bootmem.patch
>> and I'll assume that rmk will fix this up at an appropriate time.
> 
> Thanks.  I'll apply my version and then I can pull Santosh's nobootmem
> changes (which I've had a couple of times already) without adding to
> the warnings.
> 
Thanks Andrew and Russell for sorting out the patch queue.

Regards,
Santosh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: nobootmem: avoid type warning about alignment value

[Nicolas Pitre]

On Sun, 12 Jan 2014, Russell King - ARM Linux wrote:

> This patch makes their types match exactly with x86's definitions of
> the same, which is the basic problem: on ARM, they all took "int" values
> and returned "int"s, which leads to min() in nobootmem.c complaining.
> 
>  arch/arm/include/asm/bitops.h | 54 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 44 insertions(+), 10 deletions(-)

For the record:

Acked-by: Nicolas Pitre <nico@linaro.org>

The reason why macros were used at the time this was originally written 
is because gcc used to have issues forwarding the constant nature of a 
variable down multiple levels of inline functions and 
__builtin_constant_p() always returned false.  But that was quite a long 
time ago.


> diff --git a/arch/arm/include/asm/bitops.h b/arch/arm/include/asm/bitops.h
> index e691ec91e4d3..b2e298a90d76 100644
> --- a/arch/arm/include/asm/bitops.h
> +++ b/arch/arm/include/asm/bitops.h
> @@ -254,25 +254,59 @@ static inline int constant_fls(int x)
>  }
>  
>  /*
> - * On ARMv5 and above those functions can be implemented around
> - * the clz instruction for much better code efficiency.
> + * On ARMv5 and above those functions can be implemented around the
> + * clz instruction for much better code efficiency.  __clz returns
> + * the number of leading zeros, zero input will return 32, and
> + * 0x80000000 will return 0.
>   */
> +static inline unsigned int __clz(unsigned int x)
> +{
> +	unsigned int ret;
> +
> +	asm("clz\t%0, %1" : "=r" (ret) : "r" (x));
>  
> +	return ret;
> +}
> +
> +/*
> + * fls() returns zero if the input is zero, otherwise returns the bit
> + * position of the last set bit, where the LSB is 1 and MSB is 32.
> + */
>  static inline int fls(int x)
>  {
> -	int ret;
> -
>  	if (__builtin_constant_p(x))
>  	       return constant_fls(x);
>  
> -	asm("clz\t%0, %1" : "=r" (ret) : "r" (x));
> -       	ret = 32 - ret;
> -	return ret;
> +	return 32 - __clz(x);
> +}
> +
> +/*
> + * __fls() returns the bit position of the last bit set, where the
> + * LSB is 0 and MSB is 31.  Zero input is undefined.
> + */
> +static inline unsigned long __fls(unsigned long x)
> +{
> +	return fls(x) - 1;
> +}
> +
> +/*
> + * ffs() returns zero if the input was zero, otherwise returns the bit
> + * position of the first set bit, where the LSB is 1 and MSB is 32.
> + */
> +static inline int ffs(int x)
> +{
> +	return fls(x & -x);
> +}
> +
> +/*
> + * __ffs() returns the bit position of the first bit set, where the
> + * LSB is 0 and MSB is 31.  Zero input is undefined.
> + */
> +static inline unsigned long __ffs(unsigned long x)
> +{
> +	return ffs(x) - 1;
>  }
>  
> -#define __fls(x) (fls(x) - 1)
> -#define ffs(x) ({ unsigned long __t = (x); fls(__t & -__t); })
> -#define __ffs(x) (ffs(x) - 1)
>  #define ffz(x) __ffs( ~(x) )
>  
>  #endif
> 
> 
> -- 
> FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
> in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
> Estimate before purchase was "up to 13.2Mbit".
> --
> 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/
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>