From: Valentine Barshak <vbarshak@ru•mvista.com>
To: Benjamin Herrenschmidt <benh@kernel•crashing.org>,
linuxppc-dev@ozlabs•org,
David Gibson <david@gibson•dropbear.id.au>
Subject: Re: [ PATCH ] PowerPC cascade UIC IRQ handler fix.
Date: Mon, 13 Aug 2007 16:31:03 +0400 [thread overview]
Message-ID: <46C04F07.3040909@ru.mvista.com> (raw)
In-Reply-To: <20070813010848.GC18526@localhost.localdomain>
David Gibson wrote:
> On Fri, Aug 03, 2007 at 04:23:46PM +1000, David Gibson wrote:
>> On Fri, Aug 03, 2007 at 02:57:05PM +1000, David Gibson wrote:
>>> On Fri, Aug 03, 2007 at 11:18:09AM +1000, Benjamin Herrenschmidt wrote:
>>>> On Thu, 2007-08-02 at 13:48 +1000, David Gibson wrote:
>>>>> On Mon, Jul 30, 2007 at 08:35:17PM +0400, Valentine Barshak wrote:
>>>>>> PPC44x cascade UIC irq handler fix.
>>>>>>
>>>>>> According to PPC44x UM, if an interrupt is configured as level-sensitive,
>>>>>> and a clear is attempted on the UIC_SR, the UIC_SR field is not
>>>>>> cleared if the incoming interrupt signal is at the asserted polarity.
>>>>>> This causes us to enter a cascade handler twice, since we first ack
>>>>>> parent UIC interrupt and ack child UIC one after that.
>>>>>> The patch checks child UIC msr value and returns IRQ_HANDLED
>>>>>> if there're no pending interrupts. Otherwise we get a kernel panic
>>>>>> with a "Fatal exception in interrupt" (illegal vector).
>>>>>> The patch also fixes status flags.
>>>>>>
>>>>>> Signed-off-by: Valentine Barshak <vbarshak@ru•mvista.com>
>>>>> Hrm... This doesn't seem like the right fix to me. Instead, I think
>>>>> the cascaded IRQ handler should ack the interrupt on the child first.
>>>>> I'm a little surprised it doesn't at the moment.
>>>> Well, we certainly do also need to make the code more solid vs.
>>>> spurrious interrupts.
>>> Actually that's true. The suggested patch is a good improvement for
>>> general robustness, but doesn't actually address the real problem.
>>>
>>>> The main thing is, if the cascade is a level interrupt, it should
>>>> probably use a smarter cascade handler that masks it, handle the child
>>>> interrupts, then unmasks it.
>>> We already have that, since I just use setup_irq() to set up a cascade
>>> handler, rather than a custom flow handler.
>>>
>>> The problem is that the standard handle_level_irq() flow handler acks
>>> before the ISR is called, whereas because of this UIC behaviour, we
>>> need to ack after the ISR has cleared the interrupt in the source.
>>> This is not specific to cascades, but is a problem for all
>>> level-triggered interrupts (i.e. basically everything).
>>>
>>> I think it means we must currently be getting a whole lot of spurious
>>> interrupts - will do some investigation in a moment.
>>>
>>> To fix this either we'll need a custom flow handler for UIC, or we can
>>> use the standard one, but clear the UIC_SR bits from the ->unmask()
>>> callback for level interrupts. I'm not entirely sure if the latter
>>> approach is safe - I *think* it is, but I could do with more
>>> convincing.
>> Ok, here's a patch which fixes up the flow handling on the UIC. It
>> needs some polish yet, but should be ok to test. Valentine, can you
>> test this on your setup, *without* your original proposed patch.
>> Eventually, for robustness, we'll want something like your original
>> patch as well for robustness, but in the meantime leaving it out
>> should tell us if my patch is actually having the intended effect.
>
> Valentine, it would be really helpful if you could test this on the
> problem you observed with the cascade interrupt. Any word on this?
>
Thanks David,
the patch works fine here (without the original one).
Don't think we really need a "fastcall" here on a powerpc though.
The original patch also fixes a minor issue with /proc/interrupts
(the the "if (trigger)" stuff).
Currently level-triggered interrupts are displayed as edge-triggered
ones and vice versa.
Thanks,
Valentine.
next prev parent reply other threads:[~2007-08-13 12:32 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-07-30 16:35 [ PATCH ] PowerPC cascade UIC IRQ handler fix Valentine Barshak
2007-08-02 3:48 ` David Gibson
2007-08-02 20:08 ` Josh Boyer
2007-08-03 11:09 ` Valentine Barshak
2007-08-03 1:18 ` Benjamin Herrenschmidt
2007-08-03 4:57 ` David Gibson
2007-08-03 6:23 ` David Gibson
2007-08-13 1:08 ` David Gibson
2007-08-13 12:31 ` Valentine Barshak [this message]
2007-08-14 1:35 ` David Gibson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=46C04F07.3040909@ru.mvista.com \
--to=vbarshak@ru$(echo .)mvista.com \
--cc=benh@kernel$(echo .)crashing.org \
--cc=david@gibson$(echo .)dropbear.id.au \
--cc=linuxppc-dev@ozlabs$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox