[PATCH dev-5.0 0/3] Improve stability of Aspeed video engine driver

[PATCH dev-5.0 0/3] Improve stability of Aspeed video engine driver

[Jae Hyun Yoo]

This patch series improves stability of Aspeed video engine driver by fixing
clock control and irq handling logic in the driver.

Jae Hyun Yoo (3):
  media: platform: aspeed: fix a kernel warning on clk control
  media: platform: aspeed: refine clock control logic
  media: platform: aspeed: change irq to threaded irq

 drivers/media/platform/aspeed-video.c | 75 ++++++++++++++++++++++-----
 1 file changed, 61 insertions(+), 14 deletions(-)

-- 
2.21.0

[PATCH dev-5.0 1/3] media: platform: aspeed: fix a kernel warning on clk control

[Jae Hyun Yoo]

Video engine clock control functions in the Aspeed video engine
driver are being called from multiple context without any
protection so video clocks can be double disabled and eventually
it causes a kernel warning with stack dump printing out like below:

[  515.540498] ------------[ cut here ]------------
[  515.545174] WARNING: CPU: 0 PID: 1310 at drivers/clk/clk.c:684 clk_core_unprepare+0x13c/0x170
[  515.553806] vclk-gate already unprepared
[  515.557841] CPU: 0 PID: 1310 Comm: obmc-ikvm Tainted: G        W         5.0.6-df66fbc97853fbba90a0bfa44de32f3d5f7602b4 #1
[  515.568973] Hardware name: Generic DT based system
[  515.573777] Backtrace:
[  515.576272] [<80107cdc>] (dump_backtrace) from [<80107f10>] (show_stack+0x20/0x24)
[  515.583930]  r7:803a5614 r6:00000009 r5:00000000 r4:9d88fe1c
[  515.589712] [<80107ef0>] (show_stack) from [<80690184>] (dump_stack+0x20/0x28)
[  515.597053] [<80690164>] (dump_stack) from [<80116044>] (__warn.part.3+0xb4/0xdc)
[  515.604557] [<80115f90>] (__warn.part.3) from [<801160d8>] (warn_slowpath_fmt+0x6c/0x90)
[  515.612734]  r6:000002ac r5:8080befc r4:80a07008
[  515.617463] [<80116070>] (warn_slowpath_fmt) from [<803a5614>] (clk_core_unprepare+0x13c/0x170)
[  515.626167]  r3:8080cdf4 r2:8080bfc0
[  515.629834]  r7:98d682a8 r6:9d8a9200 r5:9e5151a0 r4:97abd620
[  515.635530] [<803a54d8>] (clk_core_unprepare) from [<803a76a4>] (clk_unprepare+0x34/0x3c)
[  515.643812]  r5:9e5151a0 r4:97abd620
[  515.647529] [<803a7670>] (clk_unprepare) from [<804f36ec>] (aspeed_video_off+0x38/0x50)
[  515.655539]  r5:9e5151a0 r4:9e504000
[  515.659242] [<804f36b4>] (aspeed_video_off) from [<804f4358>] (aspeed_video_release+0x90/0x114)
[  515.668036]  r5:9e5044b0 r4:9e504000
[  515.671643] [<804f42c8>] (aspeed_video_release) from [<804d302c>] (v4l2_release+0xd4/0xe8)
[  515.679999]  r7:98d682a8 r6:9d087810 r5:9d8a9200 r4:9e504318
[  515.685695] [<804d2f58>] (v4l2_release) from [<80236454>] (__fput+0x98/0x1c4)
[  515.692914]  r5:9e51b608 r4:9d8a9200
[  515.696597] [<802363bc>] (__fput) from [<802365e8>] (____fput+0x18/0x1c)
[  515.703315]  r9:80a0700c r8:801011e4 r7:00000000 r6:80a64b9c r5:9d8e35a0 r4:9d8e38dc
[  515.711167] [<802365d0>] (____fput) from [<80131ca4>] (task_work_run+0x7c/0xa0)
[  515.718596] [<80131c28>] (task_work_run) from [<80106884>] (do_work_pending+0x4a8/0x578)
[  515.726777]  r7:801011e4 r6:80a07008 r5:9d88ffb0 r4:ffffe000
[  515.732466] [<801063dc>] (do_work_pending) from [<8010106c>] (slow_work_pending+0xc/0x20)
[  515.740727] Exception stack(0x9d88ffb0 to 0x9d88fff8)
[  515.745840] ffa0:                                     00000000 76f18094 00000000 00000000
[  515.754122] ffc0: 00000007 00176778 7eda4c20 00000006 00000000 00000000 48e20fa4 00000000
[  515.762386] ffe0: 00000002 7eda4b08 00000000 48f91efc 80000010 00000007
[  515.769097]  r10:00000000 r9:9d88e000 r8:801011e4 r7:00000006 r6:7eda4c20 r5:00176778
[  515.777006]  r4:00000007
[  515.779558] ---[ end trace 12c04aadef8afbbb ]---
[  515.784176] ------------[ cut here ]------------
[  515.788817] WARNING: CPU: 0 PID: 1310 at drivers/clk/clk.c:825 clk_core_disable+0x18c/0x204
[  515.797161] eclk-gate already disabled
[  515.800916] CPU: 0 PID: 1310 Comm: obmc-ikvm Tainted: G        W         5.0.6-df66fbc97853fbba90a0bfa44de32f3d5f7602b4 #1
[  515.811945] Hardware name: Generic DT based system
[  515.816730] Backtrace:
[  515.819210] [<80107cdc>] (dump_backtrace) from [<80107f10>] (show_stack+0x20/0x24)
[  515.826782]  r7:803a5900 r6:00000009 r5:00000000 r4:9d88fe04
[  515.832454] [<80107ef0>] (show_stack) from [<80690184>] (dump_stack+0x20/0x28)
[  515.839687] [<80690164>] (dump_stack) from [<80116044>] (__warn.part.3+0xb4/0xdc)
[  515.847170] [<80115f90>] (__warn.part.3) from [<801160d8>] (warn_slowpath_fmt+0x6c/0x90)
[  515.855247]  r6:00000339 r5:8080befc r4:80a07008
[  515.859868] [<80116070>] (warn_slowpath_fmt) from [<803a5900>] (clk_core_disable+0x18c/0x204)
[  515.868385]  r3:8080cdd0 r2:8080c00c
[  515.871957]  r7:98d682a8 r6:9d8a9200 r5:97abd560 r4:97abd560
[  515.877615] [<803a5774>] (clk_core_disable) from [<803a59a0>] (clk_core_disable_lock+0x28/0x34)
[  515.886301]  r7:98d682a8 r6:9d8a9200 r5:97abd560 r4:a0000013
[  515.891960] [<803a5978>] (clk_core_disable_lock) from [<803a7714>] (clk_disable+0x2c/0x30)
[  515.900216]  r5:9e5151a0 r4:9e515f60
[  515.903816] [<803a76e8>] (clk_disable) from [<804f36f8>] (aspeed_video_off+0x44/0x50)
[  515.911656] [<804f36b4>] (aspeed_video_off) from [<804f4358>] (aspeed_video_release+0x90/0x114)
[  515.920341]  r5:9e5044b0 r4:9e504000
[  515.923921] [<804f42c8>] (aspeed_video_release) from [<804d302c>] (v4l2_release+0xd4/0xe8)
[  515.932184]  r7:98d682a8 r6:9d087810 r5:9d8a9200 r4:9e504318
[  515.937851] [<804d2f58>] (v4l2_release) from [<80236454>] (__fput+0x98/0x1c4)
[  515.944980]  r5:9e51b608 r4:9d8a9200
[  515.948559] [<802363bc>] (__fput) from [<802365e8>] (____fput+0x18/0x1c)
[  515.955257]  r9:80a0700c r8:801011e4 r7:00000000 r6:80a64b9c r5:9d8e35a0 r4:9d8e38dc
[  515.963008] [<802365d0>] (____fput) from [<80131ca4>] (task_work_run+0x7c/0xa0)
[  515.970333] [<80131c28>] (task_work_run) from [<80106884>] (do_work_pending+0x4a8/0x578)
[  515.978421]  r7:801011e4 r6:80a07008 r5:9d88ffb0 r4:ffffe000
[  515.984086] [<801063dc>] (do_work_pending) from [<8010106c>] (slow_work_pending+0xc/0x20)
[  515.992247] Exception stack(0x9d88ffb0 to 0x9d88fff8)
[  515.997296] ffa0:                                     00000000 76f18094 00000000 00000000
[  516.005473] ffc0: 00000007 00176778 7eda4c20 00000006 00000000 00000000 48e20fa4 00000000
[  516.013642] ffe0: 00000002 7eda4b08 00000000 48f91efc 80000010 00000007
[  516.020257]  r10:00000000 r9:9d88e000 r8:801011e4 r7:00000006 r6:7eda4c20 r5:00176778
[  516.028072]  r4:00000007
[  516.030606] ---[ end trace 12c04aadef8afbbc ]---

