linux-kernel - Re: [PATCH v2 6/9] irqchip / gic: Add stacked irqdomain support for ACPI based GICv2 init

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]

Date:	Sat, 27 Jun 2015 11:52:14 +0800
From:	Hanjun Guo <hanjun.guo@...aro.org>
To:	Lorenzo Pieralisi <lorenzo.pieralisi@....com>
CC:	Marc Zyngier <Marc.Zyngier@....com>,
	Jason Cooper <jason@...edaemon.net>,
	Will Deacon <Will.Deacon@....com>,
	Catalin Marinas <Catalin.Marinas@....com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Thomas Gleixner <tglx@...utronix.de>,
	Jiang Liu <jiang.liu@...ux.intel.com>,
	Arnd Bergmann <arnd@...db.de>,
	Tomasz Nowicki <tomasz.nowicki@...aro.org>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	Olof Johansson <olof@...om.net>, Wei Huang <wei@...hat.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linaro-acpi@...ts.linaro.org" <linaro-acpi@...ts.linaro.org>
Subject: Re: [PATCH v2 6/9] irqchip / gic: Add stacked irqdomain support for
 ACPI based GICv2 init

On 06/24/2015 01:38 AM, Lorenzo Pieralisi wrote:
> On Tue, Jun 23, 2015 at 04:11:38PM +0100, Hanjun Guo wrote:
>
> [...]
>
>>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>>>> index 8fc67bc..d1b2131 100644
>>>> --- a/drivers/irqchip/irq-gic.c
>>>> +++ b/drivers/irqchip/irq-gic.c
>>>> @@ -851,15 +851,22 @@ static struct notifier_block gic_cpu_notifier = {
>>>>    static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>>>>    				unsigned int nr_irqs, void *arg)
>>>>    {
>>>> -	int i, ret;
>>>> +	int i;
>>>>    	irq_hw_number_t hwirq;
>>>> -	unsigned int type = IRQ_TYPE_NONE;
>>>> -	struct of_phandle_args *irq_data = arg;
>>>>
>>>> -	ret = gic_irq_domain_xlate(domain, irq_data->np, irq_data->args,
>>>> -				   irq_data->args_count, &hwirq, &type);
>>>> -	if (ret)
>>>> -		return ret;
>>>> +	if (domain->of_node) {	/* DT case */
>>>> +		int ret;
>>>> +		unsigned int type = IRQ_TYPE_NONE;
>>>> +		struct of_phandle_args *irq_data = arg;
>>>> +
>>>> +		ret = gic_irq_domain_xlate(domain, irq_data->np,
>>>> +					irq_data->args,
>>>> +					irq_data->args_count, &hwirq, &type);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	} else {	/* ACPI case */
>>>> +		hwirq = (irq_hw_number_t)*(u32 *)arg;
>>>> +	}
>>>
>>> If domain->of_node is NULL and system booted with DT the code above
>>> does not fail (and if it fails almost certainly that won't be graceful)
>>> but it should.
>>
>> how about the following logic?
>>
>> if (!domain->of_node && acpi_disabled)
>> 	return -ENODEV;
>> else if (domain->of_node)
>> 	dt related code;
>> else
>> 	ACPI related code;
>
> Code is not checking the node at present so:
>
> if (acpi_disabled)
> 	dt code;
> else
> 	ACPI code;
>
> would do, but that's a nit.
>
>>>>    	for (i = 0; i < nr_irqs; i++)
>>>>    		gic_irq_domain_map(domain, virq + i, hwirq + i);
>>>> @@ -945,11 +952,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>>>>    		gic_irqs = 1020;
>>>>    	gic->gic_irqs = gic_irqs;
>>>>
>>>> -	if (node) {		/* DT case */
>>>> +	if (node || !acpi_disabled) {		/* DT or ACPI case */
>>>>    		gic->domain = irq_domain_add_linear(node, gic_irqs,
>>>>    						    &gic_irq_domain_hierarchy_ops,
>>>>    						    gic);
>
> I think this is a bit more worrying, I mean passing a NULL node pointer to
> the irqdomain layer which basically means you are booting out of ACPI

I'm little confused here, would you mind explaining more for your
worrying? To me, node pointer is optional and it's ok for ACPI
case.

> (for you, if that's true for the irq_domain_add_linear implementation
> that's another story), the node pointer should be optional but you
> need feedback from IRQ layer maintainers here.

Sure.

Thanks
Hanjun
--
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/