[PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Andrei Warkentin]

1 is a power of two, therefore rounddown_pow_of_two(1) should return 1. It does
in case the argument is a variable but in case it's a constant it behaves
wrong and returns 0. Probably nobody ever did it so this was never noticed,
however net/drivers/vmxnet3 with latest GCC does and breaks on unicpu systems.

This is similar to Rolf's patch to roundup_pow_of_two(1).

Cc: Rolf Eike Beer <eike-kernel@sf-tec.de>
Cc: opensuse-kernel@opensuse.org
Reviewed-by: Jesper Juhl <jj@chaosbits.net>
Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
---
 include/linux/log2.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/log2.h b/include/linux/log2.h
index 25b8086..ccda848 100644
--- a/include/linux/log2.h
+++ b/include/linux/log2.h
@@ -185,7 +185,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
 #define rounddown_pow_of_two(n)			\
 (						\
 	__builtin_constant_p(n) ? (		\
-		(n == 1) ? 0 :			\
+		(n == 1) ? 1 :			\
 		(1UL << ilog2(n))) :		\
 	__rounddown_pow_of_two(n)		\
  )
-- 
1.7.4.1

Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Rolf Eike Beer]

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

Andrei Warkentin wrote:
> 1 is a power of two, therefore rounddown_pow_of_two(1) should return 1. It
> does in case the argument is a variable but in case it's a constant it
> behaves wrong and returns 0. Probably nobody ever did it so this was never
> noticed, however net/drivers/vmxnet3 with latest GCC does and breaks on
> unicpu systems.

Obviously correct.

Reviewed-by: Rolf Eike Beer <eike-kernel@sf-tec.de>

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Andrew Morton]

On Wed, 16 Nov 2011 14:56:06 -0500
Andrei Warkentin <andreiw@vmware.com> wrote:

> 1 is a power of two, therefore rounddown_pow_of_two(1) should return 1. It does
> in case the argument is a variable but in case it's a constant it behaves
> wrong and returns 0. Probably nobody ever did it so this was never noticed,
> however net/drivers/vmxnet3 with latest GCC does and breaks on unicpu systems.
> 
> This is similar to Rolf's patch to roundup_pow_of_two(1).
> 
> Cc: Rolf Eike Beer <eike-kernel@sf-tec.de>
> Cc: opensuse-kernel@opensuse.org
> Reviewed-by: Jesper Juhl <jj@chaosbits.net>
> Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
> ---
>  include/linux/log2.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/log2.h b/include/linux/log2.h
> index 25b8086..ccda848 100644
> --- a/include/linux/log2.h
> +++ b/include/linux/log2.h
> @@ -185,7 +185,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
>  #define rounddown_pow_of_two(n)			\
>  (						\
>  	__builtin_constant_p(n) ? (		\
> -		(n == 1) ? 0 :			\
> +		(n == 1) ? 1 :			\
>  		(1UL << ilog2(n))) :		\
>  	__rounddown_pow_of_two(n)		\
>   )

I assume that nobody has gone off and checked whether all current
callers will survive this change.  If they had, they'd have looked in
drivers/char/ramoops.c and seen:

	rounddown_pow_of_two(pdata->mem_size);
	rounddown_pow_of_two(pdata->record_size);

These operations are no-ops.  It should be

	pdata->mem_size = rounddown_pow_of_two(pdata->mem_size);
	pdata->record_size = rounddown_pow_of_two(pdata->record_size);

Marco or Sergio: please fix, test and send it over sometime?


drivers/scsi/bnx2i/bnx2i_hwi.c does

                if (!is_power_of_2(hba->max_sqes))
                        hba->max_sqes = rounddown_pow_of_two(hba->max_sqes);

                if (!is_power_of_2(hba->max_rqes))
                        hba->max_rqes = rounddown_pow_of_two(hba->max_rqes);


Both the "if" statements can and should be removed.  I would blame upon
inadequate documentation of rounddown_pow_of_two().


drivers/scsi/be2iscsi/be_main.c does

	if (curr_alloc_size - rounddown_pow_of_two(curr_alloc_size))
		curr_alloc_size = rounddown_pow_of_two(curr_alloc_size);

which is a strange way of doing

	if (!is_power_of_2(curr_alloc_size))
		curr_alloc_size = rounddown_pow_of_two(curr_alloc_size);

