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

Andrei Warkentin

16 Nov 2011 16 Nov '11

19:56

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 Cc: opensuse-kernel@opensuse.org Reviewed-by: Jesper Juhl Signed-off-by: Andrei Warkentin --- 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 -- To unsubscribe, e-mail: opensuse-kernel+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-kernel+owner@opensuse.org

Show replies by date

Rolf Eike Beer

16 Nov 16 Nov

22:51

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

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

Andrew Morton

17 Nov 17 Nov

23:05

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

On Wed, 16 Nov 2011 14:56:06 -0500 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.

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

Cc: Rolf Eike Beer Cc: opensuse-kernel@opensuse.org Reviewed-by: Jesper Juhl Signed-off-by: Andrei Warkentin --- 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. -- To unsubscribe, e-mail: opensuse-kernel+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-kernel+owner@opensuse.org

Cristian Rodríguez

23:19

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

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. -- To unsubscribe, e-mail: opensuse-kernel+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-kernel+owner@opensuse.org

Andrew Morton

23:32

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

On Thu, 17 Nov 2011 20:19:06 -0300 Cristian Rodr__guez 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 #include +#include + +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. -- To unsubscribe, e-mail: opensuse-kernel+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-kernel+owner@opensuse.org

Andrei Warkentin

18 Nov 18 Nov

18:46

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

Hi, ----- Original Message -----

...

From: "Andrew Morton" To: "Andrei Warkentin" Cc: linux-kernel@vger.kernel.org, "Rolf Eike Beer" , opensuse-kernel@opensuse.org, "Sergiu Iordache" , "Marco Stornelli" , "Eddie Wai" , "Jayamohan Kallickal" , "Guennadi Liakhovetski" 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 -- To unsubscribe, e-mail: opensuse-kernel+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-kernel+owner@opensuse.org

Marco Stornelli

19 Nov 19 Nov

09:26

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

Il 18/11/2011 00:05, Andrew Morton ha scritto:

...

On Wed, 16 Nov 2011 14:56:06 -0500 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.

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

Cc: Rolf Eike Beer Cc: opensuse-kernel@opensuse.org Reviewed-by: Jesper Juhl Signed-off-by: Andrei Warkentin --- 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 -- To unsubscribe, e-mail: opensuse-kernel+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-kernel+owner@opensuse.org

Guennadi Liakhovetski

22 Nov 22 Nov

23:34

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

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 #include #include #include @@ -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/ -- To unsubscribe, e-mail: opensuse-kernel+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-kernel+owner@opensuse.org

4540

Age (days ago)

4546

Last active (days ago)

List overview

Download

7 comments

7 participants

participants (7)

Andrei Warkentin
Andrei Warkentin
Andrew Morton
Cristian Rodríguez
Guennadi Liakhovetski
Marco Stornelli
Rolf Eike Beer