Message-ID: <5AF29818.2010705@rock-chips.com>
Date:   Wed, 09 May 2018 14:41:28 +0800
From:   JeffyChen <jeffy.chen@...k-chips.com>
To:     Doug Anderson <dianders@...omium.org>
CC:     Brian Norris <briannorris@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Heiko Stübner <heiko@...ech.de>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-gpio@...r.kernel.org,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [RESEND PATCH] pinctrl: rockchip: Disable interrupt when changing
 it's capability

Hi Doug,

Thanks for your reply :)

On 05/09/2018 01:18 PM, Doug Anderson wrote:
>> >
>> >
>> >right, so we now have 2 cases: rockchip_irq_demux/ rockchip_irq_set_type
>> >
>> >if i'm right about the spurious irq(only happen when set rising for a high
>> >gpio, or set falling for a low gpio), then:
>> >
>> >1/ rockchip_irq_demux
>> >it's important to not losing irqs in this case, maybe we can
>> >
>> >a) ack irq
>> >b) update polarity for edge both irq
>> >
>> >we don't need to disable irq in b), since we would not hit the spurious irq
>> >cases here(always check gpio level to toggle it)
> Unless you have some sort of proof that rockchip_irq_demux(), I would
> take it as an example of something that works.  I remember stress
> testing the heck out of it.  Do you have some evidence that it's not
> working?  I think Brian was simply speculating that there might be a
> race here, but I don't think anyone has shown it have they?  Looking
> back at my notes, the thing I really made sure to stress was that we
> never got into a situation where we were losing an edge (AKA we were
> never configured to look for a falling edge when the line was already
> low).  I'm not sure I confirmed that we never got an extra interrupt.
>
> I'm at home right now and I can't add prints and poke at things, but
> as I understand it for edge interrupts the usual flow to make sure
> interrupts aren't ever lost is:
>
> 1. See that the interrupt went off
> 2. Ack it (clear it)
> 3. Call the interrupt handler
>
> ...presumably in this case rockchip_irq_demux() is called after step
> #2 (but I don't know if it's called before or after step #3).  If the
> line is toggling like crazy while the 3 steps are going on, it's OK if
> the interrupt handler is called more than once.  In general this could
> be considered expected.  That's why you Ack before handling--any extra
> edges that come in any time after the interrupt handler starts (even
> after the very first instruction) need to cause the interrupt handler
> to get called again.
>
> This is different than Brian's understanding since he seemed to think
> the Ack was happening later.  If you're in front of something where
> you can add printouts, maybe you can tell us.  I tried to look through
> the code and it was too twisted for me to be sure.

i think the current edge both irq flow for rk3399 would be:
gic_handle_irq //irq-gic-v3.c
   handle_domain_irq
     rockchip_irq_demux //pinctrl-rockchip.c
       toggle polarity
       generic_handle_irq
         handle_edge_irq //kernel/irq
           irq_ack
           handle_irq_event
             action->handler

so i think the race might actually exist (maybe we can add some delay 
after toggle polarity to confirm)


BTW, checking other drivers, there're quite a few using this kind of 
toggle edge for edge both, and they go different ways:

1/ toggle it in ack():
mediatek/pinctrl-mtk-common.c
gpio-ingenic.c
gpio-ep93xx.c

2/ toggle it before handle_irq:
pinctrl-rockchip.c
pinctrl-armada-37xx.c
gpio-ath79.c
gpio-mxs.c
gpio-omap.c
gpio-mvebu.c

3/ toggle it after handle_irq:
gpio-dwapb.c
gpio-pmic-eic-sprd.c


would it make sense to support this kind of emulate edge both irq in 
some core codes?

>
>
>> >2/ rockchip_irq_set_type
>> >it's important to not having spurious irqs
>> >
>> >so we can disable irq during changing polarity only in these case:
>> >((rising && gpio is heigh) || (falling && gpio is low))
>> >
>> >i'm still confirming the spurious irq with IC guys.
> Hmmm, thinking about all this more, I'm curious how callers expect
> this to work.  Certainly things are undefined if you have the
> following situation:
>
> Start: rising edge trigger, line is actually high
> Request: change to falling edge trigger
> Line falls during the request
>
> In that case it's OK to throw the interrupt away because it can be
> argued that the line could have fallen before the request actually
> took place.  ...but it seems like there could be situations where the
> user wouldn't expect interrupts to be thrown away by a call to
> irq_set_type().  In other words:
>
> Start: rising edge trigger, line is actually high
> Request: change to falling edge trigger
> Line falls, rises, and falls during the request
>
> ...in that case you'd expect that some sort of interrupt would have
> gone off and not be thrown away.  No matter what instant in time the
> request actually took place it should have caught an edge, right?
>
>
> Said another way: As a client of irq_set_type() I'd expect it to not
> throw away interrupts, but I'd expect that the change in type would be
> atomic.  That is: if the interrupt came in before the type change in
> type applied then it should trigger with the old rules.  If the
> interrupt came in after the type change then it should trigger with
> the new rules.
>
>
> I would be tempted to confirm your testing and just clear the spurious
> interrupts that you're aware of.  AKA: if there's no interrupt pending
> and you change the type to "IRQ_TYPE_EDGE_RISING" or
> "IRQ_TYPE_EDGE_FALLING" then you should clear the interrupt.  It's
> still racy, but I guess it's the best you can do unless IC guys come
> up with something better.
>

hmmm, right, clear the spurious irq seems to be a better way, will try 
to do it in the next version.
>
>
> Anyway, it's past my bedtime.  Hopefully some of the above made sense.
> I'm sure you'll tell me if it didn't or if I said something
> stupid/wrong.:-P
>
> -Doug
>
>
>

Powered by blists - more mailing lists