To prevent this issue, this commit adds spinlock protection and
clock status checking logic into the Aspeed video engine driver.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
 drivers/media/platform/aspeed-video.c | 32 ++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index 8144fe36ad48..e70be8fdbde5 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -187,6 +187,7 @@ enum {
 	VIDEO_STREAMING,
 	VIDEO_FRAME_INPRG,
 	VIDEO_STOPPED,
+	VIDEO_CLOCKS_ON,
 };
 
 struct aspeed_video_addr {
@@ -483,19 +484,29 @@ static void aspeed_video_enable_mode_detect(struct aspeed_video *video)
 
 static void aspeed_video_off(struct aspeed_video *video)
 {
+	if (!test_bit(VIDEO_CLOCKS_ON, &video->flags))
+		return;
+
 	/* Disable interrupts */
 	aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
 
 	/* Turn off the relevant clocks */
 	clk_disable_unprepare(video->vclk);
 	clk_disable_unprepare(video->eclk);
+
+	clear_bit(VIDEO_CLOCKS_ON, &video->flags);
 }
 
 static void aspeed_video_on(struct aspeed_video *video)
 {
+	if (test_bit(VIDEO_CLOCKS_ON, &video->flags))
+		return;
+
 	/* Turn on the relevant clocks */
 	clk_prepare_enable(video->eclk);
 	clk_prepare_enable(video->vclk);
+
+	set_bit(VIDEO_CLOCKS_ON, &video->flags);
 }
 
 static void aspeed_video_bufs_done(struct aspeed_video *video,
@@ -513,12 +524,14 @@ static void aspeed_video_bufs_done(struct aspeed_video *video,
 
 static void aspeed_video_irq_res_change(struct aspeed_video *video)
 {
+	spin_lock(&video->lock);
 	dev_dbg(video->dev, "Resolution changed; resetting\n");
 
 	set_bit(VIDEO_RES_CHANGE, &video->flags);
 	clear_bit(VIDEO_FRAME_INPRG, &video->flags);
 
 	aspeed_video_off(video);
+	spin_unlock(&video->lock);
 	aspeed_video_bufs_done(video, VB2_BUF_STATE_ERROR);
 
 	schedule_delayed_work(&video->res_work, RESOLUTION_CHANGE_DELAY);
@@ -938,9 +951,13 @@ static void aspeed_video_init_regs(struct aspeed_video *video)
 
 static void aspeed_video_start(struct aspeed_video *video)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&video->lock, flags);
 	aspeed_video_on(video);
 
 	aspeed_video_init_regs(video);
+	spin_unlock_irqrestore(&video->lock, flags);
 
 	/* Resolution set to 640x480 if no signal found */
 	aspeed_video_get_resolution(video);
@@ -956,6 +973,9 @@ static void aspeed_video_start(struct aspeed_video *video)
 
 static void aspeed_video_stop(struct aspeed_video *video)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&video->lock, flags);
 	set_bit(VIDEO_STOPPED, &video->flags);
 	cancel_delayed_work_sync(&video->res_work);
 
@@ -969,6 +989,7 @@ static void aspeed_video_stop(struct aspeed_video *video)
 
 	video->v4l2_input_status = V4L2_IN_ST_NO_SIGNAL;
 	video->flags = 0;
+	spin_unlock_irqrestore(&video->lock, flags);
 }
 
 static int aspeed_video_querycap(struct file *file, void *fh,
@@ -1306,16 +1327,21 @@ static void aspeed_video_resolution_work(struct work_struct *work)
 	struct delayed_work *dwork = to_delayed_work(work);
 	struct aspeed_video *video = container_of(dwork, struct aspeed_video,
 						  res_work);
-	u32 input_status = video->v4l2_input_status;
+	unsigned long flags;
+	u32 input_status;
 
+	spin_lock_irqsave(&video->lock, flags);
+	input_status = video->v4l2_input_status;
 	aspeed_video_on(video);
 
 	/* Exit early in case no clients remain */
-	if (test_bit(VIDEO_STOPPED, &video->flags))
+	if (test_bit(VIDEO_STOPPED, &video->flags)) {
+		spin_unlock_irqrestore(&video->lock, flags);
 		goto done;
+	}
 
 	aspeed_video_init_regs(video);
-
+	spin_unlock_irqrestore(&video->lock, flags);
 	aspeed_video_get_resolution(video);
 
 	if (video->detected_timings.width != video->active_timings.width ||
-- 
2.21.0

[PATCH dev-5.0 2/3] media: platform: aspeed: refine clock control logic

[Jae Hyun Yoo]

Currently, this driver calls clk_prepare and clk_unprepare from
interrupt context too but these should be called from sleepable
context only. To fix this issue, this commit splits out
clk_enable/disable and clk_prepare/unprepare, and it places
clk_prepare/unprepare calls into the module probe/remove function.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
 drivers/media/platform/aspeed-video.c | 38 ++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index e70be8fdbde5..75b43488ae8e 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -491,8 +491,8 @@ static void aspeed_video_off(struct aspeed_video *video)
 	aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
 
 	/* Turn off the relevant clocks */
-	clk_disable_unprepare(video->vclk);
-	clk_disable_unprepare(video->eclk);
+	clk_disable(video->vclk);
+	clk_disable(video->eclk);
 
 	clear_bit(VIDEO_CLOCKS_ON, &video->flags);
 }
@@ -503,8 +503,8 @@ static void aspeed_video_on(struct aspeed_video *video)
 		return;
 
 	/* Turn on the relevant clocks */
-	clk_prepare_enable(video->eclk);
-	clk_prepare_enable(video->vclk);
+	clk_enable(video->eclk);
+	clk_enable(video->vclk);
 
 	set_bit(VIDEO_CLOCKS_ON, &video->flags);
 }
@@ -1628,31 +1628,46 @@ static int aspeed_video_init(struct aspeed_video *video)
 		return PTR_ERR(video->eclk);
 	}
 