which is equivalent to doing

	curr_alloc_size = rounddown_pow_of_two(curr_alloc_size);

but there's an `else' clause to that `if' which I am presently finding
incomprehensible.


drivers/mmc/host/sh_mmcif.c unnecessarily uses __rounddown_pow_of_two()
then feeds the result into ilog2() in an apparent attempt to
reimplement fls().



That we have this many warts using these interfaces is an indication
that the interfaces aren't very good.  Poorly documented, at least.

Re: [opensuse-kernel] Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Cristian Rodríguez]

On 17/11/11 20:05, Andrew Morton wrote:

> I assume that nobody has gone off and checked whether all current
> callers will survive this change.  If they had, they'd have looked in
> drivers/char/ramoops.c and seen:
> 
> 	rounddown_pow_of_two(pdata->mem_size);
> 	rounddown_pow_of_two(pdata->record_size);
> 
> These operations are no-ops.  It should be
> 
> 	pdata->mem_size = rounddown_pow_of_two(pdata->mem_size);
> 	pdata->record_size = rounddown_pow_of_two(pdata->record_size);
> 
> That we have this many warts using these interfaces is an indication
> that the interfaces aren't very good.  Poorly documented, at least.
> 

making that macro an inline function and annotating with
__attribute__((warn_unused_result)) looks like a good start for me.

Re: [opensuse-kernel] Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Andrew Morton]

On Thu, 17 Nov 2011 20:19:06 -0300
Cristian Rodr__guez <crrodriguez@opensuse.org> wrote:

> On 17/11/11 20:05, Andrew Morton wrote:
> 
> > I assume that nobody has gone off and checked whether all current
> > callers will survive this change.  If they had, they'd have looked in
> > drivers/char/ramoops.c and seen:
> > 
> > 	rounddown_pow_of_two(pdata->mem_size);
> > 	rounddown_pow_of_two(pdata->record_size);
> > 
> > These operations are no-ops.  It should be
> > 
> > 	pdata->mem_size = rounddown_pow_of_two(pdata->mem_size);
> > 	pdata->record_size = rounddown_pow_of_two(pdata->record_size);
> > 
> > That we have this many warts using these interfaces is an indication
> > that the interfaces aren't very good.  Poorly documented, at least.
> > 
> 
> making that macro an inline function and annotating with
> __attribute__((warn_unused_result)) looks like a good start for me.

The problem is:

 * - this can be used to initialise global variables from constant data

I'm surprised that this is true.  Is gcc smart enough to actually do
this?

<tests it>

--- a/fs/open.c~a
+++ a/fs/open.c
@@ -31,6 +31,10 @@
 #include <linux/ima.h>
 #include <linux/dnotify.h>
 
+#include <linux/log2.h>
+
+int blap = rounddown_pow_of_two(42);
+
 #include "internal.h"
 
 int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
_

ooh, it worked.

Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Andrei Warkentin]

Hi,

----- Original Message -----
> From: "Andrew Morton" <akpm@linux-foundation.org>
> To: "Andrei Warkentin" <andreiw@vmware.com>
> Cc: linux-kernel@vger.kernel.org, "Rolf Eike Beer" <eike-kernel@sf-tec.de>, opensuse-kernel@opensuse.org, "Sergiu
> Iordache" <sergiu@chromium.org>, "Marco Stornelli" <marco.stornelli@gmail.com>, "Eddie Wai"
> <eddie.wai@broadcom.com>, "Jayamohan Kallickal" <jayamohan.kallickal@emulex.com>, "Guennadi Liakhovetski"
> <g.liakhovetski@gmx.de>
> Sent: Thursday, November 17, 2011 6:05:49 PM
> Subject: Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)
> 
> 
> I assume that nobody has gone off and checked whether all current
> callers will survive this change.  If they had, they'd have looked in
> drivers/char/ramoops.c and seen:
> 
> 	rounddown_pow_of_two(pdata->mem_size);
> 	rounddown_pow_of_two(pdata->record_size);
> 
> These operations are no-ops.  It should be
> 
> 	pdata->mem_size = rounddown_pow_of_two(pdata->mem_size);
> 	pdata->record_size = rounddown_pow_of_two(pdata->record_size);
> 
> Marco or Sergio: please fix, test and send it over sometime?

I did quickly look through for code that expected rounddown_pow_of_two(1) to give 0,
but I didn't apparently look close enough for other issues.

A

Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Marco Stornelli]

Il 18/11/2011 00:05, Andrew Morton ha scritto:
> On Wed, 16 Nov 2011 14:56:06 -0500
> Andrei Warkentin<andreiw@vmware.com>  wrote:
>
>> 1 is a power of two, therefore rounddown_pow_of_two(1) should return 1. It does
>> in case the argument is a variable but in case it's a constant it behaves
>> wrong and returns 0. Probably nobody ever did it so this was never noticed,
>> however net/drivers/vmxnet3 with latest GCC does and breaks on unicpu systems.
>>
>> This is similar to Rolf's patch to roundup_pow_of_two(1).
>>
>> Cc: Rolf Eike Beer<eike-kernel@sf-tec.de>
>> Cc: opensuse-kernel@opensuse.org
>> Reviewed-by: Jesper Juhl<jj@chaosbits.net>
>> Signed-off-by: Andrei Warkentin<andreiw@vmware.com>
>> ---
>>   include/linux/log2.h |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/linux/log2.h b/include/linux/log2.h
>> index 25b8086..ccda848 100644
>> --- a/include/linux/log2.h
>> +++ b/include/linux/log2.h
>> @@ -185,7 +185,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
>>   #define rounddown_pow_of_two(n)			\
>>   (						\
>>   	__builtin_constant_p(n) ? (		\
>> -		(n == 1) ? 0 :			\
>> +		(n == 1) ? 1 :			\
>>   		(1UL<<  ilog2(n))) :		\
>>   	__rounddown_pow_of_two(n)		\
>>    )
>
> I assume that nobody has gone off and checked whether all current
> callers will survive this change.  If they had, they'd have looked in
> drivers/char/ramoops.c and seen:
>
> 	rounddown_pow_of_two(pdata->mem_size);
> 	rounddown_pow_of_two(pdata->record_size);
>
> These operations are no-ops.  It should be
>
> 	pdata->mem_size = rounddown_pow_of_two(pdata->mem_size);
> 	pdata->record_size = rounddown_pow_of_two(pdata->record_size);
>
> Marco or Sergio: please fix, test and send it over sometime?

Ok, I'll look at it.

Marco

Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Guennadi Liakhovetski]

On Thu, 17 Nov 2011, Andrew Morton wrote:

[snip]

> drivers/mmc/host/sh_mmcif.c unnecessarily uses __rounddown_pow_of_two()
> then feeds the result into ilog2() in an apparent attempt to
> reimplement fls().

Yeah, I was wondering too, but wasn't sufficiently motivated to patch 
it;-) I'll test this "obvious" patch tomorrow and submit then:

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index c021482..824fee5 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -16,6 +16,7 @@
  *
  */
 
+#include <linux/bitops.h>
 #include <linux/clk.h>
 #include <linux/completion.h>
 #include <linux/delay.h>
@@ -386,7 +387,7 @@ static void sh_mmcif_clock_control(struct sh_mmcif_host *host, unsigned int clk)
 		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_SUP_PCLK);
 	else
 		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_CLEAR &
-			(ilog2(__rounddown_pow_of_two(host->clk / clk)) << 16));
+				((fls(host->clk / clk) - 1) << 16));
 
 	sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_ENABLE);
 }

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

[PATCH] mmc: sh_mmcif: simplify clock divisor calculation

[Guennadi Liakhovetski]

Replace ilog2(__rounddown_pow_of_two(x)) with the equivalent but much
simpler fls(x) - 1.

Reported-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/mmc/host/sh_mmcif.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index c021482..824fee5 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -16,6 +16,7 @@
  *
  */
 
+#include <linux/bitops.h>
 #include <linux/clk.h>
 #include <linux/completion.h>
 #include <linux/delay.h>
@@ -386,7 +387,7 @@ static void sh_mmcif_clock_control(struct sh_mmcif_host *host, unsigned int clk)
 		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_SUP_PCLK);
 	else
 		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_CLEAR &
-			(ilog2(__rounddown_pow_of_two(host->clk / clk)) << 16));
+				((fls(host->clk / clk) - 1) << 16));
 
 	sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_ENABLE);
 }
-- 
1.7.2.5

[PATCH] mmc: sh_mmcif: simplify clock divisor calculation

[Guennadi Liakhovetski]

Replace ilog2(__rounddown_pow_of_two(x)) with the equivalent but much
simpler fls(x) - 1.

Reported-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/mmc/host/sh_mmcif.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index c021482..824fee5 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -16,6 +16,7 @@
  *
  */
 
+#include <linux/bitops.h>
 #include <linux/clk.h>
 #include <linux/completion.h>
 #include <linux/delay.h>
@@ -386,7 +387,7 @@ static void sh_mmcif_clock_control(struct sh_mmcif_host *host, unsigned int clk)
 		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_SUP_PCLK);
 	else
 		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_CLEAR &
-			(ilog2(__rounddown_pow_of_two(host->clk / clk)) << 16));
+				((fls(host->clk / clk) - 1) << 16));
 
 	sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_ENABLE);
 }
-- 
1.7.2.5

Re: [PATCH] mmc: sh_mmcif: simplify clock divisor calculation

[Chris Ball]

Hi,

On Wed, Nov 23 2011, Guennadi Liakhovetski wrote:
> Replace ilog2(__rounddown_pow_of_two(x)) with the equivalent but much
> simpler fls(x) - 1.
>
> Reported-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/mmc/host/sh_mmcif.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index c021482..824fee5 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -16,6 +16,7 @@
>   *
>   */
>  
> +#include <linux/bitops.h>
>  #include <linux/clk.h>
>  #include <linux/completion.h>
>  #include <linux/delay.h>
> @@ -386,7 +387,7 @@ static void sh_mmcif_clock_control(struct sh_mmcif_host *host, unsigned int clk)
>  		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_SUP_PCLK);
>  	else
>  		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_CLEAR &
> -			(ilog2(__rounddown_pow_of_two(host->clk / clk)) << 16));
> +				((fls(host->clk / clk) - 1) << 16));
>  
>  	sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_ENABLE);
>  }

Thanks, pushed to mmc-next for 3.3.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: sh_mmcif: simplify clock divisor calculation

[Chris Ball]

Hi,

On Wed, Nov 23 2011, Guennadi Liakhovetski wrote:
> Replace ilog2(__rounddown_pow_of_two(x)) with the equivalent but much
> simpler fls(x) - 1.
>
> Reported-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/mmc/host/sh_mmcif.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index c021482..824fee5 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -16,6 +16,7 @@
>   *
>   */
>  
> +#include <linux/bitops.h>
>  #include <linux/clk.h>
>  #include <linux/completion.h>
>  #include <linux/delay.h>
> @@ -386,7 +387,7 @@ static void sh_mmcif_clock_control(struct sh_mmcif_host *host, unsigned int clk)
>  		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_SUP_PCLK);
>  	else
>  		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_CLEAR &
> -			(ilog2(__rounddown_pow_of_two(host->clk / clk)) << 16));
> +				((fls(host->clk / clk) - 1) << 16));
>  
>  	sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_ENABLE);
>  }

Thanks, pushed to mmc-next for 3.3.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Dmitry Torokhov]

From: Andrei Warkentin <andreiw@vmware.com>

1 is a power of two, therefore rounddown_pow_of_two(1) should return 1.
It does in case the argument is a variable but in case it's a constant
it behaves incorrectly and returns 0. Probably nobody ever did it so
this was never noticed, however drivers/net/vmxnet3 with latest GCC does
and breaks on unicpu systems.

This is similar to Rolf's patch to roundup_pow_of_two(1).

Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
Reviewed-by: Jesper Juhl <jj@chaosbits.net>
Reviewed-by: Rolf Eike Beer <eike-kernel@sf-tec.de>
Cc: stable@kernel.org
Signed-off-by: Dmitry Torokhov <dtor@vmware.com>
---
 include/linux/log2.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/log2.h b/include/linux/log2.h
index 25b8086..ccda848 100644
--- a/include/linux/log2.h
+++ b/include/linux/log2.h
@@ -185,7 +185,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
 #define rounddown_pow_of_two(n)			\
 (						\
 	__builtin_constant_p(n) ? (		\
-		(n == 1) ? 0 :			\
+		(n == 1) ? 1 :			\
 		(1UL << ilog2(n))) :		\
 	__rounddown_pow_of_two(n)		\
  )
-- 
1.7.4.1

Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Linus Torvalds]

On Mon, Dec 12, 2011 at 10:02 AM, Dmitry Torokhov <dtor@vmware.com> wrote:
> From: Andrei Warkentin <andreiw@vmware.com>
>
> 1 is a power of two, therefore rounddown_pow_of_two(1) should return 1.
> It does in case the argument is a variable but in case it's a constant
> it behaves incorrectly and returns 0. Probably nobody ever did it so
> this was never noticed, however drivers/net/vmxnet3 with latest GCC does
> and breaks on unicpu systems.
>
> This is similar to Rolf's patch to roundup_pow_of_two(1).

Umm. I already applied this patch, but then I started looking at it
more, and asked myself:

 - Why is that "n == 1" test there AT ALL?

Afaik, that whole test is just plain stupid. It seems to have been
copied from the "roundup()" case (where it exists due to the "-1/+1"
hackery that breaks ilog2()) without any thought about the actual math
of the function at all.

I think the *real* fix is to just remove that incorrect line, no?

It's a bit sad that we apparently have several reviewers for this
trivial patch, and nobody reacted to the math just not making any
sense.

                                      Linus

Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Dmitry Torokhov]

On Mon, Dec 12, 2011 at 03:50:11PM -0800, Linus Torvalds wrote:
> On Mon, Dec 12, 2011 at 10:02 AM, Dmitry Torokhov <dtor@vmware.com> wrote:
> > From: Andrei Warkentin <andreiw@vmware.com>
> >
> > 1 is a power of two, therefore rounddown_pow_of_two(1) should return 1.
> > It does in case the argument is a variable but in case it's a constant
> > it behaves incorrectly and returns 0. Probably nobody ever did it so
> > this was never noticed, however drivers/net/vmxnet3 with latest GCC does
> > and breaks on unicpu systems.
> >
> > This is similar to Rolf's patch to roundup_pow_of_two(1).
> 
> Umm. I already applied this patch, but then I started looking at it
> more, and asked myself:
> 
>  - Why is that "n == 1" test there AT ALL?
> 
> Afaik, that whole test is just plain stupid. It seems to have been
> copied from the "roundup()" case (where it exists due to the "-1/+1"
> hackery that breaks ilog2()) without any thought about the actual math
> of the function at all.
> 
> I think the *real* fix is to just remove that incorrect line, no?

Yes, you are right, special-casing for 1 is not necessary in rounddown
case.

Thanks,
Dmitry

Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Peter Zijlstra]

On Mon, 2011-12-12 at 10:02 -0800, Dmitry Torokhov wrote:
> 1 is a power of two, therefore rounddown_pow_of_two(1) should return 1.

x^0 = 1
x^1 = x

Seems to me 0 was the right answer, no?

Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Peter Zijlstra]

On Tue, 2011-12-13 at 21:00 +0100, Peter Zijlstra wrote:
> On Mon, 2011-12-12 at 10:02 -0800, Dmitry Torokhov wrote:
> > 1 is a power of two, therefore rounddown_pow_of_two(1) should return 1.
> 
> x^0 = 1
> x^1 = x
> 
> Seems to me 0 was the right answer, no?

n/m head-up-arse, missed that this wasn't actually a log variant.

[PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Andrei Warkentin]

1 is a power of two, therefore rounddown_pow_of_two(1) should return 1. It does
in case the argument is a variable but in case it's a constant it behaves
wrong and returns 0. Probably nobody ever did it so this was never noticed,
however net/drivers/vmxnet3 with latest GCC does and breaks on unicpu systems.

Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
---
 include/linux/log2.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/log2.h b/include/linux/log2.h
index 25b8086..ccda848 100644
--- a/include/linux/log2.h
+++ b/include/linux/log2.h
@@ -185,7 +185,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
 #define rounddown_pow_of_two(n)			\
 (						\
 	__builtin_constant_p(n) ? (		\
-		(n == 1) ? 0 :			\
+		(n == 1) ? 1 :			\
 		(1UL << ilog2(n))) :		\
 	__rounddown_pow_of_two(n)		\
  )
-- 
1.7.4.1

Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Jesper Juhl]

On Tue, 8 Nov 2011, Andrei Warkentin wrote:

> 1 is a power of two, therefore rounddown_pow_of_two(1) should return 1. It does
> in case the argument is a variable but in case it's a constant it behaves
> wrong and returns 0. Probably nobody ever did it so this was never noticed,
> however net/drivers/vmxnet3 with latest GCC does and breaks on unicpu systems.
> 
> Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
> ---
>  include/linux/log2.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/log2.h b/include/linux/log2.h
> index 25b8086..ccda848 100644
> --- a/include/linux/log2.h
> +++ b/include/linux/log2.h
> @@ -185,7 +185,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
>  #define rounddown_pow_of_two(n)			\
>  (						\
>  	__builtin_constant_p(n) ? (		\
> -		(n == 1) ? 0 :			\
> +		(n == 1) ? 1 :			\
>  		(1UL << ilog2(n))) :		\
>  	__rounddown_pow_of_two(n)		\
>   )
> 

For what it's worth; looks good to me.

-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Andrei Warkentin]

----- Original Message -----
> From: "Jesper Juhl" <jj@chaosbits.net>
> To: "Andrei Warkentin" <andreiw@vmware.com>
> Cc: linux-kernel@vger.kernel.org
> Sent: Tuesday, November 8, 2011 2:57:27 PM
> Subject: Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)
> 
> On Tue, 8 Nov 2011, Andrei Warkentin wrote:
> 
> > 1 is a power of two, therefore rounddown_pow_of_two(1) should
> > return 1. It does
> > in case the argument is a variable but in case it's a constant it
> > behaves
> > wrong and returns 0. Probably nobody ever did it so this was never
> > noticed,
> > however net/drivers/vmxnet3 with latest GCC does and breaks on
> > unicpu systems.
> > 
> > Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
> > ---
> >  include/linux/log2.h |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/include/linux/log2.h b/include/linux/log2.h
> > index 25b8086..ccda848 100644
> > --- a/include/linux/log2.h
> > +++ b/include/linux/log2.h
> > @@ -185,7 +185,7 @@ unsigned long __rounddown_pow_of_two(unsigned
> > long n)
> >  #define rounddown_pow_of_two(n)			\
> >  (						\
> >  	__builtin_constant_p(n) ? (		\
> > -		(n == 1) ? 0 :			\
> > +		(n == 1) ? 1 :			\
> >  		(1UL << ilog2(n))) :		\
> >  	__rounddown_pow_of_two(n)		\
> >   )
> > 
> 
> For what it's worth; looks good to me.

Is that an Acked-by or a Reviewed-by?

Thanks,
A

Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Jesper Juhl]

On Mon, 14 Nov 2011, Andrei Warkentin wrote:

> ----- Original Message -----
> > From: "Jesper Juhl" <jj@chaosbits.net>
> > To: "Andrei Warkentin" <andreiw@vmware.com>
> > Cc: linux-kernel@vger.kernel.org
> > Sent: Tuesday, November 8, 2011 2:57:27 PM
> > Subject: Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)
> > 
> > On Tue, 8 Nov 2011, Andrei Warkentin wrote:
> > 
> > > 1 is a power of two, therefore rounddown_pow_of_two(1) should
> > > return 1. It does
> > > in case the argument is a variable but in case it's a constant it
> > > behaves
> > > wrong and returns 0. Probably nobody ever did it so this was never
> > > noticed,
> > > however net/drivers/vmxnet3 with latest GCC does and breaks on
> > > unicpu systems.
> > > 
> > > Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
> > > ---
> > >  include/linux/log2.h |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/include/linux/log2.h b/include/linux/log2.h
> > > index 25b8086..ccda848 100644
> > > --- a/include/linux/log2.h
> > > +++ b/include/linux/log2.h
> > > @@ -185,7 +185,7 @@ unsigned long __rounddown_pow_of_two(unsigned
> > > long n)
> > >  #define rounddown_pow_of_two(n)			\
> > >  (						\
> > >  	__builtin_constant_p(n) ? (		\
> > > -		(n == 1) ? 0 :			\
> > > +		(n == 1) ? 1 :			\
> > >  		(1UL << ilog2(n))) :		\
> > >  	__rounddown_pow_of_two(n)		\
> > >   )
> > > 
> > 
> > For what it's worth; looks good to me.
> 
> Is that an Acked-by or a Reviewed-by?
> 
I don't really know (to be honest). I guess it is whichever is weakest, 
since I did review the patch and convinced myself that the change was 
sane, but didn't spent a whole lot of time checking that *all* users were 
OK with it...


-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.