linux-kernel - Re: [PATCH (v4) 2/11] MIPS: bmips: Add bcm6345-l2-timer interrupt controller

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: <alpine.DEB.2.11.1511270926580.3572@nanos>
Date:	Fri, 27 Nov 2015 09:37:54 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Simon Arlott <simon@...e.lp0.eu>
cc:	Florian Fainelli <f.fainelli@...il.com>,
	MIPS Mailing List <linux-mips@...ux-mips.org>,
	Jonas Gorski <jogo@...nwrt.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Ralf Baechle <ralf@...ux-mips.org>,
	Jason Cooper <jason@...edaemon.net>,
	Marc Zyngier <marc.zyngier@....com>,
	Kevin Cernekee <cernekee@...il.com>,
	Wim Van Sebroeck <wim@...ana.be>,
	Miguel Gaio <miguel.gaio@...xo.com>,
	Maxime Bizon <mbizon@...ebox.fr>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-watchdog@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>
Subject: Re: [PATCH (v4) 2/11] MIPS: bmips: Add bcm6345-l2-timer interrupt
 controller

Simon,

On Thu, 26 Nov 2015, Simon Arlott wrote:
> +static inline u32 bcm6345_timer_read_int_status(struct bcm6345_timer *timer)
> +{
> +	if (timer->interrupt_bits == 32)
> +		return __raw_readl(timer->base + timer->regs[TIMER_INT_STATUS]);
> +	else
> +		return __raw_readb(timer->base + timer->regs[TIMER_INT_STATUS]);
> +}

Instead of having that pile of conditionals you could just define two
functions and have a function pointer in struct bcm6345_timer which
you initialize at init time.

> +static inline void bcm6345_timer_write_control(struct bcm6345_timer *timer,
> +	unsigned int id, u32 val)
> +{
> +	if (id >= timer->nr_timers) {
> +		WARN(1, "%s: %d >= %d", __func__, id, timer->nr_timers);

This is more than silly. You call that form the init function via:

	for (i = 0; i < timer->nr_timers; i++)

Hmm?

> +static void bcm6345_timer_unmask(struct irq_data *d)
> +{
> +	struct bcm6345_timer *timer = irq_data_get_irq_chip_data(d);
> +	unsigned long flags;
> +	u8 val;
> +
> +	if (d->hwirq < timer->nr_timers) {

Again. You can have two different interrupt chips without that
completely undocumented and non obvious conditional.

BTW, how are those simple interrupts masked at all?

> +		raw_spin_lock_irqsave(&timer->lock, flags);
> +		val = bcm6345_timer_read_int_enable(timer);
> +		val |= BIT(d->hwirq);
> +		bcm6345_timer_write_int_enable(timer, val);
> +		raw_spin_unlock_irqrestore(&timer->lock, flags);
> +	}
> +}

> +	raw_spin_lock_init(&timer->lock);
> +	timer->regs = regs;
> +	timer->interrupt_bits = interrupt_bits;
> +	timer->nr_timers = nr_timers;
> +	timer->nr_interrupts = nr_timers + 1;

What is that extra interrupt about? For the casual reader this looks
like a bug ... Comments exist for a reason.

Thanks,

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