| Message ID | 1301989401-11984-1-git-send-email-j.weitzel@phytec.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 05, 2011 at 08:56:22AM +0100, Russell King - ARM Linux wrote: > On Tue, Apr 05, 2011 at 09:43:21AM +0200, Jan Weitzel wrote: > > parameter "u32 mask" type cast befor inversion s/befor/before/ > Nak. I want a 32-bit all ones quantity. > > unsigned long long vali = (unsigned short)~0; > unsigned long long vall = ~(unsigned short)0; > BTW, the definiton of vall is equivalent to unsigned long long valu = ~(unsigned int)0; because ~ converts the unsigned short to unsigned int. > compiles to: > > vali: > .word 65535 > .word 0 > > vall: > .word -1 > .word -1 I really wonder about that. If I take a value of 0xffffffff (i.e. a 32 bit wide int == ~0U) and assign that to an 64-bit unsigned long long I'd expect it to get the value 0x00000000ffffffffULL, not 0xffffffffffffffffULL. What's wrong? > So moving the ~ to be evaluated after the cast has the effect of making > the cast pointless, and produces wrong values. (u32)~0 does the 32-bit My C-Book tells that using ~ on a signed it produces implementation defined behaviour. That's what I pointed out to Jan and I guess that's the reason why he created the patch. > cast _after_ the inversion which ensures that its always truncated to > a 32-bit value. > > As the function is declared as taking a u32, the cast isn't needed. If > the function gets changed to take a u64, the casts will need to be > re-added. So, (u32)~0 makes the fact that we want a 32-bit all-ones > mask explicit. Actually this only results in a 32-bit all-ones value if int is at least 32 bits long, so technically using ~(u32)0 is better. (Obviously this is given on ARM and I guess even on all platforms that Linux runs on.) Best regards Uwe PS: this mail is not about trolling.
On Tue, Apr 05, 2011 at 10:31:44AM +0200, Uwe Kleine-König wrote: > On Tue, Apr 05, 2011 at 08:56:22AM +0100, Russell King - ARM Linux wrote: > > On Tue, Apr 05, 2011 at 09:43:21AM +0200, Jan Weitzel wrote: > > > parameter "u32 mask" type cast befor inversion > s/befor/before/ > > > Nak. I want a 32-bit all ones quantity. > > > > unsigned long long vali = (unsigned short)~0; > > unsigned long long vall = ~(unsigned short)0; > > > BTW, the definiton of vall is equivalent to > > unsigned long long valu = ~(unsigned int)0; > > because ~ converts the unsigned short to unsigned int. No. The value gets promoted to int not unsigned int. > > compiles to: > > > > vali: > > .word 65535 > > .word 0 > > > > vall: > > .word -1 > > .word -1 > I really wonder about that. If I take a value of 0xffffffff (i.e. a 32 > bit wide int == ~0U) and assign that to an 64-bit unsigned long long I'd > expect it to get the value 0x00000000ffffffffULL, not > 0xffffffffffffffffULL. What's wrong? See above. int not unsigned int. And it makes a difference: unsigned long long vals = ~(int)0; unsigned long long valu = ~(unsigned int)0; vals: .word -1 .word -1 valu: .word -1 .word 0 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
linux-arm-kernel-bounces@lists.infradead.org schrieb am 05.04.2011 10:31:44: > Von: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > An: Russell King - ARM Linux <linux@arm.linux.org.uk> > Kopie: rubini@unipv.it, kgene.kim@samsung.com, > eric.y.miao@gmail.com, ben-linux@fluff.org, tony@atomide.com, Jan > Weitzel <j.weitzel@phytec.de>, linux-tegra@vger.kernel.org, linux- > omap@vger.kernel.org, kernel@pengutronix.de, > STEricsson_nomadik_linux@list.st.com, trivial@kernel.org, > kaloz@openwrt.org, linux-arm-kernel@lists.infradead.org, khc@pm.waw.pl > Datum: 05.04.2011 10:33 > Betreff: Re: [PATCH] ARM: type casts update_sched_clock > cyc_to_sched_clock cyc_to_fixed_sched_clock > Gesendet von: linux-arm-kernel-bounces@lists.infradead.org > > On Tue, Apr 05, 2011 at 08:56:22AM +0100, Russell King - ARM Linux wrote: > > On Tue, Apr 05, 2011 at 09:43:21AM +0200, Jan Weitzel wrote: > > > parameter "u32 mask" type cast befor inversion > s/befor/before/ > > > Nak. I want a 32-bit all ones quantity. > > > > unsigned long long vali = (unsigned short)~0; > > unsigned long long vall = ~(unsigned short)0; > > > BTW, the definiton of vall is equivalent to > > unsigned long long valu = ~(unsigned int)0; > > because ~ converts the unsigned short to unsigned int. > > > compiles to: > > > > vali: > > .word 65535 > > .word 0 > > > > vall: > > .word -1 > > .word -1 > I really wonder about that. If I take a value of 0xffffffff (i.e. a 32 > bit wide int == ~0U) and assign that to an 64-bit unsigned long long I'd > expect it to get the value 0x00000000ffffffffULL, not > 0xffffffffffffffffULL. What's wrong? > > > So moving the ~ to be evaluated after the cast has the effect of making > > the cast pointless, and produces wrong values. (u32)~0 does the 32-bit > My C-Book tells that using ~ on a signed it produces implementation > defined behaviour. That's what I pointed out to Jan and I guess that's > the reason why he created the patch. That's right. I tested it and got for both static unsigned long long valuica = (unsigned int)~0; static unsigned long long valuicb = ~(unsigned int)0; the same: .word 0xffffffff .word 0x00000000 With unsigned short I see the problem. Sorry for the noise. Kind regards Jan -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 05, 2011 at 10:01:00AM +0100, Russell King - ARM Linux wrote: > On Tue, Apr 05, 2011 at 10:31:44AM +0200, Uwe Kleine-König wrote: > > On Tue, Apr 05, 2011 at 08:56:22AM +0100, Russell King - ARM Linux wrote: > > > On Tue, Apr 05, 2011 at 09:43:21AM +0200, Jan Weitzel wrote: > > > > parameter "u32 mask" type cast befor inversion > > s/befor/before/ > > > > > Nak. I want a 32-bit all ones quantity. > > > > > > unsigned long long vali = (unsigned short)~0; > > > unsigned long long vall = ~(unsigned short)0; > > > > > BTW, the definiton of vall is equivalent to > > > > unsigned long long valu = ~(unsigned int)0; > > > > because ~ converts the unsigned short to unsigned int. > > No. The value gets promoted to int not unsigned int. > > > > compiles to: > > > > > > vali: > > > .word 65535 > > > .word 0 > > > > > > vall: > > > .word -1 > > > .word -1 > > I really wonder about that. If I take a value of 0xffffffff (i.e. a 32 > > bit wide int == ~0U) and assign that to an 64-bit unsigned long long I'd > > expect it to get the value 0x00000000ffffffffULL, not > > 0xffffffffffffffffULL. What's wrong? > > See above. int not unsigned int. And it makes a difference: > > unsigned long long vals = ~(int)0; > unsigned long long valu = ~(unsigned int)0; > > vals: > .word -1 > .word -1 > valu: > .word -1 > .word 0 > ah, ok, makes sense and actually is consistent with my book. (After knowing what I search I found it. The rules are: - A signed type of rank less than int -> int - An unsigned type of rank less than int, all of whose values can be represented in type int -> int - An unsigned type of rank less than int, all of whose values cannot be represented in type int -> unsigned int ). So the maximal correct variant is (u32)(int)~0U or alternatively (u32)(-1), right? Thanks for your answer, best regards Uwe
On Tue, Apr 05, 2011 at 01:12:57PM +0200, Uwe Kleine-König wrote: > ah, ok, makes sense and actually is consistent with my book. > (After knowing what I search I found it. The rules are: > > - A signed type of rank less than int > -> int > - An unsigned type of rank less than int, > all of whose values can be represented in type int > -> int > - An unsigned type of rank less than int, > all of whose values cannot be represented in type int > -> unsigned int > ). > > So the maximal correct variant is (u32)(int)~0U or alternatively > (u32)(-1), right? If you really want to be pedantic, (u32)~0UL, as we assume that longs will always be equal or larger than 32-bit throughout the kernel. The u32 cast then becomes truncating in itself. But... in the interests of avoiding churn, we know the current code works, we know that its safe should we chose to change the function argument to be a u64, so lets leave it as-is. We know the only time that it'd break is if an 'int' becomes less than 32-bit, which we aren't going to see on ARM any time soon. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-ixp4xx/common.c b/arch/arm/mach-ixp4xx/common.c index ed19bc3..20338bc 100644 --- a/arch/arm/mach-ixp4xx/common.c +++ b/arch/arm/mach-ixp4xx/common.c @@ -407,13 +407,13 @@ static DEFINE_CLOCK_DATA(cd); unsigned long long notrace sched_clock(void) { u32 cyc = *IXP4XX_OSTS; - return cyc_to_sched_clock(&cd, cyc, (u32)~0); + return cyc_to_sched_clock(&cd, cyc, ~(u32)0); } static void notrace ixp4xx_update_sched_clock(void) { u32 cyc = *IXP4XX_OSTS; - update_sched_clock(&cd, cyc, (u32)~0); + update_sched_clock(&cd, cyc, ~(u32)0); } /* diff --git a/arch/arm/mach-mmp/time.c b/arch/arm/mach-mmp/time.c index aeb9ae2..78d178f 100644 --- a/arch/arm/mach-mmp/time.c +++ b/arch/arm/mach-mmp/time.c @@ -62,13 +62,13 @@ static inline uint32_t timer_read(void) unsigned long long notrace sched_clock(void) { u32 cyc = timer_read(); - return cyc_to_sched_clock(&cd, cyc, (u32)~0); + return cyc_to_sched_clock(&cd, cyc, ~(u32)0); } static void notrace mmp_update_sched_clock(void) { u32 cyc = timer_read(); - update_sched_clock(&cd, cyc, (u32)~0); + update_sched_clock(&cd, cyc, ~(u32)0); } static irqreturn_t timer_interrupt(int irq, void *dev_id) diff --git a/arch/arm/mach-omap1/time.c b/arch/arm/mach-omap1/time.c index 6885d2f..830e4d8 100644 --- a/arch/arm/mach-omap1/time.c +++ b/arch/arm/mach-omap1/time.c @@ -221,7 +221,7 @@ static DEFINE_CLOCK_DATA(cd); static inline unsigned long long notrace _omap_mpu_sched_clock(void) { u32 cyc = mpu_read(&clocksource_mpu); - return cyc_to_sched_clock(&cd, cyc, (u32)~0); + return cyc_to_sched_clock(&cd, cyc, ~(u32)0); } #ifndef CONFIG_OMAP_32K_TIMER @@ -239,7 +239,7 @@ static unsigned long long notrace omap_mpu_sched_clock(void) static void notrace mpu_update_sched_clock(void) { u32 cyc = mpu_read(&clocksource_mpu); - update_sched_clock(&cd, cyc, (u32)~0); + update_sched_clock(&cd, cyc, ~(u32)0); } static void __init omap_init_clocksource(unsigned long rate) diff --git a/arch/arm/mach-pxa/time.c b/arch/arm/mach-pxa/time.c index 428da3f..57e1432 100644 --- a/arch/arm/mach-pxa/time.c +++ b/arch/arm/mach-pxa/time.c @@ -37,13 +37,13 @@ static DEFINE_CLOCK_DATA(cd); unsigned long long notrace sched_clock(void) { u32 cyc = OSCR; - return cyc_to_sched_clock(&cd, cyc, (u32)~0); + return cyc_to_sched_clock(&cd, cyc, ~(u32)0); } static void notrace pxa_update_sched_clock(void) { u32 cyc = OSCR; - update_sched_clock(&cd, cyc, (u32)~0); + update_sched_clock(&cd, cyc, ~(u32)0); } diff --git a/arch/arm/mach-sa1100/time.c b/arch/arm/mach-sa1100/time.c index ae4f3d8..0251d67 100644 --- a/arch/arm/mach-sa1100/time.c +++ b/arch/arm/mach-sa1100/time.c @@ -36,13 +36,13 @@ static DEFINE_CLOCK_DATA(cd); unsigned long long notrace sched_clock(void) { u32 cyc = OSCR; - return cyc_to_fixed_sched_clock(&cd, cyc, (u32)~0, SC_MULT, SC_SHIFT); + return cyc_to_fixed_sched_clock(&cd, cyc, ~(u32)0, SC_MULT, SC_SHIFT); } static void notrace sa1100_update_sched_clock(void) { u32 cyc = OSCR; - update_sched_clock(&cd, cyc, (u32)~0); + update_sched_clock(&cd, cyc, ~(u32)0); } #define MIN_OSCR_DELTA 2 diff --git a/arch/arm/mach-tegra/timer.c b/arch/arm/mach-tegra/timer.c index 0fcb1eb..6394dfc 100644 --- a/arch/arm/mach-tegra/timer.c +++ b/arch/arm/mach-tegra/timer.c @@ -131,13 +131,13 @@ static DEFINE_CLOCK_DATA(cd); unsigned long long notrace sched_clock(void) { u32 cyc = timer_readl(TIMERUS_CNTR_1US); - return cyc_to_fixed_sched_clock(&cd, cyc, (u32)~0, SC_MULT, SC_SHIFT); + return cyc_to_fixed_sched_clock(&cd, cyc, ~(u32)0, SC_MULT, SC_SHIFT); } static void notrace tegra_update_sched_clock(void) { u32 cyc = timer_readl(TIMERUS_CNTR_1US); - update_sched_clock(&cd, cyc, (u32)~0); + update_sched_clock(&cd, cyc, ~(u32)0); } /* diff --git a/arch/arm/mach-u300/timer.c b/arch/arm/mach-u300/timer.c index 3ec58bd..825b138 100644 --- a/arch/arm/mach-u300/timer.c +++ b/arch/arm/mach-u300/timer.c @@ -359,13 +359,13 @@ static DEFINE_CLOCK_DATA(cd); unsigned long long notrace sched_clock(void) { u32 cyc = readl(U300_TIMER_APP_VBASE + U300_TIMER_APP_GPT2CC); - return cyc_to_sched_clock(&cd, cyc, (u32)~0); + return cyc_to_sched_clock(&cd, cyc, ~(u32)0); } static void notrace u300_update_sched_clock(void) { u32 cyc = readl(U300_TIMER_APP_VBASE + U300_TIMER_APP_GPT2CC); - update_sched_clock(&cd, cyc, (u32)~0); + update_sched_clock(&cd, cyc, ~(u32)0); } diff --git a/arch/arm/plat-iop/time.c b/arch/arm/plat-iop/time.c index 07f23bb..e851900 100644 --- a/arch/arm/plat-iop/time.c +++ b/arch/arm/plat-iop/time.c @@ -60,13 +60,13 @@ static DEFINE_CLOCK_DATA(cd); unsigned long long notrace sched_clock(void) { u32 cyc = 0xffffffffu - read_tcr1(); - return cyc_to_sched_clock(&cd, cyc, (u32)~0); + return cyc_to_sched_clock(&cd, cyc, ~(u32)0); } static void notrace iop_update_sched_clock(void) { u32 cyc = 0xffffffffu - read_tcr1(); - update_sched_clock(&cd, cyc, (u32)~0); + update_sched_clock(&cd, cyc, ~(u32)0); } /* diff --git a/arch/arm/plat-mxc/time.c b/arch/arm/plat-mxc/time.c index 2237ff8..20017f3 100644 --- a/arch/arm/plat-mxc/time.c +++ b/arch/arm/plat-mxc/time.c @@ -134,13 +134,13 @@ unsigned long long notrace sched_clock(void) { cycle_t cyc = clocksource_mxc.read(&clocksource_mxc); - return cyc_to_sched_clock(&cd, cyc, (u32)~0); + return cyc_to_sched_clock(&cd, cyc, ~(u32)0); } static void notrace mxc_update_sched_clock(void) { cycle_t cyc = clocksource_mxc.read(&clocksource_mxc); - update_sched_clock(&cd, cyc, (u32)~0); + update_sched_clock(&cd, cyc, ~(u32)0); } static int __init mxc_clocksource_init(struct clk *timer_clk) diff --git a/arch/arm/plat-nomadik/timer.c b/arch/arm/plat-nomadik/timer.c index 4172340..d307440 100644 --- a/arch/arm/plat-nomadik/timer.c +++ b/arch/arm/plat-nomadik/timer.c @@ -63,13 +63,13 @@ unsigned long long notrace sched_clock(void) return 0; cyc = -readl(mtu_base + MTU_VAL(0)); - return cyc_to_sched_clock(&cd, cyc, (u32)~0); + return cyc_to_sched_clock(&cd, cyc, ~(u32)0); } static void notrace nomadik_update_sched_clock(void) { u32 cyc = -readl(mtu_base + MTU_VAL(0)); - update_sched_clock(&cd, cyc, (u32)~0); + update_sched_clock(&cd, cyc, ~(u32)0); } /* Clockevent device: use one-shot mode */ diff --git a/arch/arm/plat-omap/counter_32k.c b/arch/arm/plat-omap/counter_32k.c index f7fed60..bb7b7a2 100644 --- a/arch/arm/plat-omap/counter_32k.c +++ b/arch/arm/plat-omap/counter_32k.c @@ -123,7 +123,7 @@ static DEFINE_CLOCK_DATA(cd); static inline unsigned long long notrace _omap_32k_sched_clock(void) { u32 cyc = clocksource_32k.read(&clocksource_32k); - return cyc_to_fixed_sched_clock(&cd, cyc, (u32)~0, SC_MULT, SC_SHIFT); + return cyc_to_fixed_sched_clock(&cd, cyc, ~(u32)0, SC_MULT, SC_SHIFT); } #ifndef CONFIG_OMAP_MPU_TIMER @@ -141,7 +141,7 @@ unsigned long long notrace omap_32k_sched_clock(void) static void notrace omap_update_sched_clock(void) { u32 cyc = clocksource_32k.read(&clocksource_32k); - update_sched_clock(&cd, cyc, (u32)~0); + update_sched_clock(&cd, cyc, ~(u32)0); } /** diff --git a/arch/arm/plat-orion/time.c b/arch/arm/plat-orion/time.c index 742b032..60f6291 100644 --- a/arch/arm/plat-orion/time.c +++ b/arch/arm/plat-orion/time.c @@ -65,14 +65,14 @@ static DEFINE_CLOCK_DATA(cd); unsigned long long notrace sched_clock(void) { u32 cyc = ~readl(timer_base + TIMER0_VAL_OFF); - return cyc_to_sched_clock(&cd, cyc, (u32)~0); + return cyc_to_sched_clock(&cd, cyc, ~(u32)0); } static void notrace orion_update_sched_clock(void) { u32 cyc = ~readl(timer_base + TIMER0_VAL_OFF); - update_sched_clock(&cd, cyc, (u32)~0); + update_sched_clock(&cd, cyc, ~(u32)0); } static void __init setup_sched_clock(unsigned long tclk) diff --git a/arch/arm/plat-s5p/s5p-time.c b/arch/arm/plat-s5p/s5p-time.c index 8090403..1a1f7ab 100644 --- a/arch/arm/plat-s5p/s5p-time.c +++ b/arch/arm/plat-s5p/s5p-time.c @@ -346,7 +346,7 @@ unsigned long long notrace sched_clock(void) } cyc = ~__raw_readl(S3C_TIMERREG(offset)); - return cyc_to_sched_clock(&cd, cyc, (u32)~0); + return cyc_to_sched_clock(&cd, cyc, ~(u32)0); } static void notrace s5p_update_sched_clock(void) @@ -371,7 +371,7 @@ static void notrace s5p_update_sched_clock(void) } cyc = ~__raw_readl(S3C_TIMERREG(offset)); - update_sched_clock(&cd, cyc, (u32)~0); + update_sched_clock(&cd, cyc, ~(u32)0); } struct clocksource time_clocksource = { diff --git a/arch/arm/plat-versatile/sched-clock.c b/arch/arm/plat-versatile/sched-clock.c index 3d6a4c2..7fa3418 100644 --- a/arch/arm/plat-versatile/sched-clock.c +++ b/arch/arm/plat-versatile/sched-clock.c @@ -38,7 +38,7 @@ unsigned long long notrace sched_clock(void) { if (ctr) { u32 cyc = readl(ctr); - return cyc_to_fixed_sched_clock(&cd, cyc, (u32)~0, + return cyc_to_fixed_sched_clock(&cd, cyc, ~(u32)0, SC_MULT, SC_SHIFT); } else return 0; @@ -47,7 +47,7 @@ unsigned long long notrace sched_clock(void) static void notrace versatile_update_sched_clock(void) { u32 cyc = readl(ctr); - update_sched_clock(&cd, cyc, (u32)~0); + update_sched_clock(&cd, cyc, ~(u32)0); } void __init versatile_sched_clock_init(void __iomem *reg, unsigned long rate)
parameter "u32 mask" type cast befor inversion Signed-off-by: Jan Weitzel <j.weitzel@phytec.de> --- arch/arm/mach-ixp4xx/common.c | 4 ++-- arch/arm/mach-mmp/time.c | 4 ++-- arch/arm/mach-omap1/time.c | 4 ++-- arch/arm/mach-pxa/time.c | 4 ++-- arch/arm/mach-sa1100/time.c | 4 ++-- arch/arm/mach-tegra/timer.c | 4 ++-- arch/arm/mach-u300/timer.c | 4 ++-- arch/arm/plat-iop/time.c | 4 ++-- arch/arm/plat-mxc/time.c | 4 ++-- arch/arm/plat-nomadik/timer.c | 4 ++-- arch/arm/plat-omap/counter_32k.c | 4 ++-- arch/arm/plat-orion/time.c | 4 ++-- arch/arm/plat-s5p/s5p-time.c | 4 ++-- arch/arm/plat-versatile/sched-clock.c | 4 ++-- 14 files changed, 28 insertions(+), 28 deletions(-)