linux-kernel - Re: [Patch v3 03/11] irqchip: axi-intc: Add support for parent intc

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: <81b5f75c-51b5-0482-50d1-f51c5687edbc@imgtec.com>
Date:   Thu, 1 Sep 2016 12:01:47 +0100
From:   Zubair Lutfullah Kakakhel <Zubair.Kakakhel@...tec.com>
To:     Marc Zyngier <marc.zyngier@....com>, <monstr@...str.eu>,
        <ralf@...ux-mips.org>, <tglx@...utronix.de>, <jason@...edaemon.net>
CC:     <soren.brinkmann@...inx.com>, <linux-kernel@...r.kernel.org>,
        <linux-mips@...ux-mips.org>, <michal.simek@...inx.com>,
        <netdev@...r.kernel.org>
Subject: Re: [Patch v3 03/11] irqchip: axi-intc: Add support for parent intc

Hi,

Thanks for the review
Comments inline.

On 08/31/2016 05:57 PM, Marc Zyngier wrote:
> On 31/08/16 17:35, Zubair Lutfullah Kakakhel wrote:
>> The MIPS based xilfpga platform has the following IRQ structure
>>
>> Peripherals --> xilinx_intcontroller -> mips_cpu_int controller
>>
>> Add support for the driver to chain the irq handler
>>
>> Signed-off-by: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@...tec.com>
>>
>> ---
>> V2 -> V3
>> Reused existing parent node instead of finding again.
>> Cleanup up handler based on review
>>
>> V1 -> V2
>>
>> No change
>> ---
>>  drivers/irqchip/irq-axi-intc.c | 24 +++++++++++++++++++++++-
>>  1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-axi-intc.c b/drivers/irqchip/irq-axi-intc.c
>> index cb69241..30bb084 100644
>> --- a/drivers/irqchip/irq-axi-intc.c
>> +++ b/drivers/irqchip/irq-axi-intc.c
>> @@ -15,6 +15,7 @@
>>  #include <linux/of_address.h>
>>  #include <linux/io.h>
>>  #include <linux/bug.h>
>> +#include <linux/of_irq.h>
>>
>>  /* No one else should require these constants, so define them locally here. */
>>  #define ISR 0x00			/* Interrupt Status Register */
>> @@ -154,11 +155,23 @@ static const struct irq_domain_ops xintc_irq_domain_ops = {
>>  	.map = xintc_map,
>>  };
>>
>> +static void xil_intc_irq_handler(struct irq_desc *desc)
>> +{
>> +	u32 pending;
>> +
>> +	do {
>> +		pending = get_irq();
>> +		if (pending == -1U)
>> +			break;
>> +		generic_handle_irq(pending);
>> +	} while (true);
>> +}
>> +
>>  static int __init xilinx_intc_of_init(struct device_node *intc,
>>  					     struct device_node *parent)
>>  {
>>  	u32 nr_irq;
>> -	int ret;
>> +	int ret, irq;
>>  	struct xintc_irq_chip *irqc;
>>
>>  	irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
>> @@ -211,6 +224,15 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
>>  	root_domain = irq_domain_add_linear(intc, nr_irq, &xintc_irq_domain_ops,
>>  					    irqc);
>>
>> +	if (parent) {
>> +		irq = irq_of_parse_and_map(intc, 0);
>> +		if (irq)
>> +			irq_set_chained_handler_and_data(irq,
>> +							 xil_intc_irq_handler,
>> +							 root_domain);
>> +
>> +	}
>> +
>>  	irq_set_default_host(root_domain);
>>
>>  	return 0;
>>
>
> This doesn't seem right. You've now overridden the xintc_irqc pointer,
> so I don't know how you can still process interrupts once you've
> discovered a secondary interrupt controller. You've also allocated a
> second root_domain, changed the default domain to point to the secondary
> controller...
>
> Have you tested this code? Or am I missing something obvious?

Yes it works. I'll try to explain the platform setup a bit.
Perhaps that will make it clear about what I'm trying to do.

UART IRQ -> AXI INTC -> MIPS internal INTC -> CPU

MIPS Internal Interrupt controller in drivers/irqchip/irq-mips-cpu.c
uses irq_domain_add_legacy while AXI Intc uses irq_domain_add_linear

My aim was to set up a chained irq handler with least disturbance.

Hence the above code.

Your concerns are valid. The code is working because read/writes rely
on the static xintc_irqc in the file.
And the second root domain is also not breaking the platform because
the irq-mips-cpu.c uses irq_domain_add_legacy and doesn't use
irq_set_default_host.

# cat /proc/interrupts
            CPU0
   7:      43493      MIPS   7  timer
   8:         83  Xilinx INTC   1-level     eth0
   9:        417  Xilinx INTC   0-level     serial
  10:         15  Xilinx INTC   4-level     10a00000.i2c
ERR:          0
#

Given the above concerns. How about doing things this way?

	if (parent) {
		irq = irq_of_parse_and_map(intc, 0);
		if (irq)
			irq_set_chained_handler_and_data(irq,
							 xil_intc_irq_handler,
							 irqc);

	} else
		irq_set_default_host(root_domain);

default host is only set if no parent exists.
And the irqc pointer is passed as the data.

Thanks
ZubairLK

>
> Thanks,
>
> 	M.
>