Porting a custom interrupt handler from kernel 2.6.25 to 3.16.45

Porting a custom interrupt handler from kernel 2.6.25 to 3.16.45

[Mircea Ciocan]

Hello everybody, I'm new to the intricacies of the ARM platform
interrupt handling and I have to port a custom interrupt handler from
2.6.25 to 3.16.45.
The current situation is like this:
The timer IRQ (only one active for the moment) triggers, and the
interrupt routine is executed only once correctly.
The next time it triggers, or maybe at the end of the generic arm
interrupt handler, I got an Oops because the stack or PC seems to be
smashed and the CPU goes haywire.
The strange thing is that if in the "normal" irq handler of the timer
I don't clear the interrupt bit, the kernel loops endlessly in the
interrupt routine but it doesn't crash at all.

In the original 2.6 arch/arm/kernel/entry-armv.S the code is inserted like this:
/*  Interrupt handling.  Preserves r7, r8, r9 */
        .macro  irq_handler
#ifdef CONFIG_ARCH_CUSTOM
        irq_handler_custom r0, r6, r5, r1, lr, r2
#else
        get_irqnr_preamble r5, lr
1:      get_irqnr_and_base r0, r6, r5, lr
        movne   r1, sp
        @
        @ routine called with r0 = irq number, r1 = struct pt_regs *
        @
        adrne   lr, 1b
        bne     asm_do_IRQ
#endif
[...snip...]
        .endm

In the 3.16 new kernel the code, that doesn't work, looks like this:
/* Interrupt handling.  */

        .macro  irq_handler
#ifdef CONFIG_ARCH_CUSTOM
        irq_handler_spc300 r0, r6, r5, r1, lr, r2
#else
#ifdef CONFIG_MULTI_IRQ_HANDLER
        ldr     r1, =handle_arch_irq
        mov     r0, sp
        adr     lr, BSYM(9997f)
        ldr     pc, [r1]
#else
        arch_irq_handler_default
#endif
#endif
9997:
        .endm

