linux-kernel - Re: [PATCH v2 2/2] ARM: irqchip: mxs: add Alpascale ASM9260 support

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: <20150920122843.7cc93682@arm.com>
Date:	Sun, 20 Sep 2015 12:28:43 +0100
From:	Marc Zyngier <marc.zyngier@....com>
To:	Oleksij Rempel <linux@...pel-privat.de>
Cc:	<linux-kernel@...r.kernel.org>, <jason@...edaemon.net>,
	<tglx@...utronix.de>,
	Oleksij Rempel <external.Oleksij.Rempel@...bosch.com>
Subject: Re: [PATCH v2 2/2] ARM: irqchip: mxs: add Alpascale ASM9260 support

On Sat, 19 Sep 2015 07:53:34 +0200
Oleksij Rempel <linux@...pel-privat.de> wrote:

> Am 18.09.2015 um 12:42 schrieb Marc Zyngier:
> > On Fri, 18 Sep 2015 11:18:42 +0200
> > Oleksij Rempel <linux@...pel-privat.de> wrote:
>

[...]

> >> +static void asm9260_mask_irq(struct irq_data *d)
> >> +{
> >> +	raw_spin_lock(&icoll_lock);
> >> +	__raw_writel(icoll_intr_bitshift(d, BM_ICOLL_INTR_ENABLE),
> >> +			icoll_intr_reg(d) + CLR_REG);
> >> +	raw_spin_unlock(&icoll_lock);
> >> +}
> >> +
> >> +static void asm9260_unmask_irq(struct irq_data *d)
> >> +{
> >> +	raw_spin_lock(&icoll_lock);
> >> +	__raw_writel(ASM9260_BM_CLEAR_BIT(d->hwirq),
> >> +		     icoll_priv.clear +
> >> +		     ASM9260_HW_ICOLL_CLEARn(d->hwirq));
> >> +
> >> +	__raw_writel(icoll_intr_bitshift(d, BM_ICOLL_INTR_ENABLE),
> >> +			icoll_intr_reg(d) + SET_REG);
> >> +	raw_spin_unlock(&icoll_lock);
> >> +}
> > 
> > Can you please explain the rational for this lock? mask/unmask use
> > different registers, and it is not obvious to me what race you are
> > trying to avoid here.
> 
> Uff... in one of earliest reviews i was asked to add lock..
> I also was asked to add asm9260 to some existing driver. Not sure if it
> is still making sense.

Adding or removing a lock is not about what people ask you to do. It is
about requirements dictated by either the HW (you need to perform
a given sequence atomically with respect to another code sequence), the
kernel, or both. So I'd like to understand what is the underlying
reason for this lock. It is not disabling interrupts, so it could end
up being called in an interrupt context -> deadlock.

So either the HW requires it and you have the wrong accessors, or it
doesn't, and you can remove it. Either way, we need to know.

	M.
-- 
Jazz is not dead. It just smells funny.
--
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/