linux-kernel - Re: [patch] powerpc: change u64/s64 to a long long integer type

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20081231154027.457d9197.sfr@canb.auug.org.au>
Date:	Wed, 31 Dec 2008 15:40:27 +1100
From:	Stephen Rothwell <sfr@...b.auug.org.au>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	linux-kernel@...r.kernel.org, Ken Chen <kenchen@...gle.com>,
	Paul Mackerras <paulus@...ba.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>, linux-next@...r.kernel.org
Subject: Re: [patch] powerpc: change u64/s64 to a long long integer type

Hi Ingo,

On Mon, 22 Dec 2008 09:03:41 +0100 Ingo Molnar <mingo@...e.hu> wrote:
>
> Subject: powerpc: change u64/s64 to a long long integer type
> From: Ingo Molnar <mingo@...e.hu>
> Date: Mon Dec 22 08:32:41 CET 2008
> 
> Convert arch/powerpc/ over to long long based u64:
> 
>  -#ifdef __powerpc64__
>  -# include <asm-generic/int-l64.h>
>  -#else
>  -# include <asm-generic/int-ll64.h>
>  -#endif
>  +#include <asm-generic/int-ll64.h>
> 
> This will avoid reoccuring spurious warnings in core kernel code that 
> comes when people test on their own hardware. (i.e. x86 in ~98% of the 
> cases) This is what x86 uses and it generally helps keep 64-bit code 
> 32-bit clean too.

Thanks for this great start.  Just a few comments ...

Firstly, it would be nice if we could split this into things we can do
now (e.g. logical bug fixes) and things that must be done at the time of
the u64 type change (e.g. the printk's etc).  That will make the second
patch hopefully somewhat smaller.

> Index: linux/arch/powerpc/kernel/prom.c
> ===================================================================
> --- linux.orig/arch/powerpc/kernel/prom.c
> +++ linux/arch/powerpc/kernel/prom.c
> @@ -824,11 +824,11 @@ static int __init early_init_dt_scan_cho
>  #endif
>  
>  #ifdef CONFIG_KEXEC
> -	lprop = (u64*)of_get_flat_dt_prop(node, "linux,crashkernel-base", NULL);
> +	lprop = (unsigned long *)of_get_flat_dt_prop(node, "linux,crashkernel-base", NULL);
>  	if (lprop)
>  		crashk_res.start = *lprop;
>  
> -	lprop = (u64*)of_get_flat_dt_prop(node, "linux,crashkernel-size", NULL);
> +	lprop = (unsigned long *)of_get_flat_dt_prop(node, "linux,crashkernel-size", NULL);

These casts are actually not needed at all as of_get_flat_dt_prop()
returns "void *".

> Index: linux/arch/powerpc/oprofile/cell/vma_map.c
> ===================================================================
> --- linux.orig/arch/powerpc/oprofile/cell/vma_map.c
> +++ linux/arch/powerpc/oprofile/cell/vma_map.c
> @@ -92,7 +92,7 @@ vma_map_add(struct vma_to_fileoffset_map
>   * A pointer to the first vma_map in the generated list
>   * of vma_maps is returned.  */
>  struct vma_to_fileoffset_map *create_vma_map(const struct spu *aSpu,
> -					     unsigned long __spu_elf_start)
> +					     u64 __spu_elf_start)

Wouldn't it make more sense to change the prototype to match the
implementation and the only caller (which passes an "unsigned long")?

> Index: linux/arch/powerpc/platforms/cell/interrupt.c
> ===================================================================
> --- linux.orig/arch/powerpc/platforms/cell/interrupt.c
> +++ linux/arch/powerpc/platforms/cell/interrupt.c
> @@ -148,7 +148,7 @@ static unsigned int iic_get_irq(void)
>  
>  	iic = &__get_cpu_var(iic);
>  	*(unsigned long *) &pending =
> -		in_be64((unsigned long __iomem *) &iic->regs->pending_destr);
> +		in_be64((unsigned long long __iomem *) &iic->regs->pending_destr);

in_be64()'s argument is "const volatile u64 __iomem *" so the
original "unsigned long" should have been "u64".

> Index: linux/arch/powerpc/platforms/cell/spu_base.c
> ===================================================================
> --- linux.orig/arch/powerpc/platforms/cell/spu_base.c
> +++ linux/arch/powerpc/platforms/cell/spu_base.c
> @@ -139,10 +139,10 @@ static void spu_restart_dma(struct spu *
>  {
>  	struct spu_priv2 __iomem *priv2 = spu->priv2;
>  
> -	if (!test_bit(SPU_CONTEXT_SWITCH_PENDING, &spu->flags))
> +	if (!test_bit(SPU_CONTEXT_SWITCH_PENDING, (unsigned long *)&spu->flags))
>  		out_be64(&priv2->mfc_control_RW, MFC_CNTL_RESTART_DMA_COMMAND);
>  	else {
> -		set_bit(SPU_CONTEXT_FAULT_PENDING, &spu->flags);
> +		set_bit(SPU_CONTEXT_FAULT_PENDING, (unsigned long *)&spu->flags);

I have submitted a different patch for this.  The bitops work on unsigned
longs, so I changed the "flags" to be unsigned long.

So, would you like me to push this along - including splitting it up a
bit (keeping your Signed-off-by, of course)?
-- 
Cheers,
Stephen Rothwell                    sfr@...b.auug.org.au
http://www.canb.auug.org.au/~sfr/

Content of type "application/pgp-signature" skipped