printk %0*X is broken.

printk %0*X is broken.

[Yinghai Lu]

it seems someone broke 

printk(   "%0*X\n", width, x);

looks like 0 is dumped.

YH

[    0.000000] MTRR variable ranges enabled:
[    0.000000]   0 base 0   00000000 mask FF0 00000000 write-back
[    0.000000]   1 base 10  00000000 mask FFF 80000000 write-back
[    0.000000]   2 base 0   80000000 mask FFF 80000000 uncachable
[    0.000000]   3 base 0   7F800000 mask FFF FF800000 uncachable


code:
        for (i = 0; i < num_var_ranges; ++i) {
                if (mtrr_state.var_ranges[i].mask_lo & (1 << 11))
                        printk(KERN_DEBUG "  %u base %0*X%05X000 mask %0*X%05X000 %s\n",
                               i,
                               high_width,
                               mtrr_state.var_ranges[i].base_hi,
                               mtrr_state.var_ranges[i].base_lo >> 12,
                               high_width,
                               mtrr_state.var_ranges[i].mask_hi,
                               mtrr_state.var_ranges[i].mask_lo >> 12,
                               mtrr_attrib_to_str(mtrr_state.var_ranges[i].base_lo & 0xff));
                else
                        printk(KERN_DEBUG "  %u disabled\n", i);
        }

Re: printk %0*X is broken.

[Ingo Molnar]

Cc:-ed more folks who modified lib/vsprintf.c recently.

	Ingo

* Yinghai Lu <yinghai@kernel.org> wrote:

> it seems someone broke 
> 
> printk(   "%0*X\n", width, x);
> 
> looks like 0 is dumped.
> 
> YH
> 
> [    0.000000] MTRR variable ranges enabled:
> [    0.000000]   0 base 0   00000000 mask FF0 00000000 write-back
> [    0.000000]   1 base 10  00000000 mask FFF 80000000 write-back
> [    0.000000]   2 base 0   80000000 mask FFF 80000000 uncachable
> [    0.000000]   3 base 0   7F800000 mask FFF FF800000 uncachable
> 
> 
> code:
>         for (i = 0; i < num_var_ranges; ++i) {
>                 if (mtrr_state.var_ranges[i].mask_lo & (1 << 11))
>                         printk(KERN_DEBUG "  %u base %0*X%05X000 mask %0*X%05X000 %s\n",
>                                i,
>                                high_width,
>                                mtrr_state.var_ranges[i].base_hi,
>                                mtrr_state.var_ranges[i].base_lo >> 12,
>                                high_width,
>                                mtrr_state.var_ranges[i].mask_hi,
>                                mtrr_state.var_ranges[i].mask_lo >> 12,
>                                mtrr_attrib_to_str(mtrr_state.var_ranges[i].base_lo & 0xff));
>                 else
>                         printk(KERN_DEBUG "  %u disabled\n", i);
>         }
> 

Re: printk %0*X is broken.

[Li Zefan]

Ingo Molnar wrote:
> Cc:-ed more folks who modified lib/vsprintf.c recently.
> 

I'm CC-ed, but I never ever touched lib/vsprintf.c. ;)

Lai Jiangshan will look into this issue.

> 	Ingo
> 
> * Yinghai Lu <yinghai@kernel.org> wrote:
> 
>> it seems someone broke 
>>
>> printk(   "%0*X\n", width, x);
>>
>> looks like 0 is dumped.
>>
>> YH
>>
>> [    0.000000] MTRR variable ranges enabled:
>> [    0.000000]   0 base 0   00000000 mask FF0 00000000 write-back
>> [    0.000000]   1 base 10  00000000 mask FFF 80000000 write-back
>> [    0.000000]   2 base 0   80000000 mask FFF 80000000 uncachable
>> [    0.000000]   3 base 0   7F800000 mask FFF FF800000 uncachable
>>
>>
>> code:
>>         for (i = 0; i < num_var_ranges; ++i) {
>>                 if (mtrr_state.var_ranges[i].mask_lo & (1 << 11))
>>                         printk(KERN_DEBUG "  %u base %0*X%05X000 mask %0*X%05X000 %s\n",
>>                                i,
>>                                high_width,
>>                                mtrr_state.var_ranges[i].base_hi,
>>                                mtrr_state.var_ranges[i].base_lo >> 12,
>>                                high_width,
>>                                mtrr_state.var_ranges[i].mask_hi,
>>                                mtrr_state.var_ranges[i].mask_lo >> 12,
>>                                mtrr_attrib_to_str(mtrr_state.var_ranges[i].base_lo & 0xff));
>>                 else
>>                         printk(KERN_DEBUG "  %u disabled\n", i);
>>         }
>>
> 
> 

Re: printk %0*X is broken.

[Vegard Nossum]

2009/5/6 Ingo Molnar <mingo@elte.hu>:
>
> Cc:-ed more folks who modified lib/vsprintf.c recently.
>
>        Ingo
>
> * Yinghai Lu <yinghai@kernel.org> wrote:
>
>> it seems someone broke
>>
>> printk(   "%0*X\n", width, x);
>>
>> looks like 0 is dumped.

After %, we look for flags. The problem is that when a flag is found, we
don't advance in the format string. And thus we start looking for the
precision, which is read as 0, because we are still at the 0. I think
this patch should fix it.

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 7536ace..ae7d4b2 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -800,14 +800,15 @@ static int format_decode(const char *fmt, struct printf_spec *spec)
 	if (fmt != start || !*fmt)
 		return fmt - start;
 
+	/* this skips the first '%' */
+	++fmt;
+
 	/* Process flags */
 	spec->flags = 0;
 
-	while (1) { /* this also skips first '%' */
+	while (1) {
 		bool found = true;
 
-		++fmt;
-
 		switch (*fmt) {
 		case '-': spec->flags |= LEFT;    break;
 		case '+': spec->flags |= PLUS;    break;
@@ -819,6 +820,8 @@ static int format_decode(const char *fmt, struct printf_spec *spec)
 
 		if (!found)
 			break;
+
+		++fmt;
 	}
 
 	/* get field width */

Re: printk %0*X is broken.

[Lai Jiangshan]

Vegard Nossum wrote:
> 2009/5/6 Ingo Molnar <mingo@elte.hu>:
>> Cc:-ed more folks who modified lib/vsprintf.c recently.
>>
>>        Ingo
>>
>> * Yinghai Lu <yinghai@kernel.org> wrote:
>>
>>> it seems someone broke
>>>
>>> printk(   "%0*X\n", width, x);
>>>
>>> looks like 0 is dumped.
> 
> After %, we look for flags. The problem is that when a flag is found, we
> don't advance in the format string. And thus we start looking for the
> precision, which is read as 0, because we are still at the 0. I think
> this patch should fix it.
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 7536ace..ae7d4b2 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -800,14 +800,15 @@ static int format_decode(const char *fmt, struct printf_spec *spec)
>  	if (fmt != start || !*fmt)
>  		return fmt - start;
>  
> +	/* this skips the first '%' */
> +	++fmt;
> +
>  	/* Process flags */
>  	spec->flags = 0;
>  
> -	while (1) { /* this also skips first '%' */
> +	while (1) {
>  		bool found = true;
>  
> -		++fmt;
> -
>  		switch (*fmt) {
>  		case '-': spec->flags |= LEFT;    break;
>  		case '+': spec->flags |= PLUS;    break;
> @@ -819,6 +820,8 @@ static int format_decode(const char *fmt, struct printf_spec *spec)
>  
>  		if (!found)
>  			break;
> +
> +		++fmt;
>  	}
>

It seems that your patch does not change anything.
The code logic is still the same as before.

Lai

Re: printk %0*X is broken.

[Li Zefan]

Ingo Molnar wrote:
> Cc:-ed more folks who modified lib/vsprintf.c recently.
> 
> 	Ingo
> 
> * Yinghai Lu <yinghai@kernel.org> wrote:
> 
>> it seems someone broke 
>>
>> printk(   "%0*X\n", width, x);
>>
>> looks like 0 is dumped.
>>
>> YH
>>
>> [    0.000000] MTRR variable ranges enabled:
>> [    0.000000]   0 base 0   00000000 mask FF0 00000000 write-back
>> [    0.000000]   1 base 10  00000000 mask FFF 80000000 write-back
>> [    0.000000]   2 base 0   80000000 mask FFF 80000000 uncachable
>> [    0.000000]   3 base 0   7F800000 mask FFF FF800000 uncachable
>>

2.6.30-rc4-tip, the output of my box:

high_width: 1
MTRR variable ranges enabled:
  0 base 000000000 mask FC0000000 write-back
  1 base 03C000000 mask FFC000000 uncachable
  2 base 0D0000000 mask FF8000000 write-combining

Is it possible that high_width is negative in your output?
If high_width == -3, we can get exactly the same output with yours.

>>
>> code:
>>         for (i = 0; i < num_var_ranges; ++i) {
>>                 if (mtrr_state.var_ranges[i].mask_lo & (1 << 11))
>>                         printk(KERN_DEBUG "  %u base %0*X%05X000 mask %0*X%05X000 %s\n",
>>                                i,
>>                                high_width,
>>                                mtrr_state.var_ranges[i].base_hi,
>>                                mtrr_state.var_ranges[i].base_lo >> 12,
>>                                high_width,
>>                                mtrr_state.var_ranges[i].mask_hi,
>>                                mtrr_state.var_ranges[i].mask_lo >> 12,
>>                                mtrr_attrib_to_str(mtrr_state.var_ranges[i].base_lo & 0xff));
>>                 else
>>                         printk(KERN_DEBUG "  %u disabled\n", i);
>>         }
>>
> 
> 

Re: printk %0*X is broken.

[Li Zefan]

Lai Jiangshan wrote:
> Vegard Nossum wrote:
>> 2009/5/6 Ingo Molnar <mingo@elte.hu>:
>>> Cc:-ed more folks who modified lib/vsprintf.c recently.
>>>
>>>        Ingo
>>>
>>> * Yinghai Lu <yinghai@kernel.org> wrote:
>>>
>>>> it seems someone broke
>>>>
>>>> printk(   "%0*X\n", width, x);
>>>>
>>>> looks like 0 is dumped.
>> After %, we look for flags. The problem is that when a flag is found, we
>> don't advance in the format string. And thus we start looking for the
>> precision, which is read as 0, because we are still at the 0. I think
>> this patch should fix it.
>>

No, we break out of the while loop when we can't find a flag,
So we are at '*' after we found '0'.

>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index 7536ace..ae7d4b2 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -800,14 +800,15 @@ static int format_decode(const char *fmt, struct printf_spec *spec)
>>  	if (fmt != start || !*fmt)
>>  		return fmt - start;
>>  
>> +	/* this skips the first '%' */
>> +	++fmt;
>> +
>>  	/* Process flags */
>>  	spec->flags = 0;
>>  
>> -	while (1) { /* this also skips first '%' */
>> +	while (1) {
>>  		bool found = true;
>>  
>> -		++fmt;
>> -
>>  		switch (*fmt) {
>>  		case '-': spec->flags |= LEFT;    break;
>>  		case '+': spec->flags |= PLUS;    break;
>> @@ -819,6 +820,8 @@ static int format_decode(const char *fmt, struct printf_spec *spec)
>>  
>>  		if (!found)
>>  			break;
>> +
>> +		++fmt;
>>  	}
>>
> 
> It seems that your patch does not change anything.
> The code logic is still the same as before.
> 
> Lai
> 
> 
> 

Re: printk %0*X is broken.

[Vegard Nossum]

2009/5/6 Li Zefan <lizf@cn.fujitsu.com>:
>> Vegard Nossum wrote:
>>> 2009/5/6 Ingo Molnar <mingo@elte.hu>:
>>>> Cc:-ed more folks who modified lib/vsprintf.c recently.
>>>>
>>>>        Ingo
>>>>
>>>> * Yinghai Lu <yinghai@kernel.org> wrote:
>>>>
>>>>> it seems someone broke
>>>>>
>>>>> printk(   "%0*X\n", width, x);
>>>>>
>>>>> looks like 0 is dumped.
>>> After %, we look for flags. The problem is that when a flag is found, we
>>> don't advance in the format string. And thus we start looking for the
>>> precision, which is read as 0, because we are still at the 0. I think
>>> this patch should fix it.
>>>
>
> No, we break out of the while loop when we can't find a flag,
> So we are at '*' after we found '0'.

> Lai Jiangshan wrote:
>> It seems that your patch does not change anything.
>> The code logic is still the same as before.

Oh really? So sorry, I didn't look close enough :-(


Vegard

Re: printk %0*X is broken.

[Frédéric Weisbecker]

2009/5/6 Ingo Molnar <mingo@elte.hu>:
>
> Cc:-ed more folks who modified lib/vsprintf.c recently.
>
>        Ingo


Thanks, I will look at it this evening.

Re: printk %0*X is broken.

[Frederic Weisbecker]

On Wed, May 06, 2009 at 04:30:21PM +0800, Li Zefan wrote:
> Ingo Molnar wrote:
> > Cc:-ed more folks who modified lib/vsprintf.c recently.
> > 
> > 	Ingo
> > 
> > * Yinghai Lu <yinghai@kernel.org> wrote:
> > 
> >> it seems someone broke 
> >>
> >> printk(   "%0*X\n", width, x);
> >>
> >> looks like 0 is dumped.
> >>
> >> YH
> >>
> >> [    0.000000] MTRR variable ranges enabled:
> >> [    0.000000]   0 base 0   00000000 mask FF0 00000000 write-back
> >> [    0.000000]   1 base 10  00000000 mask FFF 80000000 write-back
> >> [    0.000000]   2 base 0   80000000 mask FFF 80000000 uncachable
> >> [    0.000000]   3 base 0   7F800000 mask FFF FF800000 uncachable
> >>
> 
> 2.6.30-rc4-tip, the output of my box:
> 
> high_width: 1
> MTRR variable ranges enabled:
>   0 base 000000000 mask FC0000000 write-back
>   1 base 03C000000 mask FFC000000 uncachable
>   2 base 0D0000000 mask FF8000000 write-combining
> 
> Is it possible that high_width is negative in your output?
> If high_width == -3, we can get exactly the same output with yours.


Indeed, a negative value given as the width will actually pad to the
right (reverse width forcing).
If you have -3, that matches the normal printf behaviour.

Yinghai, could you check please if that's the case for you? 

Thanks,

Frederic.

[PATCH] x86: fix compute high_width with phys-addr is 44bit and more

[Yinghai Lu]

found one system that cpu address line is 44bits, mtrr printout is not right.

[    0.000000] MTRR variable ranges enabled:
[    0.000000]   0 base 0   00000000 mask FF0 00000000 write-back
[    0.000000]   1 base 10  00000000 mask FFF 80000000 write-back
[    0.000000]   2 base 0   80000000 mask FFF 80000000 uncachable
[    0.000000]   3 base 0   7F800000 mask FFF FF800000 uncachable

Li Zefan and Frederic pointed out the high_width could be -4 some how.

it turns out when phys_addr is 44bit, size_or_mask will be ffffffff,00000000
so ffs(size_or_mask) will be 0.

try to check low 32 bit, to get correct high_width.

Signed-off-by: Yinghai Lu <yinghai@kerne.org>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>

---
 arch/x86/kernel/cpu/mtrr/generic.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: linux-2.6/arch/x86/kernel/cpu/mtrr/generic.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/cpu/mtrr/generic.c
+++ linux-2.6/arch/x86/kernel/cpu/mtrr/generic.c
@@ -275,7 +275,11 @@ static void __init print_mtrr_state(void
 	}
 	printk(KERN_DEBUG "MTRR variable ranges %sabled:\n",
 	       mtrr_state.enabled & 2 ? "en" : "dis");
-	high_width = ((size_or_mask ? ffs(size_or_mask) - 1 : 32) - (32 - PAGE_SHIFT) + 3) / 4;
+	if (size_or_mask & 0xffffffffUL)
+		high_width = ffs(size_or_mask & 0xffffffffUL) - 1;
+	else
+		high_width = ffs(size_or_mask>>32) + 32 - 1;
+	high_width = (high_width - (32 - PAGE_SHIFT) + 3) / 4;
 	for (i = 0; i < num_var_ranges; ++i) {
 		if (mtrr_state.var_ranges[i].mask_lo & (1 << 11))
 			printk(KERN_DEBUG "  %u base %0*X%05X000 mask %0*X%05X000 %s\n",

[tip:x86/urgent] x86: mtrr: Fix high_width computation when phys-addr is >= 44bit

[tip-bot for Yinghai Lu]

Commit-ID:  917a0153621572e88aeb2d5df025ad2e81027287
Gitweb:     http://git.kernel.org/tip/917a0153621572e88aeb2d5df025ad2e81027287
Author:     Yinghai Lu <yinghai@kernel.org>
AuthorDate: Wed, 6 May 2009 21:36:16 -0700
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 11 May 2009 11:40:43 +0200

x86: mtrr: Fix high_width computation when phys-addr is >= 44bit

found one system where cpu address line is 44bits, mtrr printout
is not right:

 [    0.000000] MTRR variable ranges enabled:
 [    0.000000]   0 base 0   00000000 mask FF0 00000000 write-back
 [    0.000000]   1 base 10  00000000 mask FFF 80000000 write-back
 [    0.000000]   2 base 0   80000000 mask FFF 80000000 uncachable
 [    0.000000]   3 base 0   7F800000 mask FFF FF800000 uncachable

Li Zefan and Frederic pointed out the high_width could be -4 some how.

It turns out when phys_addr is 44bit, size_or_mask will be
ffffffff,00000000 so ffs(size_or_mask) will be 0.

Try to check low 32 bit, to get correct high_width.

Signed-off-by: Yinghai Lu <yinghai@kerne.org>
Also-analyzed-by: Frederic Weisbecker <fweisbec@gmail.com>
Also-analyzed-by: Li Zefan <lizf@cn.fujitsu.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Zhaolei <zhaolei@cn.fujitsu.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Vegard Nossum <vegard.nossum@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
LKML-Reference: <4A026540.8060504@kernel.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/kernel/cpu/mtrr/generic.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 0b776c0..d21d4fb 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -275,7 +275,11 @@ static void __init print_mtrr_state(void)
 	}
 	printk(KERN_DEBUG "MTRR variable ranges %sabled:\n",
 	       mtrr_state.enabled & 2 ? "en" : "dis");
-	high_width = ((size_or_mask ? ffs(size_or_mask) - 1 : 32) - (32 - PAGE_SHIFT) + 3) / 4;
+	if (size_or_mask & 0xffffffffUL)
+		high_width = ffs(size_or_mask & 0xffffffffUL) - 1;
+	else
+		high_width = ffs(size_or_mask>>32) + 32 - 1;
+	high_width = (high_width - (32 - PAGE_SHIFT) + 3) / 4;
 	for (i = 0; i < num_var_ranges; ++i) {
 		if (mtrr_state.var_ranges[i].mask_lo & (1 << 11))
 			printk(KERN_DEBUG "  %u base %0*X%05X000 mask %0*X%05X000 %s\n",