[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