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]

Date:	Thu, 18 Apr 2013 11:35:22 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.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>
Subject: Re: [PATCH v3] irqchip: Add support for ARMv7-M's NVIC

On Wed, 17 Apr 2013, Uwe Kleine-König wrote:
> +struct nvic_bank_data {
> +	/*
> +	 * For irq i base holds nvic_base + 4 * i / 32. So you can access the
> +	 * right ISER register (i.e ISER[i / 32]) by just taking base + ISER.
> +	 * Ditto for ICER.
> +	 */
> +	void __iomem *base;
> +};

What's the point of a struct with a single member? Why not having an
array of base pointers ?

> +static struct nvic_chip_data {
> +	struct irq_domain *domain;
> +	struct nvic_bank_data bdata[NVIC_MAX_BANKS];
> +} nvic_chip_data;
> +
> +asmlinkage void __exception_irq_entry
> +nvic_do_IRQ(irq_hw_number_t hwirq, struct pt_regs *regs)
> +{
> +	unsigned int irq = irq_linear_revmap(nvic_chip_data.domain, hwirq);
> +
> +	handle_IRQ(irq, regs);
> +}
> +
> +static inline void __iomem *nvic_bank_base(struct irq_data *d)
> +{
> +	struct nvic_bank_data *bank_data = irq_data_get_irq_chip_data(d);
> +	return bank_data->base;
> +}
> +
> +static void nvic_mask_irq(struct irq_data *d)
> +{
> +	u32 mask = 1 << (d->hwirq % 32);
> +
> +	writel_relaxed(mask, nvic_bank_base(d) + NVIC_ICER);
> +}
> +
> +static void nvic_unmask_irq(struct irq_data *d)
> +{
> +	u32 mask = 1 << (d->hwirq % 32);
> +
> +	writel_relaxed(mask, nvic_bank_base(d) + NVIC_ISER);
> +}

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.

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.

Thanks,

	tglx