* [PATCH] irqchip/mbigen: Fix the clear register offset @ 2017-04-25 2:16 Majun 2017-04-26 3:10 ` Hanjun Guo 0 siblings, 1 reply; 5+ messages in thread From: Majun @ 2017-04-25 2:16 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH] irqchip/mbigen: Fix the clear register offset 2017-04-25 2:16 [PATCH] irqchip/mbigen: Fix the clear register offset Majun @ 2017-04-26 3:10 ` Hanjun Guo 2017-04-26 8:01 ` Marc Zyngier 0 siblings, 1 reply; 5+ messages in thread From: Hanjun Guo @ 2017-04-26 3:10 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] irqchip/mbigen: Fix the clear register offset 2017-04-26 3:10 ` Hanjun Guo @ 2017-04-26 8:01 ` Marc Zyngier 2017-04-27 1:27 ` majun (Euler7) 0 siblings, 1 reply; 5+ messages in thread From: Marc Zyngier @ 2017-04-26 8:01 UTC (permalink / raw) To: linux-arm-kernel 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... ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] irqchip/mbigen: Fix the clear register offset 2017-04-26 8:01 ` Marc Zyngier @ 2017-04-27 1:27 ` majun (Euler7) 2017-04-27 1:59 ` Hanjun Guo 0 siblings, 1 reply; 5+ messages in thread From: majun (Euler7) @ 2017-04-27 1:27 UTC (permalink / raw) To: linux-arm-kernel 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. > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] irqchip/mbigen: Fix the clear register offset 2017-04-27 1:27 ` majun (Euler7) @ 2017-04-27 1:59 ` Hanjun Guo 0 siblings, 0 replies; 5+ messages in thread From: Hanjun Guo @ 2017-04-27 1:59 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-04-27 1:59 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-04-25 2:16 [PATCH] irqchip/mbigen: Fix the clear register offset Majun 2017-04-26 3:10 ` Hanjun Guo 2017-04-26 8:01 ` Marc Zyngier 2017-04-27 1:27 ` majun (Euler7) 2017-04-27 1:59 ` Hanjun Guo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox