linux-kernel - Re: [PATCH 3.18-rc3 v8 1/4] irqchip: gic: Make gic_raise

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <54737BB3.8060205@linaro.org>
Date:	Mon, 24 Nov 2014 18:40:51 +0000
From:	Daniel Thompson <daniel.thompson@...aro.org>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	Jason Cooper <jason@...edaemon.net>,
	Russell King <linux@....linux.org.uk>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	patches@...aro.org, linaro-kernel@...ts.linaro.org,
	John Stultz <john.stultz@...aro.org>,
	Sumit Semwal <sumit.semwal@...aro.org>,
	Dirk Behme <dirk.behme@...bosch.com>,
	Daniel Drake <drake@...lessm.com>,
	Dmitry Pervushin <dpervushin@...il.com>,
	Marc Zyngier <marc.zyngier@....com>
Subject: Re: [PATCH 3.18-rc3 v8 1/4] irqchip: gic: Make gic_raise_softirq()
 FIQ-safe

On 24/11/14 18:20, 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.

Indeed. For that reason I've been pretty anxious to hear your views on
this one.

Older versions of this patch did retain the raw lock but the code ends
up looking a bit weird and resulted in negative comments during review:

if (in_nmi())
        raw_spin_lock(&fiq_exclusive_cpu_map_lock);
else
        raw_spin_lock_irqsave(&irq_controller_lock, flags);

The above form relies for correctness on the fact the b.L switcher code
can take both locks and already runs with FIQ disabled.


> Surely nothing I worry too much about given the current state of RT.

Hobby or not, I don't want to make your work here any harder. I could go
back to the old form.

Alternatively I could provide a patch to go in -rt that converts the rw
locks to spin locks but that just sounds like a maintenance hassle for you.


Daniel.

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