linux-kernel - Re: [Patch Part2 v3 01/24] irqdomain: Introduce new interfaces to support hierarchy irqdomains

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <544FF900.4020601@arm.com>
Date:	Tue, 28 Oct 2014 20:13:52 +0000
From:	Marc Zyngier <marc.zyngier@....com>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	Yingjoe Chen <yingjoe.chen@...iatek.com>,
	Jiang Liu <jiang.liu@...ux.intel.com>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Randy Dunlap <rdunlap@...radead.org>,
	Yinghai Lu <yinghai@...nel.org>,
	Borislav Petkov <bp@...en8.de>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	Jonathan Corbet <corbet@....net>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Tony Luck <tony.luck@...el.com>,
	Joerg Roedel <joro@...tes.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"x86@...nel.org" <x86@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	Matthias Brugger <matthias.bgg@...il.com>
Subject: Re: [Patch Part2 v3 01/24] irqdomain: Introduce new interfaces to
 support hierarchy irqdomains

On 28/10/14 19:37, Thomas Gleixner wrote:
> On Tue, 28 Oct 2014, Yingjoe Chen wrote:
>> On Tue, 2014-10-28 at 16:26 +0800, Jiang Liu wrote:
>> <deleted...>
>>> @@ -471,7 +469,7 @@ unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data)
>>>  	struct irq_domain *domain;
>>>  	irq_hw_number_t hwirq;
>>>  	unsigned int type = IRQ_TYPE_NONE;
>>> -	unsigned int virq;
>>> +	int virq;
>>>  
>>>  	domain = irq_data->np ? irq_find_host(irq_data->np) : irq_default_domain;
>>>  	if (!domain) {
>>> @@ -480,6 +478,11 @@ unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data)
>>>  		return 0;
>>>  	}
>>>  
>>> +	if (irq_domain_is_hierarchy(domain)) {
>>> +		virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, irq_data);
>>> +		return virq <= 0 ? 0 : virq;
>>> +	}
>>> +
>>>  	/* If domain has no translation, then we assume interrupt line */
>>>  	if (domain->ops->xlate == NULL)
>>>  		hwirq = irq_data->args[0];
>>> @@ -540,8 +543,8 @@ unsigned int irq_find_mapping(struct irq_domain *domain,
>>>  		return 0;
>>>  
>>>  	if (hwirq < domain->revmap_direct_max_irq) {
>>> -		data = irq_get_irq_data(hwirq);
>>> -		if (data && (data->domain == domain) && (data->hwirq == hwirq))
>>> +		data = irq_domain_get_irq_data(domain, hwirq);
>>> +		if (data && data->hwirq == hwirq)
>>>  			return hwirq;
>>>  	}
>>
>> Why do you want to move irq_domain_alloc_irqs up there?
> 
> That's the wrong question. The proper question is whether we can move
> it after the xlate part. I don't see a reason why we cannot do that.
> 
> Jiang was right not to incorporate your patch to do that simply
> because we want to keep the history of the evolution of this
> code. Your patch is an enhancement and it needs to be discussed and
> applied seperately.
> 
> So while we are at it:
> 
>> +	if (irq_domain_is_hierarchy(domain)) {
>> +		if (domain->ops->xlate) {
>> +			/*
>> +			 * If we've already configured this interrupt,
>> +			 * don't do it again, or hell will break loose.
>> +			 */
>> +			virq = irq_find_mapping(domain, hwirq);
>> +			if (virq)
>> +				return virq;
> 
> I can understand that it is an issue if the mapping exists already,
> but I have to ask WHY is it correct behaviour to call into that code
> for an existing mapping.

As I have originally looked at this, I'll answer the question:

The generic DT code parses the whole tree, and generates platform
devices as it goes. As part of the platform device creation, it
populates the IRQ resources, which translates into calling into
irq_create_of_mapping(). You could argue that this behaviour is crazy,
and I wouldn't disagree.

See http://www.spinics.net/lists/devicetree/msg53164.html for more gory
details.

> And why would this check only apply if domain->ops->xlate is set?
> irq_create_mapping() does it unconditionally.

My original code used the xlate callback to parse the opaque irq_data,
computing hwirq, and I suspect this is a leftover of it. The above code
seems to pull hwirq out of thin air, which is probably not the intended
behaviour. Joe?

Thanks,

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/