+	rc = clk_prepare(video->eclk);
+	if (rc)
+		return rc;
+
 	video->vclk = devm_clk_get(dev, "vclk");
 	if (IS_ERR(video->vclk)) {
 		dev_err(dev, "Unable to get VCLK\n");
-		return PTR_ERR(video->vclk);
+		rc = PTR_ERR(video->vclk);
+		goto err_unprepare_eclk;
 	}
 
+	rc = clk_prepare(video->vclk);
+	if (rc)
+		goto err_unprepare_eclk;
+
 	of_reserved_mem_device_init(dev);
 
 	rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
 	if (rc) {
 		dev_err(dev, "Failed to set DMA mask\n");
-		of_reserved_mem_device_release(dev);
-		return rc;
+		goto err_release_reserved_mem;
 	}
 
 	if (!aspeed_video_alloc_buf(video, &video->jpeg,
 				    VE_JPEG_HEADER_SIZE)) {
 		dev_err(dev, "Failed to allocate DMA for JPEG header\n");
-		of_reserved_mem_device_release(dev);
-		return rc;
+		goto err_release_reserved_mem;
 	}
 
 	aspeed_video_init_jpeg_table(video->jpeg.virt, video->yuv420);
 
 	return 0;
+
+err_release_reserved_mem:
+	of_reserved_mem_device_release(dev);
+	clk_unprepare(video->vclk);
+err_unprepare_eclk:
+	clk_unprepare(video->eclk);
+
+	return rc;
 }
 
 static int aspeed_video_probe(struct platform_device *pdev)
