Impact: fix overflow
Constant 0x123456789abcdef0 is so big for a long.
So using saved_magic 0x12345678 for both 32 and 64 bit.
Signed-off-by: Jaswinder Singh Rajput <[email protected]>
---
arch/x86/kernel/acpi/sleep.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 7c243a2..1ad317a 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -94,7 +94,6 @@ int acpi_save_state_mem(void)
#ifndef CONFIG_64BIT
header->pmode_entry = (u32)&wakeup_pmode_return;
header->pmode_cr3 = (u32)(swsusp_pg_dir - __PAGE_OFFSET);
- saved_magic = 0x12345678;
#else /* CONFIG_64BIT */
header->trampoline_segment = setup_trampoline() >> 4;
#ifdef CONFIG_SMP
@@ -104,8 +103,8 @@ int acpi_save_state_mem(void)
initial_gs = per_cpu_offset(smp_processor_id());
#endif
initial_code = (unsigned long)wakeup_long64;
- saved_magic = 0x123456789abcdef0;
#endif /* CONFIG_64BIT */
+ saved_magic = 0x12345678;
return 0;
}
--
1.6.0.6
[Jaswinder Singh Rajput - Fri, Apr 10, 2009 at 11:06:55PM +0530]
| Impact: fix overflow
|
| Constant 0x123456789abcdef0 is so big for a long.
| So using saved_magic 0x12345678 for both 32 and 64 bit.
|
| Signed-off-by: Jaswinder Singh Rajput <[email protected]>
| ---
| arch/x86/kernel/acpi/sleep.c | 3 +--
| 1 files changed, 1 insertions(+), 2 deletions(-)
|
| diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
| index 7c243a2..1ad317a 100644
| --- a/arch/x86/kernel/acpi/sleep.c
| +++ b/arch/x86/kernel/acpi/sleep.c
| @@ -94,7 +94,6 @@ int acpi_save_state_mem(void)
| #ifndef CONFIG_64BIT
| header->pmode_entry = (u32)&wakeup_pmode_return;
| header->pmode_cr3 = (u32)(swsusp_pg_dir - __PAGE_OFFSET);
| - saved_magic = 0x12345678;
| #else /* CONFIG_64BIT */
| header->trampoline_segment = setup_trampoline() >> 4;
| #ifdef CONFIG_SMP
| @@ -104,8 +103,8 @@ int acpi_save_state_mem(void)
| initial_gs = per_cpu_offset(smp_processor_id());
| #endif
| initial_code = (unsigned long)wakeup_long64;
| - saved_magic = 0x123456789abcdef0;
| #endif /* CONFIG_64BIT */
| + saved_magic = 0x12345678;
|
| return 0;
| }
| --
| 1.6.0.6
|
Hi Jaswinder,
actually long on x86-64 is 8 bytes in size.
Cyrill
On Fri 2009-04-10 23:06:55, Jaswinder Singh Rajput wrote:
> Impact: fix overflow
> Constant 0x123456789abcdef0 is so big for a long.
> So using saved_magic 0x12345678 for both 32 and 64 bit.
Have you tested the patch? I have feeling that this is tested from
assembly... Plus it is only set to long constant on 64-bit, so I don't
think you are fixing anything. And if your code is correct the
variable is write only...
> Signed-off-by: Jaswinder Singh Rajput <[email protected]>
NAK.
Pavel
> ---
> arch/x86/kernel/acpi/sleep.c | 3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
> index 7c243a2..1ad317a 100644
> --- a/arch/x86/kernel/acpi/sleep.c
> +++ b/arch/x86/kernel/acpi/sleep.c
> @@ -94,7 +94,6 @@ int acpi_save_state_mem(void)
> #ifndef CONFIG_64BIT
> header->pmode_entry = (u32)&wakeup_pmode_return;
> header->pmode_cr3 = (u32)(swsusp_pg_dir - __PAGE_OFFSET);
> - saved_magic = 0x12345678;
> #else /* CONFIG_64BIT */
> header->trampoline_segment = setup_trampoline() >> 4;
> #ifdef CONFIG_SMP
> @@ -104,8 +103,8 @@ int acpi_save_state_mem(void)
> initial_gs = per_cpu_offset(smp_processor_id());
> #endif
> initial_code = (unsigned long)wakeup_long64;
> - saved_magic = 0x123456789abcdef0;
> #endif /* CONFIG_64BIT */
> + saved_magic = 0x12345678;
>
> return 0;
> }
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Fri, Apr 10, 2009 at 10:36 AM, Jaswinder Singh Rajput
<[email protected]> wrote:
> Impact: fix overflow
>
> Constant 0x123456789abcdef0 is so big for a long.
> So using saved_magic 0x12345678 for both 32 and 64 bit.
>
Uhm, what? saved_magic is a long, which is 64 bits wide on on 64 bit systems.
On Fri, 2009-04-10 at 19:55 +0200, Pavel Machek wrote:
> On Fri 2009-04-10 23:06:55, Jaswinder Singh Rajput wrote:
> > Impact: fix overflow
>
> > Constant 0x123456789abcdef0 is so big for a long.
> > So using saved_magic 0x12345678 for both 32 and 64 bit.
>
> Have you tested the patch? I have feeling that this is tested from
> assembly... Plus it is only set to long constant on 64-bit, so I don't
> think you are fixing anything. And if your code is correct the
> variable is write only...
>
I have tested this:
1. CHECK arch/x86/kernel/acpi/sleep.c
arch/x86/kernel/acpi/sleep.c:107:16: warning: constant 0x123456789abcdef0 is so big it is long
2. On 64 bit machine box:
long l1;
l1 = 0x123456789abcdef0;
printf("0x%16x : ", l1);
Output: 0x 9abcdef0
--
JSR
On Fri 2009-04-10 23:35:29, Jaswinder Singh Rajput wrote:
> On Fri, 2009-04-10 at 19:55 +0200, Pavel Machek wrote:
> > On Fri 2009-04-10 23:06:55, Jaswinder Singh Rajput wrote:
> > > Impact: fix overflow
> >
> > > Constant 0x123456789abcdef0 is so big for a long.
> > > So using saved_magic 0x12345678 for both 32 and 64 bit.
> >
> > Have you tested the patch? I have feeling that this is tested from
> > assembly... Plus it is only set to long constant on 64-bit, so I don't
> > think you are fixing anything. And if your code is correct the
> > variable is write only...
> >
>
> I have tested this:
> 1. CHECK arch/x86/kernel/acpi/sleep.c
> arch/x86/kernel/acpi/sleep.c:107:16: warning: constant 0x123456789abcdef0 is so big it is long
>
> 2. On 64 bit machine box:
> long l1;
>
> l1 = 0x123456789abcdef0;
> printf("0x%16x : ", l1);
>
> Output: 0x 9abcdef0
I don't have 64-bit box handy, but please make sure that
suspend/resume works before and after your change.
If the variable is _not_ tested from assembly, just remove
saved_magic...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[Jaswinder Singh Rajput - Fri, Apr 10, 2009 at 11:35:29PM +0530]
| On Fri, 2009-04-10 at 19:55 +0200, Pavel Machek wrote:
| > On Fri 2009-04-10 23:06:55, Jaswinder Singh Rajput wrote:
| > > Impact: fix overflow
| >
| > > Constant 0x123456789abcdef0 is so big for a long.
| > > So using saved_magic 0x12345678 for both 32 and 64 bit.
| >
| > Have you tested the patch? I have feeling that this is tested from
| > assembly... Plus it is only set to long constant on 64-bit, so I don't
| > think you are fixing anything. And if your code is correct the
| > variable is write only...
| >
|
| I have tested this:
| 1. CHECK arch/x86/kernel/acpi/sleep.c
| arch/x86/kernel/acpi/sleep.c:107:16: warning: constant 0x123456789abcdef0 is so big it is long
|
| 2. On 64 bit machine box:
| long l1;
|
| l1 = 0x123456789abcdef0;
| printf("0x%16x : ", l1);
|
| Output: 0x 9abcdef0
|
| --
| JSR
|
|
"0x%lx" will help
Cyrill
On Fri, 10 Apr 2009, Jaswinder Singh Rajput wrote:
> Impact: fix overflow
Nothing to fix there.
> Constant 0x123456789abcdef0 is so big for a long.
Really ? long is 64bit on a 64bit machine.
Thanks,
tglx
On Fri, 2009-04-10 at 23:35 +0530, Jaswinder Singh Rajput wrote:
> On Fri, 2009-04-10 at 19:55 +0200, Pavel Machek wrote:
> > On Fri 2009-04-10 23:06:55, Jaswinder Singh Rajput wrote:
> > > Impact: fix overflow
> >
> > > Constant 0x123456789abcdef0 is so big for a long.
> > > So using saved_magic 0x12345678 for both 32 and 64 bit.
> >
> > Have you tested the patch? I have feeling that this is tested from
> > assembly... Plus it is only set to long constant on 64-bit, so I don't
> > think you are fixing anything. And if your code is correct the
> > variable is write only...
> >
>
> I have tested this:
> 1. CHECK arch/x86/kernel/acpi/sleep.c
> arch/x86/kernel/acpi/sleep.c:107:16: warning: constant 0x123456789abcdef0 is so big it is long
>
> 2. On 64 bit machine box:
> long l1;
>
> l1 = 0x123456789abcdef0;
> printf("0x%16x : ", l1);
>
> Output: 0x 9abcdef0
>
OK printf("0x%16llx : ", l1);
Output: 0x123456789abcdef0
--
JSR
On Fri, Apr 10, 2009 at 07:55:45PM +0200, Pavel Machek wrote:
> On Fri 2009-04-10 23:06:55, Jaswinder Singh Rajput wrote:
> > Impact: fix overflow
>
> > Constant 0x123456789abcdef0 is so big for a long.
> > So using saved_magic 0x12345678 for both 32 and 64 bit.
>
> Have you tested the patch? I have feeling that this is tested from
> assembly... Plus it is only set to long constant on 64-bit, so I don't
> think you are fixing anything. And if your code is correct the
> variable is write only...
$ git grep saved_magic
kernel/acpi/sleep.c: saved_magic = 0x12345678;
kernel/acpi/sleep.c: saved_magic = 0x123456789abcdef0;
kernel/acpi/sleep.h:extern long saved_magic;
kernel/acpi/wakeup_32.S: movl %cs:saved_magic, %eax
kernel/acpi/wakeup_32.S:ENTRY(saved_magic) .long 0
kernel/acpi/wakeup_64.S: movq saved_magic, %rax
kernel/acpi/wakeup_64.S:ENTRY(saved_magic) .quad 0
wakeup_32.S:
movl %cs:saved_magic, %eax
cmpl $0x12345678, %eax
wakeup_64.S:
movq saved_magic, %rax
movq $0x123456789abcdef0, %rdx
So patch is obviously bogus.
Sam
On Fri, 2009-04-10 at 21:13 +0200, Sam Ravnborg wrote:
> On Fri, Apr 10, 2009 at 07:55:45PM +0200, Pavel Machek wrote:
> > On Fri 2009-04-10 23:06:55, Jaswinder Singh Rajput wrote:
> > > Impact: fix overflow
> >
> > > Constant 0x123456789abcdef0 is so big for a long.
> > > So using saved_magic 0x12345678 for both 32 and 64 bit.
> >
> > Have you tested the patch? I have feeling that this is tested from
> > assembly... Plus it is only set to long constant on 64-bit, so I don't
> > think you are fixing anything. And if your code is correct the
> > variable is write only...
>
> $ git grep saved_magic
> kernel/acpi/sleep.c: saved_magic = 0x12345678;
> kernel/acpi/sleep.c: saved_magic = 0x123456789abcdef0;
> kernel/acpi/sleep.h:extern long saved_magic;
> kernel/acpi/wakeup_32.S: movl %cs:saved_magic, %eax
> kernel/acpi/wakeup_32.S:ENTRY(saved_magic) .long 0
> kernel/acpi/wakeup_64.S: movq saved_magic, %rax
> kernel/acpi/wakeup_64.S:ENTRY(saved_magic) .quad 0
>
>
> wakeup_32.S:
> movl %cs:saved_magic, %eax
> cmpl $0x12345678, %eax
>
> wakeup_64.S:
> movq saved_magic, %rax
> movq $0x123456789abcdef0, %rdx
>
> So patch is obviously bogus.
>
Sorry, I was confused by this sparse warning on 64 bit box:
CHECK arch/x86/kernel/acpi/sleep.c
arch/x86/kernel/acpi/sleep.c:107:16: warning: constant 0x123456789abcdef0 is so big it is long
atleast I found problem in sparse ;-)
--
JSR
On Sat, Apr 11, 2009 at 12:46:56AM +0530, Jaswinder Singh Rajput wrote:
> On Fri, 2009-04-10 at 21:13 +0200, Sam Ravnborg wrote:
> > On Fri, Apr 10, 2009 at 07:55:45PM +0200, Pavel Machek wrote:
> > > On Fri 2009-04-10 23:06:55, Jaswinder Singh Rajput wrote:
> > > > Impact: fix overflow
> > >
> > > > Constant 0x123456789abcdef0 is so big for a long.
> > > > So using saved_magic 0x12345678 for both 32 and 64 bit.
> > >
> > > Have you tested the patch? I have feeling that this is tested from
> > > assembly... Plus it is only set to long constant on 64-bit, so I don't
> > > think you are fixing anything. And if your code is correct the
> > > variable is write only...
> >
> > $ git grep saved_magic
> > kernel/acpi/sleep.c: saved_magic = 0x12345678;
> > kernel/acpi/sleep.c: saved_magic = 0x123456789abcdef0;
> > kernel/acpi/sleep.h:extern long saved_magic;
> > kernel/acpi/wakeup_32.S: movl %cs:saved_magic, %eax
> > kernel/acpi/wakeup_32.S:ENTRY(saved_magic) .long 0
> > kernel/acpi/wakeup_64.S: movq saved_magic, %rax
> > kernel/acpi/wakeup_64.S:ENTRY(saved_magic) .quad 0
> >
> >
> > wakeup_32.S:
> > movl %cs:saved_magic, %eax
> > cmpl $0x12345678, %eax
> >
> > wakeup_64.S:
> > movq saved_magic, %rax
> > movq $0x123456789abcdef0, %rdx
> >
> > So patch is obviously bogus.
> >
>
> Sorry, I was confused by this sparse warning on 64 bit box:
>
> CHECK arch/x86/kernel/acpi/sleep.c
> arch/x86/kernel/acpi/sleep.c:107:16: warning: constant 0x123456789abcdef0 is so big it is long
>
> atleast I found problem in sparse ;-)
Sparse tells you that the constant is so big it is long - correct.
So rather than being implicit appending an L would make sparse happy.
So please use the constant 0x123456789abcdef0L and I think sparse is silenced.
And code/functionality is the same.
Sam
On Fri, 2009-04-10 at 21:29 +0200, Sam Ravnborg wrote:
> On Sat, Apr 11, 2009 at 12:46:56AM +0530, Jaswinder Singh Rajput wrote:
> > Sorry, I was confused by this sparse warning on 64 bit box:
> >
> > CHECK arch/x86/kernel/acpi/sleep.c
> > arch/x86/kernel/acpi/sleep.c:107:16: warning: constant 0x123456789abcdef0 is so big it is long
> >
> > atleast I found problem in sparse ;-)
>
> Sparse tells you that the constant is so big it is long - correct.
> So rather than being implicit appending an L would make sparse happy.
>
> So please use the constant 0x123456789abcdef0L and I think sparse is silenced.
> And code/functionality is the same.
>
Yes, this fixes the sparse warning, Thanks :-)
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 7c243a2..3445d0f 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -104,7 +104,7 @@ int acpi_save_state_mem(void)
initial_gs = per_cpu_offset(smp_processor_id());
#endif
initial_code = (unsigned long)wakeup_long64;
- saved_magic = 0x123456789abcdef0;
+ saved_magic = 0x123456789abcdef0L;
#endif /* CONFIG_64BIT */
return 0;
--
On Sat, 11 Apr 2009, Jaswinder Singh Rajput wrote:
> Sorry, I was confused by this sparse warning on 64 bit box:
>
> CHECK arch/x86/kernel/acpi/sleep.c
> arch/x86/kernel/acpi/sleep.c:107:16: warning: constant 0x123456789abcdef0 is so big it is long
>
> atleast I found problem in sparse ;-)
Sigh. There is no problem in sparse. The problem is between keyboard
and chair.
Care to read what the warning says ?
warning: constant 0x123456789abcdef0 is so big it is long
It needs to be: 0x123456789abcdef0L
Seriously, this kind of mechanical "fix" warnings no matter what and
without actually understanding the issues is more than counter
productive.
The point is, you force people into wasting a lot of time into what
should have been handled in 10 seconds. This is not efficient at all
and you need to improve your ways of communicaton and be more careful
about the patches you submit.
Thanks,
tglx
On Sat 2009-04-11 01:19:41, Jaswinder Singh Rajput wrote:
> On Fri, 2009-04-10 at 21:29 +0200, Sam Ravnborg wrote:
> > On Sat, Apr 11, 2009 at 12:46:56AM +0530, Jaswinder Singh Rajput wrote:
> > > Sorry, I was confused by this sparse warning on 64 bit box:
> > >
> > > CHECK arch/x86/kernel/acpi/sleep.c
> > > arch/x86/kernel/acpi/sleep.c:107:16: warning: constant 0x123456789abcdef0 is so big it is long
> > >
> > > atleast I found problem in sparse ;-)
> >
> > Sparse tells you that the constant is so big it is long - correct.
> > So rather than being implicit appending an L would make sparse happy.
> >
> > So please use the constant 0x123456789abcdef0L and I think sparse is silenced.
> > And code/functionality is the same.
> >
>
> Yes, this fixes the sparse warning, Thanks :-)
Acked-by: Pavel Machek <[email protected]>
> diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
> index 7c243a2..3445d0f 100644
> --- a/arch/x86/kernel/acpi/sleep.c
> +++ b/arch/x86/kernel/acpi/sleep.c
> @@ -104,7 +104,7 @@ int acpi_save_state_mem(void)
> initial_gs = per_cpu_offset(smp_processor_id());
> #endif
> initial_code = (unsigned long)wakeup_long64;
> - saved_magic = 0x123456789abcdef0;
> + saved_magic = 0x123456789abcdef0L;
> #endif /* CONFIG_64BIT */
>
> return 0;
>
> --
>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Friday 10 April 2009, Pavel Machek wrote:
> On Sat 2009-04-11 01:19:41, Jaswinder Singh Rajput wrote:
> > On Fri, 2009-04-10 at 21:29 +0200, Sam Ravnborg wrote:
> > > On Sat, Apr 11, 2009 at 12:46:56AM +0530, Jaswinder Singh Rajput wrote:
> > > > Sorry, I was confused by this sparse warning on 64 bit box:
> > > >
> > > > CHECK arch/x86/kernel/acpi/sleep.c
> > > > arch/x86/kernel/acpi/sleep.c:107:16: warning: constant 0x123456789abcdef0 is so big it is long
> > > >
> > > > atleast I found problem in sparse ;-)
> > >
> > > Sparse tells you that the constant is so big it is long - correct.
> > > So rather than being implicit appending an L would make sparse happy.
> > >
> > > So please use the constant 0x123456789abcdef0L and I think sparse is silenced.
> > > And code/functionality is the same.
> > >
> >
> > Yes, this fixes the sparse warning, Thanks :-)
>
> Acked-by: Pavel Machek <[email protected]>
OK
Ingo, are you going to take it or should I handle it?
Rafael
> > diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
> > index 7c243a2..3445d0f 100644
> > --- a/arch/x86/kernel/acpi/sleep.c
> > +++ b/arch/x86/kernel/acpi/sleep.c
> > @@ -104,7 +104,7 @@ int acpi_save_state_mem(void)
> > initial_gs = per_cpu_offset(smp_processor_id());
> > #endif
> > initial_code = (unsigned long)wakeup_long64;
> > - saved_magic = 0x123456789abcdef0;
> > + saved_magic = 0x123456789abcdef0L;
> > #endif /* CONFIG_64BIT */
> >
> > return 0;
> >
> > --
* Rafael J. Wysocki <[email protected]> wrote:
> On Friday 10 April 2009, Pavel Machek wrote:
> > On Sat 2009-04-11 01:19:41, Jaswinder Singh Rajput wrote:
> > > On Fri, 2009-04-10 at 21:29 +0200, Sam Ravnborg wrote:
> > > > On Sat, Apr 11, 2009 at 12:46:56AM +0530, Jaswinder Singh Rajput wrote:
> > > > > Sorry, I was confused by this sparse warning on 64 bit box:
> > > > >
> > > > > CHECK arch/x86/kernel/acpi/sleep.c
> > > > > arch/x86/kernel/acpi/sleep.c:107:16: warning: constant 0x123456789abcdef0 is so big it is long
> > > > >
> > > > > atleast I found problem in sparse ;-)
> > > >
> > > > Sparse tells you that the constant is so big it is long - correct.
> > > > So rather than being implicit appending an L would make sparse happy.
> > > >
> > > > So please use the constant 0x123456789abcdef0L and I think sparse is silenced.
> > > > And code/functionality is the same.
> > > >
> > >
> > > Yes, this fixes the sparse warning, Thanks :-)
> >
> > Acked-by: Pavel Machek <[email protected]>
>
> OK
>
> Ingo, are you going to take it or should I handle it?
Sure, feel free to take it - thanks.
Acked-by: Ingo Molnar <[email protected]>
Ingo