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 for Android: free password hash cracker in your pocket

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

Date:	Tue, 28 Oct 2014 20:37:17 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Yingjoe Chen <yingjoe.chen@...iatek.com>
cc:	Jiang Liu <jiang.liu@...ux.intel.com>,
	Marc Zyngier <marc.zyngier@....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 <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, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, linux-acpi@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.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 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.

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

> Joe.C
> >  
> > @@ -709,3 +712,341 @@ const struct irq_domain_ops irq_domain_simple_ops = {

Please remove the rest of the mail to which you are not answering next time.

Thanks,

	tglx
--
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/