* [PATCH] powerpc: Fix _ALIGN_* errors due to type difference.
@ 2015-10-02 14:33 Aneesh Kumar K.V
2015-10-06 9:48 ` Michael Ellerman
0 siblings, 1 reply; 4+ messages in thread
From: Aneesh Kumar K.V @ 2015-10-02 14:33 UTC (permalink / raw)
To: benh, paulus, mpe; +Cc: linuxppc-dev, Aneesh Kumar K.V
This avoid errors like
unsigned int usize = 1 << 30;
int size = 1 << 30;
unsigned long addr = 64UL << 30 ;
value = _ALIGN_DOWN(addr, usize); -> 0
value = _ALIGN_DOWN(addr, size); -> 0x1000000000
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
arch/powerpc/boot/page.h | 4 ++--
arch/powerpc/include/asm/page.h | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/boot/page.h b/arch/powerpc/boot/page.h
index 14eca30fef64..87c42d7d283d 100644
--- a/arch/powerpc/boot/page.h
+++ b/arch/powerpc/boot/page.h
@@ -22,8 +22,8 @@
#define PAGE_MASK (~(PAGE_SIZE-1))
/* align addr on a size boundary - adjust address up/down if needed */
-#define _ALIGN_UP(addr,size) (((addr)+((size)-1))&(~((size)-1)))
-#define _ALIGN_DOWN(addr,size) ((addr)&(~((size)-1)))
+#define _ALIGN_UP(addr, size) (((addr)+((size)-1))&(~((typeof(addr))(size)-1)))
+#define _ALIGN_DOWN(addr, size) ((addr)&(~((typeof(addr))(size)-1)))
/* align addr on a size boundary - adjust address up if needed */
#define _ALIGN(addr,size) _ALIGN_UP(addr,size)
diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
index 71294a6e976e..1dd69774a31c 100644
--- a/arch/powerpc/include/asm/page.h
+++ b/arch/powerpc/include/asm/page.h
@@ -240,8 +240,8 @@ extern long long virt_phys_offset;
#endif
/* align addr on a size boundary - adjust address up/down if needed */
-#define _ALIGN_UP(addr,size) (((addr)+((size)-1))&(~((size)-1)))
-#define _ALIGN_DOWN(addr,size) ((addr)&(~((size)-1)))
+#define _ALIGN_UP(addr, size) (((addr)+((size)-1))&(~((typeof(addr))(size)-1)))
+#define _ALIGN_DOWN(addr, size) ((addr)&(~((typeof(addr))(size)-1)))
/* align addr on a size boundary - adjust address up if needed */
#define _ALIGN(addr,size) _ALIGN_UP(addr,size)
--
2.5.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: powerpc: Fix _ALIGN_* errors due to type difference.
2015-10-02 14:33 [PATCH] powerpc: Fix _ALIGN_* errors due to type difference Aneesh Kumar K.V
@ 2015-10-06 9:48 ` Michael Ellerman
2015-10-07 8:46 ` Aneesh Kumar K.V
0 siblings, 1 reply; 4+ messages in thread
From: Michael Ellerman @ 2015-10-06 9:48 UTC (permalink / raw)
To: Aneesh Kumar K.V, benh, paulus; +Cc: linuxppc-dev, Aneesh Kumar K.V
On Fri, 2015-02-10 at 14:33:48 UTC, "Aneesh Kumar K.V" wrote:
> This avoid errors like
>
> unsigned int usize = 1 << 30;
> int size = 1 << 30;
> unsigned long addr = 64UL << 30 ;
>
> value = _ALIGN_DOWN(addr, usize); -> 0
> value = _ALIGN_DOWN(addr, size); -> 0x1000000000
Are you actually seeing that anywhere? I assume not.
> diff --git a/arch/powerpc/boot/page.h b/arch/powerpc/boot/page.h
> index 14eca30fef64..87c42d7d283d 100644
> --- a/arch/powerpc/boot/page.h
> +++ b/arch/powerpc/boot/page.h
> @@ -22,8 +22,8 @@
> #define PAGE_MASK (~(PAGE_SIZE-1))
>
> /* align addr on a size boundary - adjust address up/down if needed */
> -#define _ALIGN_UP(addr,size) (((addr)+((size)-1))&(~((size)-1)))
> -#define _ALIGN_DOWN(addr,size) ((addr)&(~((size)-1)))
> +#define _ALIGN_UP(addr, size) (((addr)+((size)-1))&(~((typeof(addr))(size)-1)))
> +#define _ALIGN_DOWN(addr, size) ((addr)&(~((typeof(addr))(size)-1)))
>
> /* align addr on a size boundary - adjust address up if needed */
> #define _ALIGN(addr,size) _ALIGN_UP(addr,size)
> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
> index 71294a6e976e..1dd69774a31c 100644
> --- a/arch/powerpc/include/asm/page.h
> +++ b/arch/powerpc/include/asm/page.h
> @@ -240,8 +240,8 @@ extern long long virt_phys_offset;
> #endif
>
> /* align addr on a size boundary - adjust address up/down if needed */
> -#define _ALIGN_UP(addr,size) (((addr)+((size)-1))&(~((size)-1)))
> -#define _ALIGN_DOWN(addr,size) ((addr)&(~((size)-1)))
> +#define _ALIGN_UP(addr, size) (((addr)+((size)-1))&(~((typeof(addr))(size)-1)))
> +#define _ALIGN_DOWN(addr, size) ((addr)&(~((typeof(addr))(size)-1)))
It looks like ALIGN() in kernel.h already does this right, so can we just use
that instead for _ALIGN_UP() at least.
cheers
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: powerpc: Fix _ALIGN_* errors due to type difference.
2015-10-06 9:48 ` Michael Ellerman
@ 2015-10-07 8:46 ` Aneesh Kumar K.V
2015-10-07 9:16 ` Michael Ellerman
0 siblings, 1 reply; 4+ messages in thread
From: Aneesh Kumar K.V @ 2015-10-07 8:46 UTC (permalink / raw)
To: Michael Ellerman, benh, paulus; +Cc: linuxppc-dev
Michael Ellerman <mpe@ellerman.id.au> writes:
> On Fri, 2015-02-10 at 14:33:48 UTC, "Aneesh Kumar K.V" wrote:
>> This avoid errors like
>>
>> unsigned int usize = 1 << 30;
>> int size = 1 << 30;
>> unsigned long addr = 64UL << 30 ;
>>
>> value = _ALIGN_DOWN(addr, usize); -> 0
>> value = _ALIGN_DOWN(addr, size); -> 0x1000000000
>
> Are you actually seeing that anywhere? I assume not.
I hit that in new development. So not in the current kernel.
>
>> diff --git a/arch/powerpc/boot/page.h b/arch/powerpc/boot/page.h
>> index 14eca30fef64..87c42d7d283d 100644
>> --- a/arch/powerpc/boot/page.h
>> +++ b/arch/powerpc/boot/page.h
>> @@ -22,8 +22,8 @@
>> #define PAGE_MASK (~(PAGE_SIZE-1))
>>
>> /* align addr on a size boundary - adjust address up/down if needed */
>> -#define _ALIGN_UP(addr,size) (((addr)+((size)-1))&(~((size)-1)))
>> -#define _ALIGN_DOWN(addr,size) ((addr)&(~((size)-1)))
>> +#define _ALIGN_UP(addr, size) (((addr)+((size)-1))&(~((typeof(addr))(size)-1)))
>> +#define _ALIGN_DOWN(addr, size) ((addr)&(~((typeof(addr))(size)-1)))
>>
>> /* align addr on a size boundary - adjust address up if needed */
>> #define _ALIGN(addr,size) _ALIGN_UP(addr,size)
>> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
>> index 71294a6e976e..1dd69774a31c 100644
>> --- a/arch/powerpc/include/asm/page.h
>> +++ b/arch/powerpc/include/asm/page.h
>> @@ -240,8 +240,8 @@ extern long long virt_phys_offset;
>> #endif
>>
>> /* align addr on a size boundary - adjust address up/down if needed */
>> -#define _ALIGN_UP(addr,size) (((addr)+((size)-1))&(~((size)-1)))
>> -#define _ALIGN_DOWN(addr,size) ((addr)&(~((size)-1)))
>> +#define _ALIGN_UP(addr, size) (((addr)+((size)-1))&(~((typeof(addr))(size)-1)))
>> +#define _ALIGN_DOWN(addr, size) ((addr)&(~((typeof(addr))(size)-1)))
>
>
> It looks like ALIGN() in kernel.h already does this right, so can we just use
> that instead for _ALIGN_UP() at least.
>
But we still can't get rid of _ALIGN_UP, because that is used in other
parts of the kernel and if you are suggesting use
#define _ALIGN_UP __ALIGN_KERNEL, IMHO that is unnecessary indirection
for no real benefit.
-aneesh
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: powerpc: Fix _ALIGN_* errors due to type difference.
2015-10-07 8:46 ` Aneesh Kumar K.V
@ 2015-10-07 9:16 ` Michael Ellerman
0 siblings, 0 replies; 4+ messages in thread
From: Michael Ellerman @ 2015-10-07 9:16 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: benh, paulus, linuxppc-dev
On Wed, 2015-10-07 at 14:16 +0530, Aneesh Kumar K.V wrote:
> Michael Ellerman <mpe@ellerman.id.au> writes:
>
> > On Fri, 2015-02-10 at 14:33:48 UTC, "Aneesh Kumar K.V" wrote:
> >> This avoid errors like
> >>
> >> unsigned int usize = 1 << 30;
> >> int size = 1 << 30;
> >> unsigned long addr = 64UL << 30 ;
> >>
> >> value = _ALIGN_DOWN(addr, usize); -> 0
> >> value = _ALIGN_DOWN(addr, size); -> 0x1000000000
> >
> > Are you actually seeing that anywhere? I assume not.
>
> I hit that in new development. So not in the current kernel.
OK.
> >> diff --git a/arch/powerpc/boot/page.h b/arch/powerpc/boot/page.h
> >> index 14eca30fef64..87c42d7d283d 100644
> >> --- a/arch/powerpc/boot/page.h
> >> +++ b/arch/powerpc/boot/page.h
> >> @@ -22,8 +22,8 @@
> >> #define PAGE_MASK (~(PAGE_SIZE-1))
> >>
> >> /* align addr on a size boundary - adjust address up/down if needed */
> >> -#define _ALIGN_UP(addr,size) (((addr)+((size)-1))&(~((size)-1)))
> >> -#define _ALIGN_DOWN(addr,size) ((addr)&(~((size)-1)))
> >> +#define _ALIGN_UP(addr, size) (((addr)+((size)-1))&(~((typeof(addr))(size)-1)))
> >> +#define _ALIGN_DOWN(addr, size) ((addr)&(~((typeof(addr))(size)-1)))
> >>
> >> /* align addr on a size boundary - adjust address up if needed */
> >> #define _ALIGN(addr,size) _ALIGN_UP(addr,size)
> >> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
> >> index 71294a6e976e..1dd69774a31c 100644
> >> --- a/arch/powerpc/include/asm/page.h
> >> +++ b/arch/powerpc/include/asm/page.h
> >> @@ -240,8 +240,8 @@ extern long long virt_phys_offset;
> >> #endif
> >>
> >> /* align addr on a size boundary - adjust address up/down if needed */
> >> -#define _ALIGN_UP(addr,size) (((addr)+((size)-1))&(~((size)-1)))
> >> -#define _ALIGN_DOWN(addr,size) ((addr)&(~((size)-1)))
> >> +#define _ALIGN_UP(addr, size) (((addr)+((size)-1))&(~((typeof(addr))(size)-1)))
> >> +#define _ALIGN_DOWN(addr, size) ((addr)&(~((typeof(addr))(size)-1)))
> >
> >
> > It looks like ALIGN() in kernel.h already does this right, so can we just use
> > that instead for _ALIGN_UP() at least.
> >
>
> But we still can't get rid of _ALIGN_UP, because that is used in other
> parts of the kernel and if you are suggesting use
>
> #define _ALIGN_UP __ALIGN_KERNEL, IMHO that is unnecessary indirection
> for no real benefit.
Huh? The benefit is not having to define the same macro twice, that seems like
a pretty clear win.
_ALIGN_UP is ALIGN, just with this bug unfixed.
In other words if _ALIGN_UP was already an alias for ALIGN then you would have
not seen the bug (except in _ALIGN_DOWN()).
cheers
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-10-07 9:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-10-02 14:33 [PATCH] powerpc: Fix _ALIGN_* errors due to type difference Aneesh Kumar K.V
2015-10-06 9:48 ` Michael Ellerman
2015-10-07 8:46 ` Aneesh Kumar K.V
2015-10-07 9:16 ` Michael Ellerman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).