2018-01-17 04:56:27

by Shunyong Yang

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

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 <[email protected]>
---
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


2018-01-17 08:37:08

by Marc Zyngier

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

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 <[email protected]>
> ---
> 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...

2018-01-17 09:18:37

by Thomas Gleixner

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

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.

2018-01-17 09:26:05

by Marc Zyngier

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

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...

2018-01-17 09:26:23

by Shunyong Yang

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

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

2018-01-17 09:33:46

by Thomas Gleixner

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

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