[PATCH] irqdomain: provide useful debugging information for irq domain

[PATCH] irqdomain: provide useful debugging information for irq domain

[Yang Shunyong]

With recent hashed kernel pointers change, output with %p will
output hashed address. This patch changes %p to %px in irq domain
debug information. As unprivileged user has no permission to mount
debugfs or set printk level to KERN_DEBUG to access the information,
changes in this patch are appropriate.

Signed-off-by: Yang Shunyong <shunyong.yang@hxt-semitech.com>
---
 kernel/irq/irqdomain.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 62068ad..d40fbed 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -75,7 +75,7 @@ struct fwnode_handle *__irq_domain_alloc_fwnode(unsigned int type, int id,
 		n = kasprintf(GFP_KERNEL, "%s-%d", name, id);
 		break;
 	default:
-		n = kasprintf(GFP_KERNEL, "irqchip@%p", data);
+		n = kasprintf(GFP_KERNEL, "irqchip@%px", data);
 		break;
 	}
 
@@ -450,7 +450,7 @@ EXPORT_SYMBOL_GPL(irq_domain_check_msi_remap);
  */
 void irq_set_default_host(struct irq_domain *domain)
 {
-	pr_debug("Default domain set to @0x%p\n", domain);
+	pr_debug("Default domain set to @0x%px\n", domain);
 
 	irq_default_domain = domain;
 }
@@ -635,7 +635,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
 	struct device_node *of_node;
 	int virq;
 
-	pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
+	pr_debug("%s(0x%px, 0x%lx)\n", __func__, domain, hwirq);
 
 	/* Look for default domain if nececssary */
 	if (domain == NULL)
@@ -644,7 +644,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
 		WARN(1, "%s(, %lx) called with NULL domain\n", __func__, hwirq);
 		return 0;
 	}
-	pr_debug("-> using domain @%p\n", domain);
+	pr_debug("-> using domain @%px\n", domain);
 
 	of_node = irq_domain_get_of_node(domain);
 
@@ -921,7 +921,7 @@ static void virq_debug_show_one(struct seq_file *m, struct irq_desc *desc)
 		chip = irq_data_get_irq_chip(data);
 		seq_printf(m, "%-15s  ", (chip && chip->name) ? chip->name : "none");
 
-		seq_printf(m, "0x%p  ", irq_data_get_irq_chip_data(data));
+		seq_printf(m, "0x%px  ", irq_data_get_irq_chip_data(data));
 
 		seq_printf(m, "   %c    ", (desc->action && desc->action->handler) ? '*' : ' ');
 		direct = (irq == hwirq) && (irq < domain->revmap_direct_max_irq);
-- 
2.7.4

Re: [PATCH] irqdomain: provide useful debugging information for irq domain

[Marc Zyngier]

Hi Yang,

On 17/01/18 04:37, Yang Shunyong wrote:
> With recent hashed kernel pointers change, output with %p will
> output hashed address. This patch changes %p to %px in irq domain
> debug information. As unprivileged user has no permission to mount
> debugfs or set printk level to KERN_DEBUG to access the information,
> changes in this patch are appropriate.
> 
> Signed-off-by: Yang Shunyong <shunyong.yang@hxt-semitech.com>
> ---
>  kernel/irq/irqdomain.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 62068ad..d40fbed 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -75,7 +75,7 @@ struct fwnode_handle *__irq_domain_alloc_fwnode(unsigned int type, int id,
>  		n = kasprintf(GFP_KERNEL, "%s-%d", name, id);
>  		break;
>  	default:
> -		n = kasprintf(GFP_KERNEL, "irqchip@%p", data);
> +		n = kasprintf(GFP_KERNEL, "irqchip@%px", data);
>  		break;
>  	}
>  
> @@ -450,7 +450,7 @@ EXPORT_SYMBOL_GPL(irq_domain_check_msi_remap);
>   */
>  void irq_set_default_host(struct irq_domain *domain)
>  {
> -	pr_debug("Default domain set to @0x%p\n", domain);
> +	pr_debug("Default domain set to @0x%px\n", domain);
>  
>  	irq_default_domain = domain;
>  }
> @@ -635,7 +635,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>  	struct device_node *of_node;
>  	int virq;
>  
> -	pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
> +	pr_debug("%s(0x%px, 0x%lx)\n", __func__, domain, hwirq);
>  
>  	/* Look for default domain if nececssary */
>  	if (domain == NULL)
> @@ -644,7 +644,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>  		WARN(1, "%s(, %lx) called with NULL domain\n", __func__, hwirq);
>  		return 0;
>  	}
> -	pr_debug("-> using domain @%p\n", domain);
> +	pr_debug("-> using domain @%px\n", domain);
>  
>  	of_node = irq_domain_get_of_node(domain);
>  
> @@ -921,7 +921,7 @@ static void virq_debug_show_one(struct seq_file *m, struct irq_desc *desc)
>  		chip = irq_data_get_irq_chip(data);
>  		seq_printf(m, "%-15s  ", (chip && chip->name) ? chip->name : "none");
>  
> -		seq_printf(m, "0x%p  ", irq_data_get_irq_chip_data(data));
> +		seq_printf(m, "0x%px  ", irq_data_get_irq_chip_data(data));
>  
>  		seq_printf(m, "   %c    ", (desc->action && desc->action->handler) ? '*' : ' ');
>  		direct = (irq == hwirq) && (irq < domain->revmap_direct_max_irq);
> 

