linux-kernel - Re: [PATCH] irqchip/mbigen: Fix the clear register offset

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] [day] [month] [year] [list]

Date:   Thu, 27 Apr 2017 09:59:45 +0800
From:   Hanjun Guo <hanjun.guo@...aro.org>
To:     "majun (Euler7)" <majun258@...wei.com>,
        Marc Zyngier <marc.zyngier@....com>,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        tglx@...utronix.de, dingtianhong@...wei.com, guohanjun@...wei.com
Subject: Re: [PATCH] irqchip/mbigen: Fix the clear register offset

On 2017/4/27 9:27, majun (Euler7) wrote:
> Hi Marc:
>
> 在 2017/4/26 16:01, Marc Zyngier 写道:
>> On 26/04/17 04:10, Hanjun Guo wrote:
>>> Hi Majun,
>>>
>>> On 2017/4/25 10:16, Majun wrote:
>>>> From: MaJun <majun258@...wei.com>
>>>>
>>>> Don't minus reserved interrupts (64) when get the clear register offset,because
>>>> the clear register space includes the space of these 64 interrupts.
>>>
>>> Could you mention the background that there is a timeout mechanism
>>> to clear the register in the mbigen to make the code work even we clear
>>> the wrong (and noneffective) register? that will help for review I
>>> think.
>>
>> A timeout? So if you don't clear the interrupt in a timely manner, it
>> will still bypass the masking? That feels very wrong. How is this
>> timeout configured? Can it be entirely disabled?
>
> You are right. The timeout should be turn off usually, it's just a debug or
> testing function in our chip. Because of configuration mistake in BIOS, this function
> is turned on now and covered the bug in clear offset calculate.
> Now, it's time to fix this.

Thanks for the clarify, that means it can be entirely disabled in
the firmware.

>>
>>>
>>>>
>>>> Signed-off-by: MaJun <majun258@...wei.com>
>>>> ---
>>>>  drivers/irqchip/irq-mbigen.c | 1 -
>>>>  1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
>>>> index 061cdb8..75818a5 100644
>>>> --- a/drivers/irqchip/irq-mbigen.c
>>>> +++ b/drivers/irqchip/irq-mbigen.c
>>>> @@ -108,7 +108,6 @@ static inline void get_mbigen_clear_reg(irq_hw_number_t hwirq,
>>>>  {
>>>>  	unsigned int ofst;
>>>>
>>>> -	hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP;
>>>>  	ofst = hwirq / 32 * 4;
>>>>
>>>>  	*mask = 1 << (hwirq % 32);
>>>
>>> How about following to save more lines of code:
>>>
>>> --- a/drivers/irqchip/irq-mbigen.c
>>> +++ b/drivers/irqchip/irq-mbigen.c
>>> @@ -106,10 +106,7 @@ static inline void
>>> get_mbigen_type_reg(irq_hw_number_t hwirq,
>>>   static inline void get_mbigen_clear_reg(irq_hw_number_t hwirq,
>>>                                          u32 *mask, u32 *addr)
>>>   {
>>> -       unsigned int ofst;
>>> -
>>> -       hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP;
>>> -       ofst = hwirq / 32 * 4;
>>> +       unsigned int ofst = hwirq / 32 * 4;
>>>
>>>          *mask = 1 << (hwirq % 32);
>>>          *addr = ofst + REG_MBIGEN_CLEAR_OFFSET;
>>
>> Well, this is not a code deletion contest... ;-)

That's just my personal addiction :)

Thanks
Hanjun