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

[Marc Zyngier <marc.zyngier@arm.com>]

On Sat, 19 Sep 2015 07:53:34 +0200
Oleksij Rempel <linux@rempel-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@rempel-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.