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

Jae Hyun Yoo jae.hyun.yoo at linux.intel.com
Wed May 1 06:00:35 AEST 2019


On 4/29/2019 11:01 PM, Joel Stanley wrote:
> 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?

Issue is caused because VE_INTERRUPT_MODE_DETECT_WD or
VE_INTERRUPT_MODE_DETECT is cleared by a 500ms delayed work when the
handler calls aspeed_video_irq_res_change(). As Eddie said, it disables
interrupt through aspeed_video_off() but it doesn't mean that the
interrupt bit is cleared at that timing. Actually, the interrupt bit
is cleared by aspeed_video_resolution_work() 500ms after it gets the
interrupt request so this patch is going to clear the bits immediately.

> 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?

The watch dog reset issue happens when video mode change is occurred. I
use Ubuntu GUI mode and set screen saver to blank screen with 1 minute
expiration time. And then, open bmcweb page and navigate to Server
control -> KVM page. As long as a user stays in this page, KVM video
will be off by the screen saver and then it will be awaken by KVM
service so this sequence will be repeated infinitely. I usually see the
issue at least once if I put my system under this overnight stress test.

> --- 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;
>   }
> 

Looks good but we need to consider cases that needs going thru all
if statements other than cases call aspeed_video_irq_res_change().

I made a new patch like below. It has worked well so far. Will submit
v2 after making sufficient test using it.


diff --git a/drivers/media/platform/aspeed-video.c 
b/drivers/media/platform/aspeed-video.c
index 77c209a472ca..8096319733ee 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -488,6 +488,7 @@ static void aspeed_video_off(struct aspeed_video *video)

  	/* Disable interrupts */
  	aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
+	aspeed_video_write(video, VE_INTERRUPT_STATUS, 0xffffffff);

  	/* Turn off the relevant clocks */
  	clk_disable(video->vclk);
@@ -539,24 +540,23 @@ static void aspeed_video_irq_res_change(struct 
aspeed_video *video)
  static irqreturn_t aspeed_video_irq(int irq, void *arg)
  {
  	struct aspeed_video *video = arg;
-	u32 sts = aspeed_video_read(video, VE_INTERRUPT_STATUS);
+	u32 irq_received = aspeed_video_read(video, VE_INTERRUPT_STATUS);
+	u32 irq_handled = 0;

  	/*
  	 * Resolution changed or signal was lost; reset the engine and
  	 * re-initialize
  	 */
-	if (sts & VE_INTERRUPT_MODE_DETECT_WD) {
+	if (irq_received & VE_INTERRUPT_MODE_DETECT_WD) {
  		aspeed_video_irq_res_change(video);
  		return IRQ_HANDLED;
  	}

-	if (sts & VE_INTERRUPT_MODE_DETECT) {
+	if (irq_received & VE_INTERRUPT_MODE_DETECT) {
  		if (test_bit(VIDEO_RES_DETECT, &video->flags)) {
  			aspeed_video_update(video, VE_INTERRUPT_CTRL,
  					    VE_INTERRUPT_MODE_DETECT, 0);
-			aspeed_video_write(video, VE_INTERRUPT_STATUS,
-					   VE_INTERRUPT_MODE_DETECT);
-
+			irq_handled |= VE_INTERRUPT_MODE_DETECT;
  			set_bit(VIDEO_MODE_DETECT_DONE, &video->flags);
  			wake_up_interruptible_all(&video->wait);
  		} else {
@@ -569,7 +569,7 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
  		}
  	}

-	if (sts & VE_INTERRUPT_COMP_COMPLETE) {
+	if (irq_received & VE_INTERRUPT_COMP_COMPLETE) {
  		struct aspeed_video_buffer *buf;
  		u32 frame_size = aspeed_video_read(video,
  						   VE_OFFSET_COMP_STREAM);
@@ -599,14 +599,15 @@ static irqreturn_t aspeed_video_irq(int irq, void 
*arg)
  				    VE_SEQ_CTRL_TRIG_COMP, 0);
  		aspeed_video_update(video, VE_INTERRUPT_CTRL,
  				    VE_INTERRUPT_COMP_COMPLETE, 0);
-		aspeed_video_write(video, VE_INTERRUPT_STATUS,
-				   VE_INTERRUPT_COMP_COMPLETE);
-
+		irq_handled |= VE_INTERRUPT_COMP_COMPLETE;
  		if (test_bit(VIDEO_STREAMING, &video->flags) && buf)
  			aspeed_video_start_frame(video);
  	}

-	return IRQ_HANDLED;
+	if (irq_handled)
+		aspeed_video_write(video, VE_INTERRUPT_STATUS, irq_handled);
+
+	return irq_received == irq_handled ? IRQ_HANDLED : IRQ_NONE;
  }

  static void aspeed_video_check_and_set_polarity(struct aspeed_video 
*video)


More information about the openbmc mailing list