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

[Oleksij Rempel]

Am 20.09.2015 um 13:28 schrieb Marc Zyngier:
> On Sat, 19 Sep 2015 07:53:34 +0200
> Oleksij Rempel <linux@xxxxxxxxxxxxxxxx> wrote:
> 
>> Am 18.09.2015 um 12:42 schrieb Marc Zyngier:
>>> On Fri, 18 Sep 2015 11:18:42 +0200
>>> Oleksij Rempel <linux@xxxxxxxxxxxxxxxx> 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

Attachment: signature.asc
Description: OpenPGP digital signature