In all honesty, I'd be more inclined to remove this debug feature
altogether, as CONFIG_GENERIC_IRQ_DEBUGFS is more complete and more
useful. Is there any feature missing from that infrastructure that
prevents you from using it instead?

If the answer is "none", then I'll post a patch removing it.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] irqdomain: provide useful debugging information for irq domain

[Thomas Gleixner]

On Wed, 17 Jan 2018, Marc Zyngier wrote:
> In all honesty, I'd be more inclined to remove this debug feature
> altogether, as CONFIG_GENERIC_IRQ_DEBUGFS is more complete and more
> useful. Is there any feature missing from that infrastructure that
> prevents you from using it instead?
> 
> If the answer is "none", then I'll post a patch removing it.

Wanted to do that quite some time ago and never came around to it.

Re: [PATCH] irqdomain: provide useful debugging information for irq domain

[Marc Zyngier]

On 17/01/18 09:18, Thomas Gleixner wrote:
> On Wed, 17 Jan 2018, Marc Zyngier wrote:
>> In all honesty, I'd be more inclined to remove this debug feature
>> altogether, as CONFIG_GENERIC_IRQ_DEBUGFS is more complete and more
>> useful. Is there any feature missing from that infrastructure that
>> prevents you from using it instead?
>>
>> If the answer is "none", then I'll post a patch removing it.
> 
> Wanted to do that quite some time ago and never came around to it.

Same here, but got slightly sidetracked lately. I'll post something
later today.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] irqdomain: provide useful debugging information for irq domain

[Yang, Shunyong]

Hi, Marc and Thomas,

Thanks for your feedback.

On Wed, 2018-01-17 at 10:18 +0100, Thomas Gleixner wrote:
> On Wed, 17 Jan 2018, Marc Zyngier wrote:
> > 
> > In all honesty, I'd be more inclined to remove this debug feature
> > altogether, as CONFIG_GENERIC_IRQ_DEBUGFS is more complete and more
> > useful. Is there any feature missing from that infrastructure that
> > prevents you from using it instead?
> > 
> > If the answer is "none", then I'll post a patch removing it.
> Wanted to do that quite some time ago and never came around to it.

I think they have different purpose.
irq_domain_mapping is an overiew of the mapping of each virq to hwirq
and the domain it belongs. It likes a map or index of each IRQs. I tend
to suggest to keep it.
The nodes under debugfs irq/domains describes the hierarchy of each irq
domain.
The nodes under debugfs irq/irqs describes information of every single
irq.


Thanks
Shunyong

Re: [PATCH] irqdomain: provide useful debugging information for irq domain

[Thomas Gleixner]

On Wed, 17 Jan 2018, Yang, Shunyong wrote:

> Hi, Marc and Thomas,
> 
> Thanks for your feedback.
> 
> On Wed, 2018-01-17 at 10:18 +0100, Thomas Gleixner wrote:
> > On Wed, 17 Jan 2018, Marc Zyngier wrote:
> > > 
> > > In all honesty, I'd be more inclined to remove this debug feature
> > > altogether, as CONFIG_GENERIC_IRQ_DEBUGFS is more complete and more
> > > useful. Is there any feature missing from that infrastructure that
> > > prevents you from using it instead?
> > > 
> > > If the answer is "none", then I'll post a patch removing it.
> > Wanted to do that quite some time ago and never came around to it.
> 
> I think they have different purpose.
> irq_domain_mapping is an overiew of the mapping of each virq to hwirq
> and the domain it belongs. It likes a map or index of each IRQs. I tend
> to suggest to keep it.

And how is that different from:

> The nodes under debugfs irq/irqs describes information of every single
> irq.

