[PATCH] iqrdomain: Improve formatting in debugfs.

[PATCH] iqrdomain: Improve formatting in debugfs.

[David Daney]

From: David Daney <david.daney@cavium.com>

The irq_domain_mapping file has some interesting output when chip_data
contains leading zeros:

virq   hwirq    chip name        chip data           domain name
.
.
.
  103  0x00000  CIU2-W           0x 5e0000000000000  none
  104  0x00000  CIU2-W           0x 5f0000000000000  none
  105  0x00000  CIU2-M           0x          (null)  none
  113  0x00000  CIU2-E           0x1080000000000000  none
.
.
.

I think there should be no space in there between the "0x" and the
rest.  Also the '(null)' entry doesn't make much sense at all with a
"0x".

Instead I suggest:
.
.
.
  103  0x00000  CIU2-W           0x05e0000000000000  none
  104  0x00000  CIU2-W           0x05f0000000000000  none
  105  0x00000  CIU2-M           0x0000000000000000  none
  113  0x00000  CIU2-E           0x1080000000000000  none
.
.
.

This patch also prints the "chip data" as an 8 character wide number
for 32-bit kernels, instead of the 16 characters wide in the 64-bit
case.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 kernel/irq/irqdomain.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 481f2da..83474bf 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -642,9 +642,10 @@ static int virq_debug_show(struct seq_file *m, void *private)
 	static const char none[] = "none";
 	void *data;
 	int i;
+	int ptr_width = sizeof(void *) * 2;
 
-	seq_printf(m, "%-5s  %-7s  %-15s  %-18s  %s\n", "virq", "hwirq",
-		      "chip name", "chip data", "domain name");
+	seq_printf(m, "%-5s  %-7s  %-15s  %-*s  %s\n", "virq", "hwirq",
+		   "chip name", ptr_width + 2, "chip data", "domain name");
 
 	for (i = 1; i < nr_irqs; i++) {
 		desc = irq_to_desc(i);
@@ -667,7 +668,8 @@ static int virq_debug_show(struct seq_file *m, void *private)
 			seq_printf(m, "%-15s  ", p);
 
 			data = irq_desc_get_chip_data(desc);
-			seq_printf(m, "0x%16p  ", data);
+			seq_printf(m, "0x%0*lx  ",
+				   ptr_width, (unsigned long)data);
 
 			if (desc->irq_data.domain && desc->irq_data.domain->of_node)
 				p = desc->irq_data.domain->of_node->full_name;
-- 
1.7.2.3

Re: [PATCH] iqrdomain: Improve formatting in debugfs.

[Grant Likely]

On Mon,  9 Apr 2012 17:06:57 -0700, David Daney <ddaney.cavm@gmail.com> wrote:
> From: David Daney <david.daney@cavium.com>
> 
> The irq_domain_mapping file has some interesting output when chip_data
> contains leading zeros:
> 
> virq   hwirq    chip name        chip data           domain name
> .
> .
> .
>   103  0x00000  CIU2-W           0x 5e0000000000000  none
>   104  0x00000  CIU2-W           0x 5f0000000000000  none
>   105  0x00000  CIU2-M           0x          (null)  none
>   113  0x00000  CIU2-E           0x1080000000000000  none
> .
> .
> .
> 
> I think there should be no space in there between the "0x" and the
> rest.  Also the '(null)' entry doesn't make much sense at all with a
> "0x".

Yeah, it doesn't make much sense with the '0x', but I don't want to
supress the (null) output because it is really important to highlight
when a domain isn't assigned to an active irq.

Fixing the zero pad is trivial to fix though.  Just remove the
precision from the %p format.  Here's my counter-patch:

---

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 9310a8d..eb05e40 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -643,8 +643,8 @@ static int virq_debug_show(struct seq_file *m, void *private)
 	void *data;
 	int i;
 
-	seq_printf(m, "%-5s  %-7s  %-15s  %-18s  %s\n", "virq", "hwirq",
-		      "chip name", "chip data", "domain name");
+	seq_printf(m, "%-5s  %-7s  %-15s  %-*s  %s\n", "irq", "hwirq",
+		      "chip name", 2 * sizeof(void *) + 2, "chip data", "domain name");
 
 	for (i = 1; i < nr_irqs; i++) {
 		desc = irq_to_desc(i);
@@ -667,7 +667,7 @@ static int virq_debug_show(struct seq_file *m, void *private)
 			seq_printf(m, "%-15s  ", p);
 
 			data = irq_desc_get_chip_data(desc);
-			seq_printf(m, "0x%16p  ", data);
+			seq_printf(m, data ? "0x%p  " : "  %p  ", data);
 
 			if (desc->irq_data.domain && desc->irq_data.domain->of_node)
 				p = desc->irq_data.domain->of_node->full_name;

Re: [PATCH] iqrdomain: Improve formatting in debugfs.

[Andreas Schwab]

Grant Likely <grant.likely@secretlab.ca> writes:

> @@ -667,7 +667,7 @@ static int virq_debug_show(struct seq_file *m, void *private)
>  			seq_printf(m, "%-15s  ", p);
>  
>  			data = irq_desc_get_chip_data(desc);
> -			seq_printf(m, "0x%16p  ", data);
> +			seq_printf(m, data ? "0x%p  " : "  %p  ", data);

You should be able to use %#p instead to let it add the prefix.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] iqrdomain: Improve formatting in debugfs.

[Grant Likely]

On Wed, Apr 11, 2012 at 3:16 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> Grant Likely <grant.likely@secretlab.ca> writes:
>
>> @@ -667,7 +667,7 @@ static int virq_debug_show(struct seq_file *m, void *private)
>>                       seq_printf(m, "%-15s  ", p);
>>
>>                       data = irq_desc_get_chip_data(desc);
>> -                     seq_printf(m, "0x%16p  ", data);
>> +                     seq_printf(m, data ? "0x%p  " : "  %p  ", data);
>
> You should be able to use %#p instead to let it add the prefix.

That mostly works, but it doesn't round up the field size to account
for the 2 characters in '0x'.  Looks like the %p code needs to be
tweaked to fix that.

Also, gcc complains about it:

/home/grant/hacking/linux/kernel/irq/irqdomain.c: In function ‘virq_debug_show’:
/home/grant/hacking/linux/kernel/irq/irqdomain.c:670:4: warning: '#'
flag used with ‘%p’ gnu_printf format [-Wformat]

Do you know how to suppress that?

Re: [PATCH] iqrdomain: Improve formatting in debugfs.

[Grant Likely]

On Wed, Apr 11, 2012 at 8:52 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Apr 11, 2012 at 3:16 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>> Grant Likely <grant.likely@secretlab.ca> writes:
>>
>>> @@ -667,7 +667,7 @@ static int virq_debug_show(struct seq_file *m, void *private)
>>>                       seq_printf(m, "%-15s  ", p);
>>>
>>>                       data = irq_desc_get_chip_data(desc);
>>> -                     seq_printf(m, "0x%16p  ", data);
>>> +                     seq_printf(m, data ? "0x%p  " : "  %p  ", data);
>>
>> You should be able to use %#p instead to let it add the prefix.
>
> That mostly works, but it doesn't round up the field size to account
> for the 2 characters in '0x'.  Looks like the %p code needs to be
> tweaked to fix that.

which this change does:

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index abbabec..4c4f2ce 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -946,6 +946,8 @@ char *pointer(const char *fmt, char *buf, char
*end, void *ptr,
        if (spec.field_width == -1) {
                spec.field_width = 2 * sizeof(void *);
                spec.flags |= ZEROPAD;
+               if (spec.flags & SPECIAL)
+                       spec.field_width += 2;
        }
        spec.base = 16;

Re: [PATCH] iqrdomain: Improve formatting in debugfs.

[David Daney]

On 04/11/2012 12:03 AM, Grant Likely wrote:
> On Mon,  9 Apr 2012 17:06:57 -0700, David Daney<ddaney.cavm@gmail.com>  wrote:
>> From: David Daney<david.daney@cavium.com>
>>
>> The irq_domain_mapping file has some interesting output when chip_data
>> contains leading zeros:
>>
>> virq   hwirq    chip name        chip data           domain name
>> .
>> .
>> .
>>    103  0x00000  CIU2-W           0x 5e0000000000000  none
>>    104  0x00000  CIU2-W           0x 5f0000000000000  none
>>    105  0x00000  CIU2-M           0x          (null)  none
>>    113  0x00000  CIU2-E           0x1080000000000000  none
>> .
>> .
>> .
>>
>> I think there should be no space in there between the "0x" and the
>> rest.  Also the '(null)' entry doesn't make much sense at all with a
>> "0x".
>
> Yeah, it doesn't make much sense with the '0x', but I don't want to
> supress the (null) output because it is really important to highlight
> when a domain isn't assigned to an active irq.
>
> Fixing the zero pad is trivial to fix though.  Just remove the
> precision from the %p format.  Here's my counter-patch:

This is good too.

FWIW:
Acked-by: David Daney <david.daney@cavium.com>


>
> ---
>
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 9310a8d..eb05e40 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -643,8 +643,8 @@ static int virq_debug_show(struct seq_file *m, void *private)
>   	void *data;
>   	int i;
>
> -	seq_printf(m, "%-5s  %-7s  %-15s  %-18s  %s\n", "virq", "hwirq",
> -		      "chip name", "chip data", "domain name");
> +	seq_printf(m, "%-5s  %-7s  %-15s  %-*s  %s\n", "irq", "hwirq",
> +		      "chip name", 2 * sizeof(void *) + 2, "chip data", "domain name");
>
>   	for (i = 1; i<  nr_irqs; i++) {
>   		desc = irq_to_desc(i);
> @@ -667,7 +667,7 @@ static int virq_debug_show(struct seq_file *m, void *private)
>   			seq_printf(m, "%-15s  ", p);
>
>   			data = irq_desc_get_chip_data(desc);
> -			seq_printf(m, "0x%16p  ", data);
> +			seq_printf(m, data ? "0x%p  " : "  %p  ", data);
>
>   			if (desc->irq_data.domain&&  desc->irq_data.domain->of_node)
>   				p = desc->irq_data.domain->of_node->full_name;

Re: [PATCH] iqrdomain: Improve formatting in debugfs.

[Andreas Schwab]

Grant Likely <grant.likely@secretlab.ca> writes:

> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 9310a8d..eb05e40 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -643,8 +643,8 @@ static int virq_debug_show(struct seq_file *m, void *private)
>  	void *data;
>  	int i;
>  
> -	seq_printf(m, "%-5s  %-7s  %-15s  %-18s  %s\n", "virq", "hwirq",
> -		      "chip name", "chip data", "domain name");
> +	seq_printf(m, "%-5s  %-7s  %-15s  %-*s  %s\n", "irq", "hwirq",
> +		      "chip name", 2 * sizeof(void *) + 2, "chip data", "domain name");

kernel/irq/irqdomain.c:647:9: warning: field width specifier ‘*’ expects argument of type ‘int’, but argument 6 has type ‘long unsigned int’ [-Wformat]

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] iqrdomain: Improve formatting in debugfs.

[David Daney]

On 04/12/2012 12:20 PM, Andreas Schwab wrote:
> Grant Likely<grant.likely@secretlab.ca>  writes:
>
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index 9310a8d..eb05e40 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -643,8 +643,8 @@ static int virq_debug_show(struct seq_file *m, void *private)
>>   	void *data;
>>   	int i;
>>
>> -	seq_printf(m, "%-5s  %-7s  %-15s  %-18s  %s\n", "virq", "hwirq",
>> -		      "chip name", "chip data", "domain name");
>> +	seq_printf(m, "%-5s  %-7s  %-15s  %-*s  %s\n", "irq", "hwirq",
>> +		      "chip name", 2 * sizeof(void *) + 2, "chip data", "domain name");
>
> kernel/irq/irqdomain.c:647:9: warning: field width specifier ‘*’ expects argument of type ‘int’, but argument 6 has type ‘long unsigned int’ [-Wformat]
>

My original patch assigned the size to an int variable and then passed 
that to the seq_printf(), thus avoiding this issue.  I suspect something 
like:

s/2 * sizeof(void *) + 2/(int)(2 * sizeof(void *) + 2)/

Would do the trick.

David Daney

Re: [PATCH] iqrdomain: Improve formatting in debugfs.

[Grant Likely]

On Thu, Apr 12, 2012 at 2:38 PM, David Daney <ddaney.cavm@gmail.com> wrote:
> On 04/12/2012 12:20 PM, Andreas Schwab wrote:
>>
>> Grant Likely<grant.likely@secretlab.ca>  writes:
>>
>>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>>> index 9310a8d..eb05e40 100644
>>> --- a/kernel/irq/irqdomain.c
>>> +++ b/kernel/irq/irqdomain.c
>>> @@ -643,8 +643,8 @@ static int virq_debug_show(struct seq_file *m, void
>>> *private)
>>>        void *data;
>>>        int i;
>>>
>>> -       seq_printf(m, "%-5s  %-7s  %-15s  %-18s  %s\n", "virq", "hwirq",
>>> -                     "chip name", "chip data", "domain name");
>>> +       seq_printf(m, "%-5s  %-7s  %-15s  %-*s  %s\n", "irq", "hwirq",
>>> +                     "chip name", 2 * sizeof(void *) + 2, "chip data",
>>> "domain name");
>>
>>
>> kernel/irq/irqdomain.c:647:9: warning: field width specifier ‘*’ expects
>> argument of type ‘int’, but argument 6 has type ‘long unsigned int’
>> [-Wformat]
>>
>
> My original patch assigned the size to an int variable and then passed that
> to the seq_printf(), thus avoiding this issue.  I suspect something like:
>
> s/2 * sizeof(void *) + 2/(int)(2 * sizeof(void *) + 2)/
>
> Would do the trick.

Yeah, I'll push out a fixup patch.

g

Re: [PATCH] iqrdomain: Improve formatting in debugfs.

[Andreas Schwab]

Grant Likely <grant.likely@secretlab.ca> writes:

> Also, gcc complains about it:
>
> /home/grant/hacking/linux/kernel/irq/irqdomain.c: In function ‘virq_debug_show’:
> /home/grant/hacking/linux/kernel/irq/irqdomain.c:670:4: warning: '#'
> flag used with ‘%p’ gnu_printf format [-Wformat]

Then it's probably not such a good idea after all.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] iqrdomain: Improve formatting in debugfs.

[Grant Likely]

On Mon, Apr 16, 2012 at 4:01 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> Grant Likely <grant.likely@secretlab.ca> writes:
>
>> Also, gcc complains about it:
>>
>> /home/grant/hacking/linux/kernel/irq/irqdomain.c: In function ‘virq_debug_show’:
>> /home/grant/hacking/linux/kernel/irq/irqdomain.c:670:4: warning: '#'
>> flag used with ‘%p’ gnu_printf format [-Wformat]
>
> Then it's probably not such a good idea after all.

It would be a good idea if the warning can be suppressed.

g.