linux-kernel - Re: [PATCH RESEND 3/3] irqchip: orion: reverse irq handling priority

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: <535EB4C1.9090603@gmail.com>
Date:	Mon, 28 Apr 2014 22:06:25 +0200
From:	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	Jason Cooper <jason@...edaemon.net>, Andrew Lunn <andrew@...n.ch>,
	Gregory Clement <gregory.clement@...e-electrons.com>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND 3/3] irqchip: orion: reverse irq handling priority

On 04/28/2014 09:39 PM, Thomas Gleixner wrote:
> On Sat, 26 Apr 2014, Sebastian Hesselbarth wrote:
> 
>> Non-DT irq handlers were working through irq causes from most-significant
>> to least-significant bit, while DT irqchip driver does it the other way
>> round. This revealed some more HW issues on Kirkwood peripheral IP, where
>> spurious sdio irqs can happen although IP's irq enable registers are all
>> zero. Although, not directly related with the described issue, reverse
>> irq bit handling back to original order by replacing ffs() with fls().
> 
> So why are we reverting to the original order?
> 
> The explanation above is just confusing.

Actually, I first wanted to reply "The original order worked for
years, so get back to it." But then I thought about finding a better
answer and remembered some comment of Russell a while ago.

I disassembled the generated binary and the original order saves two
instructions for each bit count using clz.

With this patch:
  60:   e3a07001        mov     r7, #1
  64:   e16f3f14        clz     r3, r4
  68:   e263301f        rsb     r3, r3, #31
  6c:   e1c44317        bic     r4, r4, r7, lsl r3
  70:   e5951004        ldr     r1, [r5, #4]

Without this patch:
  60:   e3a06001        mov     r6, #1
  64:   e2643000        rsb     r3, r4, #0
  68:   e0033004        and     r3, r3, r4
  6c:   e16f3f13        clz     r3, r3
  70:   e263301f        rsb     r3, r3, #31
  74:   e1c44316        bic     r4, r4, r6, lsl r3
  78:   e5971004        ldr     r1, [r7, #4]

You want me to reword the commit message accordingly?

Sebastian

>> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
>> Acked-by: Jason Cooper <jason@...edaemon.net>
>> ---
>> Cc: Jason Cooper <jason@...edaemon.net>
>> Cc: Andrew Lunn <andrew@...n.ch>
>> Cc: Gregory Clement <gregory.clement@...e-electrons.com>
>> Cc: Thomas Gleixner <tglx@...utronix.de>
>> Cc: linux-arm-kernel@...ts.infradead.org
>> Cc: linux-kernel@...r.kernel.org
>> ---
>>  drivers/irqchip/irq-orion.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-orion.c b/drivers/irqchip/irq-orion.c
>> index e25f246cd2fb..34d18b48bb78 100644
>> --- a/drivers/irqchip/irq-orion.c
>> +++ b/drivers/irqchip/irq-orion.c
>> @@ -42,7 +42,7 @@ __exception_irq_entry orion_handle_irq(struct pt_regs *regs)
>>  		u32 stat = readl_relaxed(gc->reg_base + ORION_IRQ_CAUSE) &
>>  			gc->mask_cache;
>>  		while (stat) {
>> -			u32 hwirq = ffs(stat) - 1;
>> +			u32 hwirq = __fls(stat);
>>  			u32 irq = irq_find_mapping(orion_irq_domain,
>>  						   gc->irq_base + hwirq);
>>  			handle_IRQ(irq, regs);
>> @@ -117,7 +117,7 @@ static void orion_bridge_irq_handler(unsigned int irq, struct irq_desc *desc)
>>  		   gc->mask_cache;
>>  
>>  	while (stat) {
>> -		u32 hwirq = ffs(stat) - 1;
>> +		u32 hwirq = __fls(stat);
>>  
>>  		generic_handle_irq(irq_find_mapping(d, gc->irq_base + hwirq));
>>  		stat &= ~(1 << hwirq);
>> -- 
>> 1.9.1
>>
>>

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