@@ -1696,6 +1711,11 @@ static int aspeed_video_remove(struct platform_device *pdev)
 	struct v4l2_device *v4l2_dev = dev_get_drvdata(dev);
 	struct aspeed_video *video = to_aspeed_video(v4l2_dev);
 
+	aspeed_video_off(video);
+
+	clk_unprepare(video->vclk);
+	clk_unprepare(video->eclk);
+
 	video_unregister_device(&video->vdev);
 
 	vb2_queue_release(&video->queue);
-- 
2.21.0

[PATCH dev-5.0 3/3] media: platform: aspeed: change irq to threaded irq

[Jae Hyun Yoo]

Differently fron other Aspeed drivers, this driver calls clock
control APIs in interrupt context. Since ECLK is coupled with a
reset bit in clk-aspeed module, aspeed_clk_enable will make 10ms of
busy waiting delay for triggering the reset and it will eventually
disturb other drivers' interrupt handling. To fix this issue, this
commit changes this driver's irq to threaded irq so that the delay
can be happened in a thread context.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
 drivers/media/platform/aspeed-video.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index 75b43488ae8e..9da61beeef52 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -1615,8 +1615,9 @@ static int aspeed_video_init(struct aspeed_video *video)
 		return -ENODEV;
 	}
 
-	rc = devm_request_irq(dev, irq, aspeed_video_irq, IRQF_SHARED,
-			      DEVICE_NAME, video);
+	rc = devm_request_threaded_irq(dev, irq, NULL, aspeed_video_irq,
+				       IRQF_ONESHOT | IRQF_SHARED, DEVICE_NAME,
+				       video);
 	if (rc < 0) {
 		dev_err(dev, "Unable to request IRQ %d\n", irq);
 		return rc;
-- 
2.21.0

Re: [PATCH dev-5.0 0/3] Improve stability of Aspeed video engine driver

[Joel Stanley]

Hi Jae,

On Tue, 9 Apr 2019 at 19:21, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> wrote:
>
> This patch series improves stability of Aspeed video engine driver by fixing
> clock control and irq handling logic in the driver.
>
> Jae Hyun Yoo (3):
>   media: platform: aspeed: fix a kernel warning on clk control
>   media: platform: aspeed: refine clock control logic
>   media: platform: aspeed: change irq to threaded irq

I had a glance at these patches and they look okay to me. Thanks for
the testing and sending fixes. Can you please send them upstream?

I can apply them to the openbmc tree once we have Eddie's ack.

>
>  drivers/media/platform/aspeed-video.c | 75 ++++++++++++++++++++++-----
>  1 file changed, 61 insertions(+), 14 deletions(-)
>
> --
> 2.21.0
>

Re: [PATCH dev-5.0 0/3] Improve stability of Aspeed video engine driver

[Jae Hyun Yoo]

Hi Joel,

On 4/10/2019 6:26 AM, Joel Stanley wrote:
> Hi Jae,
> 
> On Tue, 9 Apr 2019 at 19:21, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> wrote:
>>
>> This patch series improves stability of Aspeed video engine driver by fixing
>> clock control and irq handling logic in the driver.
>>
>> Jae Hyun Yoo (3):
>>    media: platform: aspeed: fix a kernel warning on clk control
>>    media: platform: aspeed: refine clock control logic
>>    media: platform: aspeed: change irq to threaded irq
> 
> I had a glance at these patches and they look okay to me. Thanks for
> the testing and sending fixes. Can you please send them upstream?

Yes, sure I will upstream it. Eddie is still trying to upstream his eclk
reset handling fix so I will submit this series when Eddie's patch goes
into upstream.

> I can apply them to the openbmc tree once we have Eddie's ack.

Thanks!

-Jae

>>   drivers/media/platform/aspeed-video.c | 75 ++++++++++++++++++++++-----
>>   1 file changed, 61 insertions(+), 14 deletions(-)
>>
>> --
>> 2.21.0
>>

Re: [PATCH dev-5.0 1/3] media: platform: aspeed: fix a kernel warning on clk control

[Eddie James]

On 4/9/19 2:20 PM, Jae Hyun Yoo wrote:
> Video engine clock control functions in the Aspeed video engine
> driver are being called from multiple context without any
> protection so video clocks can be double disabled and eventually
> it causes a kernel warning with stack dump printing out like below:
>
> [  515.540498] ------------[ cut here ]------------
> [  515.545174] WARNING: CPU: 0 PID: 1310 at drivers/clk/clk.c:684 clk_core_unprepare+0x13c/0x170
> [  515.553806] vclk-gate already unprepared
> [  515.557841] CPU: 0 PID: 1310 Comm: obmc-ikvm Tainted: G        W         5.0.6-df66fbc97853fbba90a0bfa44de32f3d5f7602b4 #1
> [  515.568973] Hardware name: Generic DT based system
> [  515.573777] Backtrace:
> [  515.576272] [<80107cdc>] (dump_backtrace) from [<80107f10>] (show_stack+0x20/0x24)
> [  515.583930]  r7:803a5614 r6:00000009 r5:00000000 r4:9d88fe1c
> [  515.589712] [<80107ef0>] (show_stack) from [<80690184>] (dump_stack+0x20/0x28)
> [  515.597053] [<80690164>] (dump_stack) from [<80116044>] (__warn.part.3+0xb4/0xdc)
> [  515.604557] [<80115f90>] (__warn.part.3) from [<801160d8>] (warn_slowpath_fmt+0x6c/0x90)
> [  515.612734]  r6:000002ac r5:8080befc r4:80a07008
> [  515.617463] [<80116070>] (warn_slowpath_fmt) from [<803a5614>] (clk_core_unprepare+0x13c/0x170)
> [  515.626167]  r3:8080cdf4 r2:8080bfc0
> [  515.629834]  r7:98d682a8 r6:9d8a9200 r5:9e5151a0 r4:97abd620
> [  515.635530] [<803a54d8>] (clk_core_unprepare) from [<803a76a4>] (clk_unprepare+0x34/0x3c)
> [  515.643812]  r5:9e5151a0 r4:97abd620
> [  515.647529] [<803a7670>] (clk_unprepare) from [<804f36ec>] (aspeed_video_off+0x38/0x50)
> [  515.655539]  r5:9e5151a0 r4:9e504000
> [  515.659242] [<804f36b4>] (aspeed_video_off) from [<804f4358>] (aspeed_video_release+0x90/0x114)
> [  515.668036]  r5:9e5044b0 r4:9e504000
> [  515.671643] [<804f42c8>] (aspeed_video_release) from [<804d302c>] (v4l2_release+0xd4/0xe8)
> [  515.679999]  r7:98d682a8 r6:9d087810 r5:9d8a9200 r4:9e504318
> [  515.685695] [<804d2f58>] (v4l2_release) from [<80236454>] (__fput+0x98/0x1c4)
> [  515.692914]  r5:9e51b608 r4:9d8a9200
> [  515.696597] [<802363bc>] (__fput) from [<802365e8>] (____fput+0x18/0x1c)
> [  515.703315]  r9:80a0700c r8:801011e4 r7:00000000 r6:80a64b9c r5:9d8e35a0 r4:9d8e38dc
> [  515.711167] [<802365d0>] (____fput) from [<80131ca4>] (task_work_run+0x7c/0xa0)
> [  515.718596] [<80131c28>] (task_work_run) from [<80106884>] (do_work_pending+0x4a8/0x578)
> [  515.726777]  r7:801011e4 r6:80a07008 r5:9d88ffb0 r4:ffffe000
> [  515.732466] [<801063dc>] (do_work_pending) from [<8010106c>] (slow_work_pending+0xc/0x20)
> [  515.740727] Exception stack(0x9d88ffb0 to 0x9d88fff8)
> [  515.745840] ffa0:                                     00000000 76f18094 00000000 00000000
> [  515.754122] ffc0: 00000007 00176778 7eda4c20 00000006 00000000 00000000 48e20fa4 00000000
> [  515.762386] ffe0: 00000002 7eda4b08 00000000 48f91efc 80000010 00000007
> [  515.769097]  r10:00000000 r9:9d88e000 r8:801011e4 r7:00000006 r6:7eda4c20 r5:00176778
> [  515.777006]  r4:00000007
> [  515.779558] ---[ end trace 12c04aadef8afbbb ]---
> [  515.784176] ------------[ cut here ]------------
> [  515.788817] WARNING: CPU: 0 PID: 1310 at drivers/clk/clk.c:825 clk_core_disable+0x18c/0x204
> [  515.797161] eclk-gate already disabled
> [  515.800916] CPU: 0 PID: 1310 Comm: obmc-ikvm Tainted: G        W         5.0.6-df66fbc97853fbba90a0bfa44de32f3d5f7602b4 #1
> [  515.811945] Hardware name: Generic DT based system
> [  515.816730] Backtrace:
> [  515.819210] [<80107cdc>] (dump_backtrace) from [<80107f10>] (show_stack+0x20/0x24)
> [  515.826782]  r7:803a5900 r6:00000009 r5:00000000 r4:9d88fe04
> [  515.832454] [<80107ef0>] (show_stack) from [<80690184>] (dump_stack+0x20/0x28)
> [  515.839687] [<80690164>] (dump_stack) from [<80116044>] (__warn.part.3+0xb4/0xdc)
> [  515.847170] [<80115f90>] (__warn.part.3) from [<801160d8>] (warn_slowpath_fmt+0x6c/0x90)
> [  515.855247]  r6:00000339 r5:8080befc r4:80a07008
> [  515.859868] [<80116070>] (warn_slowpath_fmt) from [<803a5900>] (clk_core_disable+0x18c/0x204)
> [  515.868385]  r3:8080cdd0 r2:8080c00c
> [  515.871957]  r7:98d682a8 r6:9d8a9200 r5:97abd560 r4:97abd560
> [  515.877615] [<803a5774>] (clk_core_disable) from [<803a59a0>] (clk_core_disable_lock+0x28/0x34)
> [  515.886301]  r7:98d682a8 r6:9d8a9200 r5:97abd560 r4:a0000013
> [  515.891960] [<803a5978>] (clk_core_disable_lock) from [<803a7714>] (clk_disable+0x2c/0x30)
> [  515.900216]  r5:9e5151a0 r4:9e515f60
> [  515.903816] [<803a76e8>] (clk_disable) from [<804f36f8>] (aspeed_video_off+0x44/0x50)
> [  515.911656] [<804f36b4>] (aspeed_video_off) from [<804f4358>] (aspeed_video_release+0x90/0x114)
> [  515.920341]  r5:9e5044b0 r4:9e504000
> [  515.923921] [<804f42c8>] (aspeed_video_release) from [<804d302c>] (v4l2_release+0xd4/0xe8)
> [  515.932184]  r7:98d682a8 r6:9d087810 r5:9d8a9200 r4:9e504318
> [  515.937851] [<804d2f58>] (v4l2_release) from [<80236454>] (__fput+0x98/0x1c4)
> [  515.944980]  r5:9e51b608 r4:9d8a9200
> [  515.948559] [<802363bc>] (__fput) from [<802365e8>] (____fput+0x18/0x1c)
> [  515.955257]  r9:80a0700c r8:801011e4 r7:00000000 r6:80a64b9c r5:9d8e35a0 r4:9d8e38dc
> [  515.963008] [<802365d0>] (____fput) from [<80131ca4>] (task_work_run+0x7c/0xa0)
> [  515.970333] [<80131c28>] (task_work_run) from [<80106884>] (do_work_pending+0x4a8/0x578)
> [  515.978421]  r7:801011e4 r6:80a07008 r5:9d88ffb0 r4:ffffe000
> [  515.984086] [<801063dc>] (do_work_pending) from [<8010106c>] (slow_work_pending+0xc/0x20)
> [  515.992247] Exception stack(0x9d88ffb0 to 0x9d88fff8)
> [  515.997296] ffa0:                                     00000000 76f18094 00000000 00000000
> [  516.005473] ffc0: 00000007 00176778 7eda4c20 00000006 00000000 00000000 48e20fa4 00000000
> [  516.013642] ffe0: 00000002 7eda4b08 00000000 48f91efc 80000010 00000007
> [  516.020257]  r10:00000000 r9:9d88e000 r8:801011e4 r7:00000006 r6:7eda4c20 r5:00176778
> [  516.028072]  r4:00000007
> [  516.030606] ---[ end trace 12c04aadef8afbbc ]---
>
> To prevent this issue, this commit adds spinlock protection and
> clock status checking logic into the Aspeed video engine driver.


Thanks Jae!

Reviewed-by: Eddie James <eajames@linux.ibm.com>


>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
>   drivers/media/platform/aspeed-video.c | 32 ++++++++++++++++++++++++---
>   1 file changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
> index 8144fe36ad48..e70be8fdbde5 100644
> --- a/drivers/media/platform/aspeed-video.c
> +++ b/drivers/media/platform/aspeed-video.c
> @@ -187,6 +187,7 @@ enum {
>   	VIDEO_STREAMING,
>   	VIDEO_FRAME_INPRG,
>   	VIDEO_STOPPED,
> +	VIDEO_CLOCKS_ON,
>   };
>   
>   struct aspeed_video_addr {
> @@ -483,19 +484,29 @@ static void aspeed_video_enable_mode_detect(struct aspeed_video *video)
>   
>   static void aspeed_video_off(struct aspeed_video *video)
>   {
> +	if (!test_bit(VIDEO_CLOCKS_ON, &video->flags))
> +		return;
> +
>   	/* Disable interrupts */
>   	aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
>   
>   	/* Turn off the relevant clocks */
>   	clk_disable_unprepare(video->vclk);
>   	clk_disable_unprepare(video->eclk);
> +
> +	clear_bit(VIDEO_CLOCKS_ON, &video->flags);
>   }
>   
>   static void aspeed_video_on(struct aspeed_video *video)
>   {
> +	if (test_bit(VIDEO_CLOCKS_ON, &video->flags))
> +		return;
> +
>   	/* Turn on the relevant clocks */
>   	clk_prepare_enable(video->eclk);
>   	clk_prepare_enable(video->vclk);
> +
> +	set_bit(VIDEO_CLOCKS_ON, &video->flags);
>   }
>   
>   static void aspeed_video_bufs_done(struct aspeed_video *video,
> @@ -513,12 +524,14 @@ static void aspeed_video_bufs_done(struct aspeed_video *video,
>   
>   static void aspeed_video_irq_res_change(struct aspeed_video *video)
>   {
> +	spin_lock(&video->lock);
>   	dev_dbg(video->dev, "Resolution changed; resetting\n");
>   
>   	set_bit(VIDEO_RES_CHANGE, &video->flags);
>   	clear_bit(VIDEO_FRAME_INPRG, &video->flags);
>   
>   	aspeed_video_off(video);
> +	spin_unlock(&video->lock);
>   	aspeed_video_bufs_done(video, VB2_BUF_STATE_ERROR);
>   
>   	schedule_delayed_work(&video->res_work, RESOLUTION_CHANGE_DELAY);
> @@ -938,9 +951,13 @@ static void aspeed_video_init_regs(struct aspeed_video *video)
>   
>   static void aspeed_video_start(struct aspeed_video *video)
>   {
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&video->lock, flags);
>   	aspeed_video_on(video);
>   
>   	aspeed_video_init_regs(video);
> +	spin_unlock_irqrestore(&video->lock, flags);
>   
>   	/* Resolution set to 640x480 if no signal found */
>   	aspeed_video_get_resolution(video);
> @@ -956,6 +973,9 @@ static void aspeed_video_start(struct aspeed_video *video)
>   
>   static void aspeed_video_stop(struct aspeed_video *video)
>   {
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&video->lock, flags);
>   	set_bit(VIDEO_STOPPED, &video->flags);
>   	cancel_delayed_work_sync(&video->res_work);
>   
> @@ -969,6 +989,7 @@ static void aspeed_video_stop(struct aspeed_video *video)
>   
>   	video->v4l2_input_status = V4L2_IN_ST_NO_SIGNAL;
>   	video->flags = 0;
> +	spin_unlock_irqrestore(&video->lock, flags);
>   }
>   
>   static int aspeed_video_querycap(struct file *file, void *fh,
> @@ -1306,16 +1327,21 @@ static void aspeed_video_resolution_work(struct work_struct *work)
>   	struct delayed_work *dwork = to_delayed_work(work);
>   	struct aspeed_video *video = container_of(dwork, struct aspeed_video,
>   						  res_work);
> -	u32 input_status = video->v4l2_input_status;
> +	unsigned long flags;
> +	u32 input_status;
>   
> +	spin_lock_irqsave(&video->lock, flags);
> +	input_status = video->v4l2_input_status;
>   	aspeed_video_on(video);
>   
>   	/* Exit early in case no clients remain */
> -	if (test_bit(VIDEO_STOPPED, &video->flags))
> +	if (test_bit(VIDEO_STOPPED, &video->flags)) {
> +		spin_unlock_irqrestore(&video->lock, flags);
>   		goto done;
> +	}
>   
>   	aspeed_video_init_regs(video);
> -
> +	spin_unlock_irqrestore(&video->lock, flags);
>   	aspeed_video_get_resolution(video);
>   
>   	if (video->detected_timings.width != video->active_timings.width ||

Re: [PATCH dev-5.0 2/3] media: platform: aspeed: refine clock control logic

[Eddie James]

On 4/9/19 2:20 PM, Jae Hyun Yoo wrote:
> Currently, this driver calls clk_prepare and clk_unprepare from
> interrupt context too but these should be called from sleepable
> context only. To fix this issue, this commit splits out
> clk_enable/disable and clk_prepare/unprepare, and it places
> clk_prepare/unprepare calls into the module probe/remove function.


Reviewed-by: Eddie James <eajames@linux.ibm.com>


>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
>   drivers/media/platform/aspeed-video.c | 38 ++++++++++++++++++++-------
>   1 file changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
> index e70be8fdbde5..75b43488ae8e 100644
> --- a/drivers/media/platform/aspeed-video.c
> +++ b/drivers/media/platform/aspeed-video.c
> @@ -491,8 +491,8 @@ static void aspeed_video_off(struct aspeed_video *video)
>   	aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
>   
>   	/* Turn off the relevant clocks */
> -	clk_disable_unprepare(video->vclk);
> -	clk_disable_unprepare(video->eclk);
> +	clk_disable(video->vclk);
> +	clk_disable(video->eclk);
>   
>   	clear_bit(VIDEO_CLOCKS_ON, &video->flags);
>   }
> @@ -503,8 +503,8 @@ static void aspeed_video_on(struct aspeed_video *video)
>   		return;
>   
>   	/* Turn on the relevant clocks */
> -	clk_prepare_enable(video->eclk);
> -	clk_prepare_enable(video->vclk);
> +	clk_enable(video->eclk);
> +	clk_enable(video->vclk);
>   
>   	set_bit(VIDEO_CLOCKS_ON, &video->flags);
>   }
> @@ -1628,31 +1628,46 @@ static int aspeed_video_init(struct aspeed_video *video)
>   		return PTR_ERR(video->eclk);
>   	}
>   
> +	rc = clk_prepare(video->eclk);
> +	if (rc)
> +		return rc;
> +
>   	video->vclk = devm_clk_get(dev, "vclk");
>   	if (IS_ERR(video->vclk)) {
>   		dev_err(dev, "Unable to get VCLK\n");
> -		return PTR_ERR(video->vclk);
> +		rc = PTR_ERR(video->vclk);
> +		goto err_unprepare_eclk;
>   	}
>   
> +	rc = clk_prepare(video->vclk);
> +	if (rc)
> +		goto err_unprepare_eclk;
> +
>   	of_reserved_mem_device_init(dev);
>   
>   	rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>   	if (rc) {
>   		dev_err(dev, "Failed to set DMA mask\n");
> -		of_reserved_mem_device_release(dev);
> -		return rc;
> +		goto err_release_reserved_mem;
>   	}
>   
>   	if (!aspeed_video_alloc_buf(video, &video->jpeg,
>   				    VE_JPEG_HEADER_SIZE)) {
>   		dev_err(dev, "Failed to allocate DMA for JPEG header\n");
> -		of_reserved_mem_device_release(dev);
> -		return rc;
> +		goto err_release_reserved_mem;
>   	}
>   
>   	aspeed_video_init_jpeg_table(video->jpeg.virt, video->yuv420);
>   
>   	return 0;
> +
> +err_release_reserved_mem:
> +	of_reserved_mem_device_release(dev);
> +	clk_unprepare(video->vclk);
> +err_unprepare_eclk:
> +	clk_unprepare(video->eclk);
> +
> +	return rc;
>   }
>   
>   static int aspeed_video_probe(struct platform_device *pdev)
> @@ -1696,6 +1711,11 @@ static int aspeed_video_remove(struct platform_device *pdev)
>   	struct v4l2_device *v4l2_dev = dev_get_drvdata(dev);
>   	struct aspeed_video *video = to_aspeed_video(v4l2_dev);
>   
> +	aspeed_video_off(video);
> +
> +	clk_unprepare(video->vclk);
> +	clk_unprepare(video->eclk);
> +
>   	video_unregister_device(&video->vdev);
>   
>   	vb2_queue_release(&video->queue);

Re: [PATCH dev-5.0 3/3] media: platform: aspeed: change irq to threaded irq

[Eddie James]

On 4/9/19 2:20 PM, Jae Hyun Yoo wrote:
> Differently fron other Aspeed drivers, this driver calls clock
> control APIs in interrupt context. Since ECLK is coupled with a
> reset bit in clk-aspeed module, aspeed_clk_enable will make 10ms of
> busy waiting delay for triggering the reset and it will eventually
> disturb other drivers' interrupt handling. To fix this issue, this
> commit changes this driver's irq to threaded irq so that the delay
> can be happened in a thread context.


Reviewed-by: Eddie James <eajames@linux.ibm.com>


>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
>   drivers/media/platform/aspeed-video.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
> index 75b43488ae8e..9da61beeef52 100644
> --- a/drivers/media/platform/aspeed-video.c
> +++ b/drivers/media/platform/aspeed-video.c
> @@ -1615,8 +1615,9 @@ static int aspeed_video_init(struct aspeed_video *video)
>   		return -ENODEV;
>   	}
>   
> -	rc = devm_request_irq(dev, irq, aspeed_video_irq, IRQF_SHARED,
> -			      DEVICE_NAME, video);
> +	rc = devm_request_threaded_irq(dev, irq, NULL, aspeed_video_irq,
> +				       IRQF_ONESHOT | IRQF_SHARED, DEVICE_NAME,
> +				       video);
>   	if (rc < 0) {
>   		dev_err(dev, "Unable to request IRQ %d\n", irq);
>   		return rc;