Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1630152 > unrolled thread

[PATCH] irqchip/mbigen: Fix the clear register offset

Started byMajun <majun258@huawei.com>
First post2017-04-25 04:20 +0200
Last post2017-04-27 04:10 +0200
Articles 5 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] irqchip/mbigen: Fix the clear register offset Majun <majun258@huawei.com> - 2017-04-25 04:20 +0200
    Re: [PATCH] irqchip/mbigen: Fix the clear register offset Hanjun Guo <hanjun.guo@linaro.org> - 2017-04-26 05:20 +0200
      Re: [PATCH] irqchip/mbigen: Fix the clear register offset Marc Zyngier <marc.zyngier@arm.com> - 2017-04-26 10:10 +0200
        Re: [PATCH] irqchip/mbigen: Fix the clear register offset "majun (Euler7)" <majun258@huawei.com> - 2017-04-27 03:30 +0200
          Re: [PATCH] irqchip/mbigen: Fix the clear register offset Hanjun Guo <hanjun.guo@linaro.org> - 2017-04-27 04:10 +0200

#1630152 — [PATCH] irqchip/mbigen: Fix the clear register offset

FromMajun <majun258@huawei.com>
Date2017-04-25 04:20 +0200
Subject[PATCH] irqchip/mbigen: Fix the clear register offset
Message-ID<tzYf7-6TM-5@gated-at.bofh.it>
From: MaJun <majun258@huawei.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.

Signed-off-by: MaJun <majun258@huawei.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);
-- 
1.7.12.4

[toc] | [next] | [standalone]

#1631125

FromHanjun Guo <hanjun.guo@linaro.org>
Date2017-04-26 05:20 +0200
Message-ID<tAlEJ-54X-7@gated-at.bofh.it>
In reply to#1630152
Hi Majun,

On 2017/4/25 10:16, Majun wrote:
> From: MaJun <majun258@huawei.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.

>
> Signed-off-by: MaJun <majun258@huawei.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;

Thanks
Hanjun

[toc] | [prev] | [next] | [standalone]

#1631221

FromMarc Zyngier <marc.zyngier@arm.com>
Date2017-04-26 10:10 +0200
Message-ID<tAqbn-80O-3@gated-at.bofh.it>
In reply to#1631125
On 26/04/17 04:10, Hanjun Guo wrote:
> Hi Majun,
> 
> On 2017/4/25 10:16, Majun wrote:
>> From: MaJun <majun258@huawei.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?

> 
>>
>> Signed-off-by: MaJun <majun258@huawei.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... ;-)

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1631820

From"majun (Euler7)" <majun258@huawei.com>
Date2017-04-27 03:30 +0200
Message-ID<tAGpQ-1Uc-7@gated-at.bofh.it>
In reply to#1631221
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@huawei.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
MaJun

> 
>>
>>>
>>> Signed-off-by: MaJun <majun258@huawei.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... ;-)
> 
> 	M.
>

[toc] | [prev] | [next] | [standalone]

#1631827

FromHanjun Guo <hanjun.guo@linaro.org>
Date2017-04-27 04:10 +0200
Message-ID<tAH2x-2nz-1@gated-at.bofh.it>
In reply to#1631820
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@huawei.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@huawei.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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web