linux-kernel - RE: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips

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:	Wed, 2 Dec 2015 15:08:51 +0000
From:	Noam Camus <noamc@...hip.com>
To:	Marc Zyngier <marc.zyngier@....com>,
	"linux-snps-arc@...ts.infradead.org" 
	<linux-snps-arc@...ts.infradead.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Chris Metcalf" <cmetcalf@...hip.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"Jason Cooper" <jason@...edaemon.net>
Subject: RE: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips

>From: Marc Zyngier [mailto:marc.zyngier@....com] 
>Sent: Tuesday, December 01, 2015 3:29 PM

> +  interrupt source. The value shall be 1.

>So you never have to encode the interrupt trigger type? Do you only support edge or level?
I Always use level sensitive.

> +
> +#define NPS_GIM_P_EN	0x100	/* Peripheral interrupts source enable */
> +#define NPS_GIM_P_BLK	0x118	/* Peripheral interrupts blocking for sources */

>>Are these the interrupts the peripherals are using? If yes, they really have nothing to do here...
I will move this from here 
>> +	__asm__ __volatile__ (
>> +	"       .word %0\n"
>> +	:
>> +	: "i"(CTOP_INST_RSPI_GIC_0_R12)
>> +	: "memory");

>Silly question: why cannot you just write the actual instruction instead of shoving the instruction like this? Also, .inst would be more appropriate...
[Noam Camus] Since this is instruction that yet is not part of up-streamed binutils of ARC.  Now ARC maintainer can build our kernel with generic ARC toolchain. 

>> +static int nps400_irq_map(struct irq_domain *d, unsigned int irq,
>> +			  irq_hw_number_t hw)
>> +{
>> +	switch (irq) {
>> +	case TIMER0_IRQ:
>> +#if defined(CONFIG_SMP)
>> +	case IPI_IRQ:
>> +#endif
>> +		irq_set_chip_and_handler(irq, &nps400_irq_chip_percpu,
>> +					 handle_percpu_irq);
>> +	break;
>> +	default:
>> +		irq_set_chip_and_handler(irq, &nps400_irq_chip_fasteoi,
>> +					 handle_fasteoi_irq);
>> +	break;
>> +	}

>No. This is just wrong. Either you get per interrupt information from the device tree to configure the interrupt the right way, or you have different interrupt controllers for each device.
I am not sure how you want me to get it from DTB? Please refer to some reference.

>But using the Linux irq number is always wrong. You should only consider the hwirq.
I will change

> +
> +	nps400_root_domain = irq_domain_add_legacy(node, NR_CPU_IRQS, 0, 0,
> +						   &nps400_irq_ops, NULL);

>And that's why you can get away with the above horror. Don't use legacy domains. This stuff is by no mean legacy.
So what is my alternative here?

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