Re: [PATCH 3.18-rc3 v8 1/4] irqchip: gic: Make gic_raise_softirq() FIQ-safe

[Thomas Gleixner]

On Mon, 24 Nov 2014, Thomas Gleixner wrote:
> On Fri, 14 Nov 2014, Daniel Thompson wrote:
> > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> > index 38493ff28fa5..0db62a6f1ee3 100644
> > --- a/drivers/irqchip/irq-gic.c
> > +++ b/drivers/irqchip/irq-gic.c
> > @@ -73,6 +73,13 @@ struct gic_chip_data {
> >  static DEFINE_RAW_SPINLOCK(irq_controller_lock);
> >  
> >  /*
> > + * This lock may be locked for reading by FIQ handlers. Thus although
> > + * read locking may be used liberally, write locking must only take
> > + * place only when local FIQ handling is disabled.
> > + */
> > +static DEFINE_RWLOCK(fiq_safe_cpu_map_lock);
> > +
> > +/*
> >   * The GIC mapping of CPU interfaces does not necessarily match
> >   * the logical CPU numbering.  Let's use a mapping as returned
> >   * by the GIC itself.
> > @@ -624,7 +631,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
> >  	int cpu;
> >  	unsigned long flags, map = 0;
> >  
> > -	raw_spin_lock_irqsave(&irq_controller_lock, flags);
> > +	read_lock_irqsave(&fiq_safe_cpu_map_lock, flags);
> 
> Just for the record:
> 
> You might have noticed that you replace a raw lock with a non raw
> one. That's not an issue on mainline, but that pretty much renders
> that code broken for RT.
>   
> Surely nothing I worry too much about given the current state of RT.

And having a second thought here. Looking at the protection scope
independent of the spin vs. rw lock

gic_raise_softirq()

	lock();

	/* Does not need any protection */
	for_each_cpu(cpu, mask)
                map |= gic_cpu_map[cpu];

	/*
	 * Can be outside the lock region as well as it makes sure
	 * that previous writes (usually the IPI data) are visible
	 * before the write to the SOFTINT register.
	 */
	 dmb(ishst);

	/* Why needs this protection? */
	write(map, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT));

	unlock();

gic_migrate_target()

	....
	lock();

	/* Migrate all peripheral interrupts */

	unlock();

So what's the point of that protection?

gic_raise_softirq() is used to send IPIs, which are PPIs on the target
CPUs so they are not affected from the migration of the peripheral
interrupts at all.

The write to the SOFTINT register in gic_migrate_target() is not
inside the lock region. So what's serialized by the lock in
gic_raise_softirq() at all?

Either I'm missing something really important here or this locking
exercise in gic_raise_softirq() and therefor the rwlock conversion is
completely pointless.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/