[PATCH dev-5.0 4/4] media: aspeed: clear interrupt status flags immediately

[Joel Stanley]

On Tue, 30 Apr 2019 at 03:04, Joel Stanley <joel at jms.id.au> wrote:
>
> On Mon, 29 Apr 2019 at 23:38, Jae Hyun Yoo <jae.hyun.yoo at linux.intel.com> wrote:
> >
> > On 4/29/2019 3:29 PM, Eddie James wrote:
> > >
> > > On 4/25/19 5:20 PM, Jae Hyun Yoo wrote:
> > >> Interrupt status flags should be cleared immediately otherwise
> > >> interrupt handler will be called again and again until the flag
> > >> is cleared, but this driver clears some flags through a 500ms
> > >> delayed work which is a bad idea in interrupt handling, so this
> > >> commit makes the interrupt handler clear the status flags
> > >> immediately.
> > >>
> > >> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo at linux.intel.com>
> > >> ---
> > >>   drivers/media/platform/aspeed-video.c | 12 +++++++-----
> > >>   1 file changed, 7 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/drivers/media/platform/aspeed-video.c
> > >> b/drivers/media/platform/aspeed-video.c
> > >> index 77c209a472ca..e218f375b9f5 100644
> > >> --- a/drivers/media/platform/aspeed-video.c
> > >> +++ b/drivers/media/platform/aspeed-video.c
> > >> @@ -546,17 +546,18 @@ static irqreturn_t aspeed_video_irq(int irq,
> > >> void *arg)
> > >>        * re-initialize
> > >>        */
> > >>       if (sts & VE_INTERRUPT_MODE_DETECT_WD) {
> > >> +        aspeed_video_write(video, VE_INTERRUPT_STATUS,
> > >> +                   VE_INTERRUPT_MODE_DETECT_WD);
> > >
> > >
> > > aspeed_video_irq_res_change disables all IRQs and turns off the clocks.
> > > This shouldn't be necessary.
> >
> > In fact, this patch fixes a watch dog reset with printing out a stack
> > trace like below. This happens very rarely but it's critical because it
> > causes a BMC reset. In my experiments, interrupt flags should be cleared
> > even with the aspeed_video_write(video, VE_INTERRUPT_CTRL, 0) in
> > aspeed_video_off(), or we should add
> > apeed_video_write(video, VE_INTERRUPT_STATUS, 0xffffffff)
> > before the disabling interrupt. I think the way in this patch is better.
>
> In general, a driver should certainly be clearing (acking) the
> interrupt bits in the interrupt handler before returning.
>
> Jae, it would be interesting to know the value of VE_INTERRUPT_STATUS
> in the soft lockup situation.
>
> I took a closer look at this function, and it should probably not
> return IRQ_HANDLED at the bottom, as it may have fallen through all of
> the if statements and not have handled any interrupt. Jae, can you
> take a look at this and send another patch if you think that is
> correct.

Something like the patch below. It also made me wonder why you don't
return  from the flags = VIDEO_RES_DETECT state?

I don't have a way to test. Is there a simple command I can run on a
BMC to test, or do I need the entire stack?

--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -566,8 +566,8 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
                         * detection; reset the engine and re-initialize
                         */
                        aspeed_video_irq_res_change(video);
-                       return IRQ_HANDLED;
                }
+               return IRQ_HANDLED;
        }

        if (sts & VE_INTERRUPT_COMP_COMPLETE) {
@@ -606,9 +606,13 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)

                if (test_bit(VIDEO_STREAMING, &video->flags) && buf)
                        aspeed_video_start_frame(video);
+
+               return IRQ_HANDLED;
        }

-       return IRQ_HANDLED;
+       dev_dbg(video->dev, "unhandled states: %08x\n", sts);
+
+       return IRQ_NONE;
 }