[PATCH] kgdb, ppc: Corrected the address using of current->thread.evr register

[PATCH] kgdb, ppc: Corrected the address using of current->thread.evr register

[Dongdong Deng]

Passing the address of current->thread.evr register to memcpy function.

Signed-off-by: Dongdong Deng <dongdong.deng@windriver.com>
CC: Hai Shan <shan.hai@windriver.com>
CC: Milton Miller <miltonm@bga.com>
CC: linuxppc-dev@lists.ozlabs.org
---
 arch/powerpc/kernel/kgdb.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/kgdb.c b/arch/powerpc/kernel/kgdb.c
index 7a9db64..781acff 100644
--- a/arch/powerpc/kernel/kgdb.c
+++ b/arch/powerpc/kernel/kgdb.c
@@ -337,7 +337,7 @@ char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs)
 		/* FP registers 32 -> 63 */
 #if defined(CONFIG_FSL_BOOKE) && defined(CONFIG_SPE)
 		if (current)
-			memcpy(mem, current->thread.evr[regno-32],
+			memcpy(mem, (void *)&current->thread.evr[regno-32],
 					dbg_reg_def[regno].size);
 #else
 		/* fp registers not used by kernel, leave zero */
@@ -362,7 +362,7 @@ int dbg_set_reg(int regno, void *mem, struct pt_regs *regs)
 	if (regno >= 32 && regno < 64) {
 		/* FP registers 32 -> 63 */
 #if defined(CONFIG_FSL_BOOKE) && defined(CONFIG_SPE)
-		memcpy(current->thread.evr[regno-32], mem,
+		memcpy((void *)&current->thread.evr[regno-32], mem,
 				dbg_reg_def[regno].size);
 #else
 		/* fp registers not used by kernel, leave zero */
-- 
1.6.0.4

Re: [Kgdb-bugreport] [PATCH] kgdb, ppc: Corrected the address using of current->thread.evr register

[Sergei Shtylyov]

Hello.

On 16-11-2010 15:58, Dongdong Deng wrote:

> Passing the address of current->thread.evr register to memcpy function.

> Signed-off-by: Dongdong Deng<dongdong.deng@windriver.com>
> CC: Hai Shan<shan.hai@windriver.com>
> CC: Milton Miller<miltonm@bga.com>
> CC: linuxppc-dev@lists.ozlabs.org
> ---
>   arch/powerpc/kernel/kgdb.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)

> diff --git a/arch/powerpc/kernel/kgdb.c b/arch/powerpc/kernel/kgdb.c
> index 7a9db64..781acff 100644
> --- a/arch/powerpc/kernel/kgdb.c
> +++ b/arch/powerpc/kernel/kgdb.c
> @@ -337,7 +337,7 @@ char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs)
>   		/* FP registers 32 ->  63 */
>   #if defined(CONFIG_FSL_BOOKE)&&  defined(CONFIG_SPE)
>   		if (current)
> -			memcpy(mem, current->thread.evr[regno-32],
> +			memcpy(mem, (void *)&current->thread.evr[regno-32],
>   					dbg_reg_def[regno].size);
>   #else
>   		/* fp registers not used by kernel, leave zero */
> @@ -362,7 +362,7 @@ int dbg_set_reg(int regno, void *mem, struct pt_regs *regs)
>   	if (regno>= 32&&  regno<  64) {
>   		/* FP registers 32 ->  63 */
>   #if defined(CONFIG_FSL_BOOKE)&&  defined(CONFIG_SPE)
> -		memcpy(current->thread.evr[regno-32], mem,
> +		memcpy((void *)&current->thread.evr[regno-32], mem,

    Doesn't any pointer type get converted to 'void *' automatically? These 
casts are not really needed...

WBR, Sergei

Re: [PATCH] kgdb, ppc: Corrected the address using of current->thread.evr register

[Jason Wessel]

On 11/16/2010 06:58 AM, Dongdong Deng wrote:
> Passing the address of current->thread.evr register to memcpy function.
>
>   

It turns out that out of all of my test configs and targets I did not
have any that defined both CONFIG_FSL_BOOKE and CONFIG_SPE else this
would have been caught in testing because will definitely crash
dereferencing the contents of the evr registers.

> Signed-off-by: Dongdong Deng <dongdong.deng@windriver.com>
> CC: Hai Shan <shan.hai@windriver.com>
> CC: Milton Miller <miltonm@bga.com>
> CC: linuxppc-dev@lists.ozlabs.org
> ---
>  arch/powerpc/kernel/kgdb.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/kgdb.c b/arch/powerpc/kernel/kgdb.c
> index 7a9db64..781acff 100644
> --- a/arch/powerpc/kernel/kgdb.c
> +++ b/arch/powerpc/kernel/kgdb.c
> @@ -337,7 +337,7 @@ char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs)
>  		/* FP registers 32 -> 63 */
>  #if defined(CONFIG_FSL_BOOKE) && defined(CONFIG_SPE)
>  		if (current)
> -			memcpy(mem, current->thread.evr[regno-32],
> +			memcpy(mem, (void *)&current->thread.evr[regno-32],
>  					dbg_reg_def[regno].size);
>   

As Sergei already pointed out, the (void *) casts should not be needed here.

This would have been fixed correctly had I seen the compile warnings
from the CONFIG_SPE at the test phase.   Namely at the compile phase
doesn't even complete:

arch/powerpc/kernel/kgdb.c: In function 'dbg_get_reg':
arch/powerpc/kernel/kgdb.c:341: error: passing argument 2 of 'memcpy'
makes pointer from integer without a cast
arch/powerpc/include/asm/string.h:25: note: expected 'const void *' but
argument is of type 'long unsigned int'
arch/powerpc/kernel/kgdb.c: In function 'dbg_set_reg':
arch/powerpc/kernel/kgdb.c:366: error: passing argument 1 of 'memcpy'
makes pointer from integer without a cast
arch/powerpc/include/asm/string.h:25: note: expected 'void *' but
argument is of type 'long unsigned int'


Moral of the story... I now have a board and config with the SPE turned
on, and after boot testing confirmed the evr registers are working.

I plan to have a pull request out to Linus with this fix and several
other accumulated regression fixes sometime in the next 48 hours.

Thanks,
Jason.

Re: [PATCH] kgdb, ppc: Corrected the address using of current->thread.evr register

[Kumar Gala]

On Nov 16, 2010, at 4:02 PM, Jason Wessel wrote:

> On 11/16/2010 06:58 AM, Dongdong Deng wrote:
>> Passing the address of current->thread.evr register to memcpy =
function.
>>=20
>>=20
>=20
> It turns out that out of all of my test configs and targets I did not
> have any that defined both CONFIG_FSL_BOOKE and CONFIG_SPE else this
> would have been caught in testing because will definitely crash
> dereferencing the contents of the evr registers.
>=20
>> Signed-off-by: Dongdong Deng <dongdong.deng@windriver.com>
>> CC: Hai Shan <shan.hai@windriver.com>
>> CC: Milton Miller <miltonm@bga.com>
>> CC: linuxppc-dev@lists.ozlabs.org
>> ---
>> arch/powerpc/kernel/kgdb.c |    4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>=20
>> diff --git a/arch/powerpc/kernel/kgdb.c b/arch/powerpc/kernel/kgdb.c
>> index 7a9db64..781acff 100644
>> --- a/arch/powerpc/kernel/kgdb.c
>> +++ b/arch/powerpc/kernel/kgdb.c
>> @@ -337,7 +337,7 @@ char *dbg_get_reg(int regno, void *mem, struct =
pt_regs *regs)
>> 		/* FP registers 32 -> 63 */
>> #if defined(CONFIG_FSL_BOOKE) && defined(CONFIG_SPE)
>> 		if (current)
>> -			memcpy(mem, current->thread.evr[regno-32],
>> +			memcpy(mem, (void =
*)&current->thread.evr[regno-32],
>> 					dbg_reg_def[regno].size);
>>=20
>=20
> As Sergei already pointed out, the (void *) casts should not be needed =
here.
>=20
> This would have been fixed correctly had I seen the compile warnings
> from the CONFIG_SPE at the test phase.   Namely at the compile phase
> doesn't even complete:
>=20
> arch/powerpc/kernel/kgdb.c: In function 'dbg_get_reg':
> arch/powerpc/kernel/kgdb.c:341: error: passing argument 2 of 'memcpy'
> makes pointer from integer without a cast
> arch/powerpc/include/asm/string.h:25: note: expected 'const void *' =
but
> argument is of type 'long unsigned int'
> arch/powerpc/kernel/kgdb.c: In function 'dbg_set_reg':
> arch/powerpc/kernel/kgdb.c:366: error: passing argument 1 of 'memcpy'
> makes pointer from integer without a cast
> arch/powerpc/include/asm/string.h:25: note: expected 'void *' but
> argument is of type 'long unsigned int'
>=20
>=20
> Moral of the story... I now have a board and config with the SPE =
turned
> on, and after boot testing confirmed the evr registers are working.
>=20
> I plan to have a pull request out to Linus with this fix and several
> other accumulated regression fixes sometime in the next 48 hours.
>=20
> Thanks,
> Jason.

Repost a version with the casts and I'll pick it up.

- k=

Re: [PATCH] kgdb, ppc: Corrected the address using of current->thread.evr register

[Jason Wessel]

On 11/17/2010 11:16 AM, Kumar Gala wrote:

>> Thanks,
>> Jason.
> 
> Repost a version with the casts and I'll pick it up.
> 
> - k

I have the final version ready for a pull request that I was going to
make in the next hour or two that is fully regression tested.

If you would prefer to these changes in the PPC, let me know, or if
you want to ack the patch, I'll add that before submitting the pull
request.

Thanks,
Jason.

--
>From b47efa1d4fa4631ee0cc59f4fcd143464b910cdc Mon Sep 17 00:00:00 2001
From: Dongdong Deng <dongdong.deng@windriver.com>
Date: Tue, 16 Nov 2010 16:02:00 -0600
Subject: [PATCH 4/4] kgdb: Fix regression in evr register handling

Commit ff10b88b5a05c8f1646dd15fb9f6093c1384ff6d (kgdb,ppc: Individual
register get/set for ppc) introduced a problem where memcpy was used
incorrectly to read and write the evr registers with a kernel that
has:

CONFIG_FSL_BOOKE=y
CONFIG_SPE=y
CONFIG_KGDB=y

This patch also fixes the following compilation problems:

arch/powerpc/kernel/kgdb.c: In function 'dbg_get_reg':
arch/powerpc/kernel/kgdb.c:341: error: passing argument 2 of 'memcpy' makes pointer from integer without a cast
arch/powerpc/kernel/kgdb.c: In function 'dbg_set_reg':
arch/powerpc/kernel/kgdb.c:366: error: passing argument 1 of 'memcpy' makes pointer from integer without a cast

[jason.wessel@windriver.com: Remove void * casts and fix patch header]
Reported-by: Milton Miller <miltonm@bga.com>
Signed-off-by: Dongdong Deng <dongdong.deng@windriver.com>
Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
CC: linuxppc-dev@lists.ozlabs.org
---
 arch/powerpc/kernel/kgdb.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/kgdb.c b/arch/powerpc/kernel/kgdb.c
index 7a9db64..42850ee 100644
--- a/arch/powerpc/kernel/kgdb.c
+++ b/arch/powerpc/kernel/kgdb.c
@@ -337,7 +337,7 @@ char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs)
 		/* FP registers 32 -> 63 */
 #if defined(CONFIG_FSL_BOOKE) && defined(CONFIG_SPE)
 		if (current)
-			memcpy(mem, current->thread.evr[regno-32],
+			memcpy(mem, &current->thread.evr[regno-32],
 					dbg_reg_def[regno].size);
 #else
 		/* fp registers not used by kernel, leave zero */
@@ -362,7 +362,7 @@ int dbg_set_reg(int regno, void *mem, struct pt_regs *regs)
 	if (regno >= 32 && regno < 64) {
 		/* FP registers 32 -> 63 */
 #if defined(CONFIG_FSL_BOOKE) && defined(CONFIG_SPE)
-		memcpy(current->thread.evr[regno-32], mem,
+		memcpy(&current->thread.evr[regno-32], mem,
 				dbg_reg_def[regno].size);
 #else
 		/* fp registers not used by kernel, leave zero */
-- 
1.7.0.4

Re: [PATCH] kgdb, ppc: Corrected the address using of current->thread.evr register

[Kumar Gala]

On Nov 17, 2010, at 11:21 AM, Jason Wessel wrote:

> On 11/17/2010 11:16 AM, Kumar Gala wrote:
>=20
>>> Thanks,
>>> Jason.
>>=20
>> Repost a version with the casts and I'll pick it up.
>>=20
>> - k
>=20
> I have the final version ready for a pull request that I was going to
> make in the next hour or two that is fully regression tested.
>=20
> If you would prefer to these changes in the PPC, let me know, or if
> you want to ack the patch, I'll add that before submitting the pull
> request.
>=20
> Thanks,
> Jason.
>=20
> --

If they'll get pulled in via another path that is fine.  I'm happy to =
ack a proper patch.

- k=

Re: [PATCH] kgdb, ppc: Corrected the address using of current->thread.evr register

[Kumar Gala]

> From: Dongdong Deng <dongdong.deng@windriver.com>
> Date: Tue, 16 Nov 2010 16:02:00 -0600
> Subject: [PATCH 4/4] kgdb: Fix regression in evr register handling
>=20
> Commit ff10b88b5a05c8f1646dd15fb9f6093c1384ff6d (kgdb,ppc: Individual
> register get/set for ppc) introduced a problem where memcpy was used
> incorrectly to read and write the evr registers with a kernel that
> has:
>=20
> CONFIG_FSL_BOOKE=3Dy
> CONFIG_SPE=3Dy
> CONFIG_KGDB=3Dy
>=20
> This patch also fixes the following compilation problems:
>=20
> arch/powerpc/kernel/kgdb.c: In function 'dbg_get_reg':
> arch/powerpc/kernel/kgdb.c:341: error: passing argument 2 of 'memcpy' =
makes pointer from integer without a cast
> arch/powerpc/kernel/kgdb.c: In function 'dbg_set_reg':
> arch/powerpc/kernel/kgdb.c:366: error: passing argument 1 of 'memcpy' =
makes pointer from integer without a cast
>=20
> [jason.wessel@windriver.com: Remove void * casts and fix patch header]
> Reported-by: Milton Miller <miltonm@bga.com>
> Signed-off-by: Dongdong Deng <dongdong.deng@windriver.com>
> Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
> CC: linuxppc-dev@lists.ozlabs.org
> ---
> arch/powerpc/kernel/kgdb.c |    4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>=20
> diff --git a/arch/powerpc/kernel/kgdb.c b/arch/powerpc/kernel/kgdb.c
> index 7a9db64..42850ee 100644
> --- a/arch/powerpc/kernel/kgdb.c
> +++ b/arch/powerpc/kernel/kgdb.c
> @@ -337,7 +337,7 @@ char *dbg_get_reg(int regno, void *mem, struct =
pt_regs *regs)
> 		/* FP registers 32 -> 63 */
> #if defined(CONFIG_FSL_BOOKE) && defined(CONFIG_SPE)
> 		if (current)
> -			memcpy(mem, current->thread.evr[regno-32],
> +			memcpy(mem, &current->thread.evr[regno-32],
> 					dbg_reg_def[regno].size);
> #else
> 		/* fp registers not used by kernel, leave zero */
> @@ -362,7 +362,7 @@ int dbg_set_reg(int regno, void *mem, struct =
pt_regs *regs)
> 	if (regno >=3D 32 && regno < 64) {
> 		/* FP registers 32 -> 63 */
> #if defined(CONFIG_FSL_BOOKE) && defined(CONFIG_SPE)
> -		memcpy(current->thread.evr[regno-32], mem,
> +		memcpy(&current->thread.evr[regno-32], mem,
> 				dbg_reg_def[regno].size);
> #else
> 		/* fp registers not used by kernel, leave zero */
> --=20
> 1.7.0.4

Acked-by: Kumar Gala <galak@kernel.crashing.org>

- k