include/log2.h: Fix rounddown_pow_of_two(1)

PATCH In review Andrei Warkentin 8 participants 10 messages Nov 16, 2011 – Dec 1, 2011

Subsystems (3)

VFS MMC, SD and SDIO SuperH

lore

Share & tools

lore

Andrei Warkentin

author

2011-11-16 19:56 UTC #0 lore raw

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

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 <[email protected]>

Cc [email protected]

Reviewed-by Jesper Juhl <[email protected]>

Signed-off-by Andrei Warkentin <[email protected]>

---
 include/linux/log2.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

include/linux/log2.h +1 -1

index 25b8086..ccda848 100644
@@ -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)		\
  )

Signature · 1 line

1.7.4.1

Rolf Eike Beer

Reviewed-by on v1 1/1

2011-11-16 22:51 UTC #1 lore raw

Quoted 5 lines · 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.

Obviously correct.

Reviewed-by Rolf Eike Beer <[email protected]>

Andrew Morton

2011-11-17 23:05 UTC #2 lore raw

In reply to Andrei Warkentin

Quoted 20 lines · On Wed, 16 Nov 2011 14:56:06 -0500 Andrei Warkentin <[email protected]> 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 <[email protected]>
Cc [email protected]
Reviewed-by Jesper Juhl <[email protected]>
Signed-off-by Andrei Warkentin <[email protected]>
---
 include/linux/log2.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
include/linux/log2.h +0 -0
index 25b8086..ccda848 100644

include/linux/log2.h unsigned long __rounddown_pow_of_two(unsigned long n) · line 191

Quoted 8 lines

include/linux/log2.h +1 -1

@@ -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.

Cristian Rodríguez

2011-11-17 23:19 UTC #3 lore raw

Quoted 7 lines · Andrew Morton

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);

Quoted 8 lines

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.

Andrew Morton

2011-11-17 23:32 UTC #4 lore raw

Quoted 14 lines · On Thu, 17 Nov 2011 20:19:06 -0300 Cristian Rodr__guez…

Quoted 12 lines · Andrew Morton
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);

Quoted 6 lines

Quoted 3 lines
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?

fs/open.c renamed from fs/open.c~a +4 -0

@@ -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.

Andrei Warkentin

2011-11-18 18:46 UTC #5 lore raw

In reply to Andrew Morton

Hi,

----- Original Message -----

Quoted 17 lines

From: "Andrew Morton" <[email protected]>
To: "Andrei Warkentin" <[email protected]>
Cc: [email protected], "Rolf Eike Beer" <[email protected]>, [email protected], "Sergiu Iordache" <[email protected]>, "Marco Stornelli" <[email protected]>, "Eddie Wai" <[email protected]>, "Jayamohan Kallickal" <[email protected]>, "Guennadi Liakhovetski" <[email protected]>
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);

Quoted 6 lines

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.

Marco Stornelli

2011-11-19 09:33 UTC #6 lore raw

In reply to Andrew Morton

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

Quoted 39 lines

Quoted 20 lines · On Wed, 16 Nov 2011 14:56:06 -0500 Andrei Warkentin<[email protected]> 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<[email protected]>
Cc [email protected]
Reviewed-by Jesper Juhl<[email protected]>
Signed-off-by Andrei Warkentin<[email protected]>
---
  include/linux/log2.h |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)
include/linux/log2.h +0 -0
index 25b8086..ccda848 100644
include/linux/log2.h unsigned long __rounddown_pow_of_two(unsigned long n) · line 191
Quoted 8 lines
include/linux/log2.h +1 -1
@@ -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);

Quoted 6 lines

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

Guennadi Liakhovetski

2011-11-22 23:35 UTC #7 lore raw

In reply to Andrew Morton

On Thu, 17 Nov 2011, Andrew Morton wrote:

[snip]

Quoted 3 lines

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:

drivers/mmc/host/sh_mmcif.c +2 -1

index c021482..824fee5 100644
@@ -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/

Guennadi Liakhovetski

2011-11-23 14:52 UTC #8 lore raw

PATCH 1/1 mmc: sh_mmcif: simplify clock divisor calculation

In reply to Andrew Morton

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

Reported-by Andrew Morton <[email protected]>

Signed-off-by Guennadi Liakhovetski <[email protected]>

---
 drivers/mmc/host/sh_mmcif.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

drivers/mmc/host/sh_mmcif.c +2 -1

index c021482..824fee5 100644
@@ -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);
 }

Signature · 1 line

1.7.2.5

Chris Ball

2011-12-01 18:24 UTC #9 lore raw

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

Hi,

Quoted 22 lines · Guennadi Liakhovetski

Replace ilog2(__rounddown_pow_of_two(x)) with the equivalent but much simpler fls(x) - 1.
Reported-by Andrew Morton <[email protected]>
Signed-off-by Guennadi Liakhovetski <[email protected]>
---
 drivers/mmc/host/sh_mmcif.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
drivers/mmc/host/sh_mmcif.c +1 -0
index c021482..824fee5 100644
@@ -16,6 +16,7 @@
  *
  */
 
+#include <linux/bitops.h>
 #include <linux/clk.h>
 #include <linux/completion.h>
 #include <linux/delay.h>

drivers/mmc/host/sh_mmcif.c static void sh_mmcif_clock_control(struct sh_mmcif_host *host, unsigned int clk) · line 393

Quoted 8 lines

drivers/mmc/host/sh_mmcif.c +1 -1

@@ -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.

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child