linux-kernel - Re: [PATCH v3] irqchip: Add support for ARMv7-M's NVIC

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: <20130422100219.GH15233@pengutronix.de>
Date:	Mon, 22 Apr 2013 12:02:19 +0200
From:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	kernel@...gutronix.de, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org,
	Jonathan Austin <jonathan.austin@....com>,
	Catalin Marinas <catalin.marinas@....com>,
	Arnd Bergmann <arnd@...db.de>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Grant Likely <grant.likely@...aro.org>,
	Paul Mundt <lethal@...ux-sh.org>, linux-sh@...r.kernel.org
Subject: Re: [PATCH v3] irqchip: Add support for ARMv7-M's NVIC

Hello,

(for the new readers of this thread: This is about using

	u32 mask = 1 << (d->hwirq % 32);

instead of

	u32 mask = 1 << (d->irq - gc->irq_base);

in the callbacks for the irq generic chip.)

On Fri, Apr 19, 2013 at 05:09:27PM +0200, Uwe Kleine-König wrote:
> On Thu, Apr 18, 2013 at 11:35:22AM +0200, Thomas Gleixner wrote:
> > On Wed, 17 Apr 2013, Uwe Kleine-König wrote:
> > How is that different from what the generic irq chip implementation
> > does? The only difference is that mask is generated by d->hwirq and
> > not by d->irq. And due to the fact, that you use a full linear mapping
> > between hwirq and virq the generic code simply works.
> I'm not sure what you mean when you say "full linear mapping". AFAICT
> using irq_domain_add_linear doesn't imply that two consecutive hardware
> irq numbers get consecutive Linux irq numbers, so using d->irq won't work.
>  
> > Even if it would not work, it would be trivial to extend the generic
> > chip with that functionality instead of hacking another slightly
> > different copy of the same thing.
> I will try that and report back.
I wonder if using hwirq % 32 should work everywhere where now d->irq -
gc->irqbase is used. Depending on d->hwirq and not d->irq has the upside
of working with non-legacy irq domains, too.

Looking at next-20130419 the affected functions
(irq_gc_mask_disable_reg, irq_gc_mask_set_bit, irq_gc_mask_clr_bit,
irq_gc_unmask_enable_reg, irq_gc_ack_set_bit, irq_gc_ack_clr_bit,
irq_gc_mask_disable_reg_and_ack, irq_gc_eoi, irq_gc_set_wake) are used
in:

arch/arm/mach-davinci/irq.c
arch/arm/mach-imx/avic.c
arch/arm/mach-imx/tzic.c
arch/arm/mach-omap2/irq.c
arch/arm/mach-omap2/prm_common.c

arch/arm/mach-s5p64x0/common.c
	-> uses irq_base=101 for irq_alloc_generic_chip
arch/arm/plat-orion/gpio.c
	-> depends on how orion_gpio_of_init is called. No callers
	found.

arch/arm/plat-orion/irq.c
arch/arm/plat-samsung/irq-vic-timer.c
	-> used for a single irq that isn't a multiple of 32

arch/arm/plat-samsung/s5p-irq-gpioint.c
	-> would need % 8?

arch/mips/jz4740/gpio.c
	-> JZ4740_IRQ_GPIO(0) != JZ4740_IRQ_GPIO0 ?
	-> uses 56 + i * 32 as irqbase

arch/mips/jz4740/irq.c
	-> uses 8 as irqbase

arch/sh/boards/mach-se/7343/irq.c
	-> uses irq_base = irq_linear_revmap(se7343_irq_domain, 0) where
	se7343_irq_domain is a linear domain.
	AFAICT this is a bug. (After adding the domain they map all irqs
	in increasing order which currently seems to guarantee that it
	works. But IMHO it should use a legacy domain.)

arch/sh/boards/mach-se/7722/irq.c
	as above.

drivers/gpio/gpio-mxc.c
drivers/gpio/gpio-mxs.c
drivers/gpio/gpio-omap.c
drivers/gpio/gpio-sodaville.c
drivers/irqchip/irq-sirfsoc.c
drivers/mfd/jz4740-adc.c
	-> uses platform_get_irq(pdev, 1) as irq_base for 5 irqs.

For the uncommented files using %32 instead of -gc->irq_base should
work.

So it seems I cannot just substitute how the mask is called. 

The options I see are:

 - introduce a new set of functions
   Do you have a nice naming scheme?
   irq_gc_unmask_enable_reg_hwirqmod32? Or should I rename the existing
   ones to irq_gc_unmask_enable_reg_irqbaseoffset?
 - use
 	u32 mask = 1 << (d->hwirq - gc->irq_base) % 32;
   This is ugly but might work assuming irq_base == 0 for chips with irq
   domain support and hwirq == irq for the others.

I'm not lucky with the options, so I'm looking forward to suggestions.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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/