I have seen that around kernel 3.0 there were some register shuffling
in the irq handler ( commit b059bdc ("ARM: entry: rejig register
allocation in exception entry handlers", 2011-06-25) )

So I will kindly ask for your assistance to point me on the right
track to have this issue solved, do I just need to shuffle the
registers or other more important changes are need ?
At least I would like to know what registers have a special meaning
and must be preserved in the latest kernel.
Also if it's a Fine Manual that I can read about the architecture of
the interrupt handling on the ARM platform, explaining the roles and
constrains on the registers used I will appreciate any pointers.

 Best regards,
 Mircea

Porting a custom interrupt handler from kernel 2.6.25 to 3.16.45

[Andrew Lunn]

On Thu, Jul 13, 2017 at 10:23:41AM +0200, Mircea Ciocan wrote:
> Hello everybody, I'm new to the intricacies of the ARM platform
> interrupt handling and I have to port a custom interrupt handler from
> 2.6.25 to 3.16.45.

Hi Mircea

Could you please take a step back and explain why you need a custom
interrupt handler.

	  Andrew

Porting a custom interrupt handler from kernel 2.6.25 to 3.16.45

[Mircea Ciocan]

On Thu, Jul 13, 2017 at 6:06 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Thu, Jul 13, 2017 at 10:23:41AM +0200, Mircea Ciocan wrote:
>> Hello everybody, I'm new to the intricacies of the ARM platform
>> interrupt handling and I have to port a custom interrupt handler from
>> 2.6.25 to 3.16.45.
>
> Hi Mircea
>
> Could you please take a step back and explain why you need a custom
> interrupt handler.
>
>           Andrew

Hi Andrew, sure I can explain, or at least offer the original
explanation of the long lost developer:
The ARM core has a custom interrupt controller and a DSP co-processor
that does some funny (read strange & abnormal) stuff.
And I can say that the "standard" kernel infrastructure is not overly
documented, I'm still struggle to understand the full purpose of
the *get_irqnr_preamble* and *get_irqnr_and_base* macros.
I'm trying to use the existing infrastructure, to boot at least
without the co-processor, but without documentation is difficult.

 Best regards, Mircea

Porting a custom interrupt handler from kernel 2.6.25 to 3.16.45

[Russell King - ARM Linux]

On Thu, Jul 13, 2017 at 06:32:29PM +0200, Mircea Ciocan wrote:
> I'm still struggle to understand the full purpose of
> the *get_irqnr_preamble* and *get_irqnr_and_base* macros.

get_irqnr_preamble does whatever you need that's common to handling a
set of interrupts read from the hardware.  It can set the two registers
to anything it likes.  Commonly, this is used to get the address of the
top level interrupt controller.  These two registers are passed to the
get_irqnr_and_base and test_for_ipi macros unmodified.

get_irqnr_and_base gets the interrupt number.  This must clear the Z
flag if an interrupt is pending, or set it if there's no interrupt.

test_for_ipi takes all the registers that were given to get_irqnr_and_base
and determines whether an IPI happened and which IPI, otherwise it
has the same behaviour as get_irqnr_and_base.

So it's not complicated - get_irqnr_preamble is basically the common
setup bits and get_irqnr_and_base is called repeatedly in a loop until
it indicates that there are no further pending interrupts.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Porting a custom interrupt handler from kernel 2.6.25 to 3.16.45

[Andrew Lunn]

On Thu, Jul 13, 2017 at 06:32:29PM +0200, Mircea Ciocan wrote:
> On Thu, Jul 13, 2017 at 6:06 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> > On Thu, Jul 13, 2017 at 10:23:41AM +0200, Mircea Ciocan wrote:
> >> Hello everybody, I'm new to the intricacies of the ARM platform
> >> interrupt handling and I have to port a custom interrupt handler from
> >> 2.6.25 to 3.16.45.
> >
> > Hi Mircea
> >
> > Could you please take a step back and explain why you need a custom
> > interrupt handler.
> >
> >           Andrew
> 
> Hi Andrew, sure I can explain, or at least offer the original
> explanation of the long lost developer:
> The ARM core has a custom interrupt controller and a DSP co-processor
> that does some funny (read strange & abnormal) stuff.

Hi Mircea

How is this interrupt controller connected? Is it chained off another
interrupt controller? Or is it a root interrupt controller?

	  Andrew

Porting a custom interrupt handler from kernel 2.6.25 to 3.16.45

[Mircea Ciocan]

On Thu, Jul 13, 2017 at 7:35 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Thu, Jul 13, 2017 at 06:32:29PM +0200, Mircea Ciocan wrote:
>> I'm still struggle to understand the full purpose of
>> the *get_irqnr_preamble* and *get_irqnr_and_base* macros.
>
> get_irqnr_preamble does whatever you need that's common to handling a
> set of interrupts read from the hardware.  It can set the two registers
> to anything it likes.  Commonly, this is used to get the address of the
> top level interrupt controller.  These two registers are passed to the
> get_irqnr_and_base and test_for_ipi macros unmodified.
>
> get_irqnr_and_base gets the interrupt number.  This must clear the Z
> flag if an interrupt is pending, or set it if there's no interrupt.
>
> test_for_ipi takes all the registers that were given to get_irqnr_and_base
> and determines whether an IPI happened and which IPI, otherwise it
> has the same behaviour as get_irqnr_and_base.
>
> So it's not complicated - get_irqnr_preamble is basically the common
> setup bits and get_irqnr_and_base is called repeatedly in a loop until
> it indicates that there are no further pending interrupts.
>
> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.

Hi Russel and thanks for the answer, first thing first, write a book,
seriously, write a book (or minimally a verbose FAQ ;) describing the
whole Linux kernel ARM architecture, including the deep details,
something focused on the low level details for avoiding creating a
class of "specialists" that were following the topic for years and of
course considers this stuff "elementary", it may be, but the barriers
for understanding it are pretty high IMHO. and sadly this is why we
have a lot of ARM SoCs with extraordinary obsolete kernels, because
everybody is kind of afraid to try to update. If it's a book I will
definitely buy it.

Now that the rant part is over, let me summarize what I understand
form my searches and your explanations, and kindly please confirm if
it's correct:

- *get_irqnr_preamble* - it's mostly used to get the top level
interrupt controller address of if it's a funky controller, to make it
ready for *get_irqnr_and_base* and  *test_for_ipi* that gets those
registers as parameters.
It is used only once.

- *get_irqnr_and_base* gets it's \base register already set by
*get_irqnr_preamble* and does it have to return the \irqnr or it
receives the \irqnr already set by *get_irqnr_preamble* ?
At the end of the macro, is any of the registers value \irqnr,
\irqstat, \base, \tmp of any significance or strictly only the zero
flag matters ?
In the original custom function I have this gem of a comment, where
the original developer said:

"... snip...
1001:
        /* WARNING: These lines override the default behavior, */
        /* which is to loop back at the start of the macro after the handler */

        /* set r1 to registers address */
        movne   r1, sp

        /* set label 2 as return address */
        adrne   lr, 2f
    .endm
"
I believe in the current framework this is an incorrect behavior, is
there any possible situation where this can offer some advantage ?

In the end many thanks for the support and hopefully in the future I
will be able to contribute myself something back to the community.

 Best regards,
Mircea

Porting a custom interrupt handler from kernel 2.6.25 to 3.16.45

[Russell King - ARM Linux]

On Fri, Jul 14, 2017 at 09:58:39AM +0200, Mircea Ciocan wrote:
> On Thu, Jul 13, 2017 at 7:35 PM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > On Thu, Jul 13, 2017 at 06:32:29PM +0200, Mircea Ciocan wrote:
> >> I'm still struggle to understand the full purpose of
> >> the *get_irqnr_preamble* and *get_irqnr_and_base* macros.
> >
> > get_irqnr_preamble does whatever you need that's common to handling a
> > set of interrupts read from the hardware.  It can set the two registers
> > to anything it likes.  Commonly, this is used to get the address of the
> > top level interrupt controller.  These two registers are passed to the
> > get_irqnr_and_base and test_for_ipi macros unmodified.
> >
> > get_irqnr_and_base gets the interrupt number.  This must clear the Z
> > flag if an interrupt is pending, or set it if there's no interrupt.
> >
> > test_for_ipi takes all the registers that were given to get_irqnr_and_base
> > and determines whether an IPI happened and which IPI, otherwise it
> > has the same behaviour as get_irqnr_and_base.
> >
> > So it's not complicated - get_irqnr_preamble is basically the common
> > setup bits and get_irqnr_and_base is called repeatedly in a loop until
> > it indicates that there are no further pending interrupts.
> >
> > --
> > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> > according to speedtest.net.
> 
> Hi Russel and thanks for the answer, first thing first, write a book,

First off, please spell my name correctly.  People tend to get upset
when their names are mis-spelt (eg, Diana's get upset if you call
them Diane and Diane's get upset if you call them Diana.)

> seriously, write a book (or minimally a verbose FAQ ;) describing the
> whole Linux kernel ARM architecture, including the deep details,
> something focused on the low level details for avoiding creating a
> class of "specialists" that were following the topic for years and of
> course considers this stuff "elementary", it may be, but the barriers
> for understanding it are pretty high IMHO. and sadly this is why we
> have a lot of ARM SoCs with extraordinary obsolete kernels, because
> everybody is kind of afraid to try to update. If it's a book I will
> definitely buy it.

This particular detail that you've asked about hasn't changed for
about 20 years.  No, I'm not going to write a book, that would take
way too long, and by the time it's complete, it'll be obsolete.

As for a FAQ, most people over the last 20 years have been able to
grasp these two macros, you're the first one to really ask in this
depth about them, so it's hardly a "frequent" question.  Most people
(as is common with much of the Linux code base) read the code where
things are used and existing implementations to augment their
understanding.

That's something even I have to do all the time, and from time to
time I have asked for better documentation, but that's never been
forthcoming.  So it's really a case of "read the code."

Sorry if that offends, but that's the way Linux is.

> Now that the rant part is over, let me summarize what I understand
> form my searches and your explanations, and kindly please confirm if
> it's correct:
> 
> - *get_irqnr_preamble* - it's mostly used to get the top level
> interrupt controller address of if it's a funky controller, to make it
> ready for *get_irqnr_and_base* and  *test_for_ipi* that gets those
> registers as parameters.
> It is used only once.

Correct.

> - *get_irqnr_and_base* gets it's \base register already set by
> *get_irqnr_preamble* and does it have to return the \irqnr or it
> receives the \irqnr already set by *get_irqnr_preamble* ?

Here's how it's used:

        get_irqnr_preamble r6, lr
1:      get_irqnr_and_base r0, r2, r6, lr
        movne   r1, sp
        @
        @ routine called with r0 = irq number, r1 = struct pt_regs *
        @
        badrne  lr, 1b
        bne     asm_do_IRQ

where r0 is "irqnr".  These two questions will help answer yours:

1. Is r0 passed to get_irqnr_preamble?
2. What would it mean if get_irqnr_and_base did not set r0?

Moreover, if the interrupt number _were_ to be only returned by
get_irqnr_preamble, then how could we ever hope to service more than
one interrupt number?

I said in my initial reply "get_irqnr_and_base gets the interrupt
number" which I think also answers your question, and the macro name
also reflects its operation.

> At the end of the macro, is any of the registers value \irqnr,
> \irqstat, \base, \tmp of any significance or strictly only the zero
> flag matters ?

>From what we can see from the above quoted code, asm_do_IRQ is called
with r0 and r1 set.  As the code following the macro sets r1 already,
r0 needs to be set, and r0 is "irqnr".

Some implementations used to also set "base" where they had multiple
interrupt controllers (which is why the macro is named the way it is)
and didn't use get_irqnr_preamble to set the base.

What's required in these two macros is really up to the implementation,
there's no hard and fast rules apart from not using registers that
aren't passed to the macro as arguments.

> In the original custom function I have this gem of a comment, where
> the original developer said:
> 
> "... snip...
> 1001:
>         /* WARNING: These lines override the default behavior, */
>         /* which is to loop back at the start of the macro after the handler */
> 
>         /* set r1 to registers address */
>         movne   r1, sp
> 
>         /* set label 2 as return address */
>         adrne   lr, 2f
>     .endm
> "

That looks like a horrid hack - it's jumping out of the macro, to some
other location, and the only way to know that is to look at the code
in the kernel version that it was created for.  Such hacky code can't
be expected to work from one kernel version to the next, so it's not
surprising that it doesn't work with modern kernels.

If it was supposed to be using the "2" label, then it would be passed
into the macro, to allow the code that calls the macro to be modified
without affecting the implementations.

In general, macros should never jump outside of themselves - that
creates unclean code that is context sensitive, destroys readability,
and creates maintenance problems.

We have the same rule for C code: C preprocessor macros are not allowed
to use "return" because that destroys the readability of the code.

> I believe in the current framework this is an incorrect behavior, is
> there any possible situation where this can offer some advantage ?

I couldn't say, because:
(1) I don't know why it would need such a hack.
(2) I don't know where it is jumping to.
so
(3) I don't know what this code is doing.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Porting a custom interrupt handler from kernel 2.6.25 to 3.16.45

[Mircea Ciocan]

On Fri, Jul 14, 2017 at 10:48 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
[snip..]
>> Hi  and thanks for the answer, first thing first, write a book,
>
> First off, please spell my name correctly.  People tend to get upset
> when their names are mis-spelt (eg, Diana's get upset if you call
> them Diane and Diane's get upset if you call them Diana.)
>
Russell, I sincerely apologize for spelling your name incorrectly, I
should have proofread better my message.
It won't happen again.

[more snip...]
>
> This particular detail that you've asked about hasn't changed for
> about 20 years.  No, I'm not going to write a book, that would take
> way too long, and by the time it's complete, it'll be obsolete.
>
This is really a pity :(

> As for a FAQ, most people over the last 20 years have been able to
> grasp these two macros, you're the first one to really ask in this
> depth about them, so it's hardly a "frequent" question.  Most people
> (as is common with much of the Linux code base) read the code where
> things are used and existing implementations to augment their
> understanding.
>
> That's something even I have to do all the time, and from time to
> time I have asked for better documentation, but that's never been
> forthcoming.  So it's really a case of "read the code."
>
> Sorry if that offends, but that's the way Linux is.
Now offense taken, reading the code do lead to enlightenment (after a
while, sadly) but for stable architecture some comments or a little
text in the doc directory will be nice to have.

>
>> Now that the rant part is over, let me summarize what I understand
>> form my searches and your explanations, and kindly please confirm if
>> it's correct:
>>
>> - *get_irqnr_preamble* - it's mostly used to get the top level
>> interrupt controller address of if it's a funky controller, to make it
>> ready for *get_irqnr_and_base* and  *test_for_ipi* that gets those
>> registers as parameters.
>> It is used only once.
>
> Correct.
>
>> - *get_irqnr_and_base* gets it's \base register already set by
>> *get_irqnr_preamble* and does it have to return the \irqnr or it
>> receives the \irqnr already set by *get_irqnr_preamble* ?
>
> Here's how it's used:
>
>         get_irqnr_preamble r6, lr
> 1:      get_irqnr_and_base r0, r2, r6, lr
>         movne   r1, sp
>         @
>         @ routine called with r0 = irq number, r1 = struct pt_regs *
>         @
>         badrne  lr, 1b
>         bne     asm_do_IRQ
>
> where r0 is "irqnr".  These two questions will help answer yours:
>
> 1. Is r0 passed to get_irqnr_preamble?
> 2. What would it mean if get_irqnr_and_base did not set r0?
>
> Moreover, if the interrupt number _were_ to be only returned by
> get_irqnr_preamble, then how could we ever hope to service more than
> one interrupt number?
>
> I said in my initial reply "get_irqnr_and_base gets the interrupt
> number" which I think also answers your question, and the macro name
> also reflects its operation.
>
>> At the end of the macro, is any of the registers value \irqnr,
>> \irqstat, \base, \tmp of any significance or strictly only the zero
>> flag matters ?
>
> From what we can see from the above quoted code, asm_do_IRQ is called
> with r0 and r1 set.  As the code following the macro sets r1 already,
> r0 needs to be set, and r0 is "irqnr".
>
> Some implementations used to also set "base" where they had multiple
> interrupt controllers (which is why the macro is named the way it is)
> and didn't use get_irqnr_preamble to set the base.
>
> What's required in these two macros is really up to the implementation,
> there's no hard and fast rules apart from not using registers that
> aren't passed to the macro as arguments.
>
>> In the original custom function I have this gem of a comment, where
>> the original developer said:
>>
>> "... snip...
>> 1001:
>>         /* WARNING: These lines override the default behavior, */
>>         /* which is to loop back at the start of the macro after the handler */
>>
>>         /* set r1 to registers address */
>>         movne   r1, sp
>>
>>         /* set label 2 as return address */
>>         adrne   lr, 2f
>>     .endm
>> "
>
> That looks like a horrid hack - it's jumping out of the macro, to some
> other location, and the only way to know that is to look at the code
> in the kernel version that it was created for.  Such hacky code can't
> be expected to work from one kernel version to the next, so it's not
> surprising that it doesn't work with modern kernels.
>
> If it was supposed to be using the "2" label, then it would be passed
> into the macro, to allow the code that calls the macro to be modified
> without affecting the implementations.
>
> In general, macros should never jump outside of themselves - that
> creates unclean code that is context sensitive, destroys readability,
> and creates maintenance problems.
>
> We have the same rule for C code: C preprocessor macros are not allowed
> to use "return" because that destroys the readability of the code.
>
>> I believe in the current framework this is an incorrect behavior, is
>> there any possible situation where this can offer some advantage ?
>
> I couldn't say, because:
> (1) I don't know why it would need such a hack.
> (2) I don't know where it is jumping to.
> so
> (3) I don't know what this code is doing.
>
In the end Russell, I thank you once more for your kind help and I
will try to re-factor the existing code to fit into the framework, the
existing stuff was looking strange to me as well, but lacking an
overview of the architecture lead me to ask this noob questions.

 Have a nice week-end and best regards,
 Mircea

Porting a custom interrupt handler from kernel 2.6.25 to 3.16.45

[Andrew Lunn]

> In the end Russell, I thank you once more for your kind help and I
> will try to re-factor the existing code to fit into the framework, the
> existing stuff was looking strange to me as well, but lacking an
> overview of the architecture lead me to ask this noob questions.

Hi Mircea

Things are probably a lot easier if your custom interrupt controller
is daisy-chained off a standard interrupt controller. If it is, you
don't need to touch this low level code. No hacks are needed, and you
probably get portable code.

      Andrew