Not at all. It contains the complete hierarchical information of each virq.

Thanks,

	tglx

Re: [此邮件可能存在风险] Re: [PATCH] irqdomain: provide useful debugging information for irq domain

[Marc Zyngier]

On 17/01/18 09:47, Yang, Shunyong wrote:
> Hi, Thomas,
> 
> On Wed, 2018-01-17 at 10:33 +0100, Thomas Gleixner wrote:
>> On Wed, 17 Jan 2018, Yang, Shunyong wrote:
>>
>>>
>>> Hi, Marc and Thomas,
>>>
>>> Thanks for your feedback.
>>>
>>> On Wed, 2018-01-17 at 10:18 +0100, Thomas Gleixner wrote:
>>>>
>>>> On Wed, 17 Jan 2018, Marc Zyngier wrote:
>>>>>
>>>>>
>>>>> In all honesty, I'd be more inclined to remove this debug
>>>>> feature
>>>>> altogether, as CONFIG_GENERIC_IRQ_DEBUGFS is more complete and
>>>>> more
>>>>> useful. Is there any feature missing from that infrastructure
>>>>> that
>>>>> prevents you from using it instead?
>>>>>
>>>>> If the answer is "none", then I'll post a patch removing it.
>>>> Wanted to do that quite some time ago and never came around to
>>>> it.
>>> I think they have different purpose.
>>> irq_domain_mapping is an overiew of the mapping of each virq to
>>> hwirq
>>> and the domain it belongs. It likes a map or index of each IRQs. I
>>> tend
>>> to suggest to keep it.
>> And how is that different from:
>>
>>>
>>> The nodes under debugfs irq/irqs describes information of every
>>> single
>>> irq.
>> Not at all. It contains the complete hierarchical information of each
>> virq.
>>
> I think irq_domain_mapping can provide some high-level information in a
> summary style.
> For example, we can check all the IRQs connect to a specific irq chip
> or irq domain.

You mean, something like this:

root@flakes:/sys/kernel/debug/irq# ls -l domains/
total 0
-r--r--r-- 1 root root 0 Jan  1  1970 default
-r--r--r-- 1 root root 0 Jan  1  1970 gpio@e0080000
-r--r--r-- 1 root root 0 Jan  1  1970 gpio@e1050000
-r--r--r-- 1 root root 0 Jan  1  1970 interrupt-controller@e1101000
-r--r--r-- 1 root root 0 Jan  1  1970 v2m@e0080000-2
-r--r--r-- 1 root root 0 Jan  1  1970 v2m@e0080000-3
-r--r--r-- 1 root root 0 Jan  1  1970 v2m@e0080000-4
root@flakes:/sys/kernel/debug/irq# grep -r v2m@e0080000-2 irqs/| cut -f1 -d:
irqs/49
irqs/48
irqs/47
irqs/46
irqs/45
irqs/44
irqs/43
irqs/42
irqs/41

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: Re: Re: [PATCH] irqdomain: provide useful debugging information for irq domain

[Yang, Shunyong]

Hi, Thomas and Marc,

On Wed, 2018-01-17 at 11:01 +0100, Thomas Gleixner wrote:
> On Wed, 17 Jan 2018, Yang, Shunyong wrote:
> > 
> > On Wed, 2018-01-17 at 10:33 +0100, Thomas Gleixner wrote:
> > > 
> > > And how is that different from:
> > > 
> > > > 
> > > > 
> > > > The nodes under debugfs irq/irqs describes information of every
> > > > single
> > > > irq.
> > > Not at all. It contains the complete hierarchical information of
> > > each
> > > virq.
> > > 
> > I think irq_domain_mapping can provide some high-level information
> > in a
> > summary style.
> > For example, we can check all the IRQs connect to a specific irq
> > chip
> > or irq domain.
> You can retrieve the same information from the irq/irqs files. All it
> takes
> is a shell script.
> 
> Aside of that with hierarchical irq domains the old debug output is
> just
> useless.
> 
Umm...Agree. Need I post a patch to remove it?

Thanks
Shunyong

Re: Re: Re: [PATCH] irqdomain: provide useful debugging information for irq domain

[Thomas Gleixner]

On Wed, 17 Jan 2018, Yang, Shunyong wrote:
> On Wed, 2018-01-17 at 11:01 +0100, Thomas Gleixner wrote:
> > You can retrieve the same information from the irq/irqs files. All it
> > takes
> > is a shell script.
> > 
> > Aside of that with hierarchical irq domains the old debug output is
> > just
> > useless.
> > 
> Umm...Agree. Need I post a patch to remove it?

Would be appreciated.

Thanks,

	tglx

Re: [PATCH] irqdomain: provide useful debugging information for irq domain

[Marc Zyngier]

On 17/01/18 10:20, Yang, Shunyong wrote:
> Hi, Thomas and Marc,
> 
> On Wed, 2018-01-17 at 11:01 +0100, Thomas Gleixner wrote:
>> On Wed, 17 Jan 2018, Yang, Shunyong wrote:
>>>
>>> On Wed, 2018-01-17 at 10:33 +0100, Thomas Gleixner wrote:
>>>>
>>>> And how is that different from:
>>>>
>>>>>
>>>>>
>>>>> The nodes under debugfs irq/irqs describes information of every
>>>>> single
>>>>> irq.
>>>> Not at all. It contains the complete hierarchical information of
>>>> each
>>>> virq.
>>>>
>>> I think irq_domain_mapping can provide some high-level information
>>> in a
>>> summary style.
>>> For example, we can check all the IRQs connect to a specific irq
>>> chip
>>> or irq domain.
>> You can retrieve the same information from the irq/irqs files. All it
>> takes
>> is a shell script.
>>
>> Aside of that with hierarchical irq domains the old debug output is
>> just
>> useless.
>>
> Umm...Agree. Need I post a patch to remove it?

I'm on it.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] irqdomain: provide useful debugging information for irq domain

[Yang, Shunyong]

Hi, Marc

On Wed, 2018-01-17 at 10:43 +0000, Marc Zyngier wrote:
> On 17/01/18 10:20, Yang, Shunyong wrote:
> > 
> > Hi, Thomas and Marc,
> > 
> > On Wed, 2018-01-17 at 11:01 +0100, Thomas Gleixner wrote:
> > > 
> > > On Wed, 17 Jan 2018, Yang, Shunyong wrote:
> > > > 
> > > > 
> > > > On Wed, 2018-01-17 at 10:33 +0100, Thomas Gleixner wrote:
> > > > > 
> > > > > 
> > > > > And how is that different from:
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > The nodes under debugfs irq/irqs describes information of
> > > > > > every
> > > > > > single
> > > > > > irq.
> > > > > Not at all. It contains the complete hierarchical information
> > > > > of
> > > > > each
> > > > > virq.
> > > > > 
> > > > I think irq_domain_mapping can provide some high-level
> > > > information
> > > > in a
> > > > summary style.
> > > > For example, we can check all the IRQs connect to a specific
> > > > irq
> > > > chip
> > > > or irq domain.
> > > You can retrieve the same information from the irq/irqs files.
> > > All it
> > > takes
> > > is a shell script.
> > > 
> > > Aside of that with hierarchical irq domains the old debug output
> > > is
> > > just
> > > useless.
> > > 
> > Umm...Agree. Need I post a patch to remove it?
> I'm on it.
> 
In addition to the "%p" to "%px" change in IRQ_DOMAIN_DEBUG you have
posted patch to remove it, my original patch includes several other
changes:
1. change to "%px" in kasprintf() parameters in
function __irq_domain_alloc_fwnode() to build name which reflects the
real pointer address from caller, this may be useful when reading debug
information.
 
n = kasprintf(GFP_KERNEL, "irqchip@%px", data);

2. other 3 changes from "%p" to "%px" in existing pr_debug().

Could I submit a V2 patch which depends on your "irqdomain: Kill
CONFIG_IRQ_DOMAIN_DEBUG" patch to handle this? 
Following is the link of your patch,
https://patchwork.kernel.org/patch/10169367/ 
.

Thanks
Shunyong

Re: [PATCH] irqdomain: provide useful debugging information for irq domain

[Marc Zyngier]

