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: <55FEB90F.8080109@rempel-privat.de>
Date:	Sun, 20 Sep 2015 15:47:59 +0200
From:	Oleksij Rempel <linux@...pel-privat.de>
To:	Marc Zyngier <marc.zyngier@....com>
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

Am 20.09.2015 um 13:28 schrieb Marc Zyngier:
> 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.

You right.

> 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.

I don't see any need to lock it.

-- 
Regards,
Oleksij


Download attachment "signature.asc" of type "application/pgp-signature" (214 bytes)