On 18/01/18 01:53, Yang, Shunyong wrote:
> Hi, Marc
> 
> On Wed, 2018-01-17 at 10:43 +0000, Marc Zyngier wrote:
>> On 17/01/18 10:20, Yang, Shunyong wrote:
>>>
>>> Hi, Thomas and Marc,
>>>
>>> On Wed, 2018-01-17 at 11:01 +0100, Thomas Gleixner wrote:
>>>>
>>>> On Wed, 17 Jan 2018, Yang, Shunyong wrote:
>>>>>
>>>>>
>>>>> On Wed, 2018-01-17 at 10:33 +0100, Thomas Gleixner wrote:
>>>>>>
>>>>>>
>>>>>> And how is that different from:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The nodes under debugfs irq/irqs describes information of
>>>>>>> every
>>>>>>> single
>>>>>>> irq.
>>>>>> Not at all. It contains the complete hierarchical information
>>>>>> of
>>>>>> each
>>>>>> virq.
>>>>>>
>>>>> I think irq_domain_mapping can provide some high-level
>>>>> information
>>>>> in a
>>>>> summary style.
>>>>> For example, we can check all the IRQs connect to a specific
>>>>> irq
>>>>> chip
>>>>> or irq domain.
>>>> You can retrieve the same information from the irq/irqs files.
>>>> All it
>>>> takes
>>>> is a shell script.
>>>>
>>>> Aside of that with hierarchical irq domains the old debug output
>>>> is
>>>> just
>>>> useless.
>>>>
>>> Umm...Agree. Need I post a patch to remove it?
>> I'm on it.
>>
> In addition to the "%p" to "%px" change in IRQ_DOMAIN_DEBUG you have
> posted patch to remove it, my original patch includes several other
> changes:
> 1. change to "%px" in kasprintf() parameters in
> function __irq_domain_alloc_fwnode() to build name which reflects the
> real pointer address from caller, this may be useful when reading debug
> information.
>  
> n = kasprintf(GFP_KERNEL, "irqchip@%px", data);

Have you investigated whether %pK would work in a debug context?

> 
> 2. other 3 changes from "%p" to "%px" in existing pr_debug().
> 
> Could I submit a V2 patch which depends on your "irqdomain: Kill
> CONFIG_IRQ_DOMAIN_DEBUG" patch to handle this? 
> Following is the link of your patch,
> https://patchwork.kernel.org/patch/10169367/ 

Sure. If you think something is missing, feel free to send an additional
patch.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] irqdomain: provide useful debugging information for irq domain

[Yang, Shunyong]

Hi, Marc

On Thu, 2018-01-18 at 08:54 +0000, Marc Zyngier wrote:
> On 18/01/18 01:53, Yang, Shunyong wrote:
> > 
> > Hi, Marc
> > 
> > On Wed, 2018-01-17 at 10:43 +0000, Marc Zyngier wrote:
> > > 
> > > On 17/01/18 10:20, Yang, Shunyong wrote:
> > > > 
> > > > 
> > > > Hi, Thomas and Marc,
> > > > 
> > > > On Wed, 2018-01-17 at 11:01 +0100, Thomas Gleixner wrote:
> > > > > 
> > > > > 
> > > > > On Wed, 17 Jan 2018, Yang, Shunyong wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Wed, 2018-01-17 at 10:33 +0100, Thomas Gleixner wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > And how is that different from:
> > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > The nodes under debugfs irq/irqs describes information
> > > > > > > > of
> > > > > > > > every
> > > > > > > > single
> > > > > > > > irq.
> > > > > > > Not at all. It contains the complete hierarchical
> > > > > > > information
> > > > > > > of
> > > > > > > each
> > > > > > > virq.
> > > > > > > 
> > > > > > I think irq_domain_mapping can provide some high-level
> > > > > > information
> > > > > > in a
> > > > > > summary style.
> > > > > > For example, we can check all the IRQs connect to a
> > > > > > specific
> > > > > > irq
> > > > > > chip
> > > > > > or irq domain.
> > > > > You can retrieve the same information from the irq/irqs
> > > > > files.
> > > > > All it
> > > > > takes
> > > > > is a shell script.
> > > > > 
> > > > > Aside of that with hierarchical irq domains the old debug
> > > > > output
> > > > > is
> > > > > just
> > > > > useless.
> > > > > 
> > > > Umm...Agree. Need I post a patch to remove it?
> > > I'm on it.
> > > 
> > In addition to the "%p" to "%px" change in IRQ_DOMAIN_DEBUG you
> > have
> > posted patch to remove it, my original patch includes several other
> > changes:
> > 1. change to "%px" in kasprintf() parameters in
> > function __irq_domain_alloc_fwnode() to build name which reflects
> > the
> > real pointer address from caller, this may be useful when reading
> > debug
> > information.
> >  
> > n = kasprintf(GFP_KERNEL, "irqchip@%px", data);
> Have you investigated whether %pK would work in a debug context?

Although build name with physical address makes it straightforward when
working with tables like MADT. I agree with you about the security
concern. As it's just a name, hashed address is OK.

But it brings new issues. Currently, component like GIC calls this
function with virtual address and ITS with physical address. Is there
possibility that GIC/ITS name is different in each boot? Should we
change GIC/ITS fw handle allocate to name/name-id style?

Thanks
Shunyong