[PATCH 0/7] Improve stability of Aspeed video engine driver

[PATCH 0/7] 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.

This series should be applied on top of:
https://www.spinics.net/lists/linux-media/msg150193.html

Jae Hyun Yoo (7):
  media: aspeed: fix a kernel warning on clk control
  media: aspeed: refine clock control logic
  media: aspeed: change irq to threaded irq
  media: aspeed: remove IRQF_SHARED flag
  media: aspeed: reduce noisy log printing outs
  media: aspeed: remove checking of VE_INTERRUPT_CAPTURE_COMPLETE
  media: aspeed: refine interrupt handling logic

 drivers/media/platform/aspeed-video.c | 103 ++++++++++++++++++--------
 1 file changed, 73 insertions(+), 30 deletions(-)

-- 
2.21.0

[PATCH 1/7] media: 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>
Reviewed-by: Eddie James <eajames@linux.ibm.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 55c55a68b016..2dac6d20b180 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

Re: [PATCH 1/7] media: aspeed: fix a kernel warning on clk control

[Benjamin Herrenschmidt]

On Thu, 2019-05-02 at 12:13 -0700, 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:

I already objected to the use of set_bit, clear_bit etc...

Either you are protected by a spinlock, in which case you don't need
them, use either the __ versions (non atomic) or just a bloody bool
flag which is a lot clearer and will generated better code. Or you
aren't protected in which case the code seems racy.


> [  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>
> Reviewed-by: Eddie James <eajames@linux.ibm.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 55c55a68b016..2dac6d20b180 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 1/7] media: aspeed: fix a kernel warning on clk control

[Jae Hyun Yoo]

On 5/7/2019 11:31 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2019-05-02 at 12:13 -0700, 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:
> 
> I already objected to the use of set_bit, clear_bit etc...
> 
> Either you are protected by a spinlock, in which case you don't need
> them, use either the __ versions (non atomic) or just a bloody bool
> flag which is a lot clearer and will generated better code. Or you
> aren't protected in which case the code seems racy.

Got it. I'll use __set_bit and __clear_bit instead. Thanks for
your pointing it out.

Regards,
Jae

>> [  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>
>> Reviewed-by: Eddie James <eajames@linux.ibm.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 55c55a68b016..2dac6d20b180 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 1/7] media: aspeed: fix a kernel warning on clk control

[Benjamin Herrenschmidt]

On Wed, 2019-05-08 at 15:19 -0700, Jae Hyun Yoo wrote:
> On 5/7/2019 11:31 PM, Benjamin Herrenschmidt wrote:
> > On Thu, 2019-05-02 at 12:13 -0700, 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:
> > 
> > I already objected to the use of set_bit, clear_bit etc...
> > 
> > Either you are protected by a spinlock, in which case you don't
> > need
> > them, use either the __ versions (non atomic) or just a bloody bool
> > flag which is a lot clearer and will generated better code. Or you
> > aren't protected in which case the code seems racy.
> 
> Got it. I'll use __set_bit and __clear_bit instead. Thanks for
> your pointing it out.

Why not a bool ? Any reason why you prefer bitops ? They are a bit of
an old-fashioned way of doing things unless you are actively trying to
save space by craming several flags in the same word.

Note: If you do the latter, ensure all users are properly locked, dont'
mix the __set_bit and set_bit variants on the same word.

Cheers,
Ben.
> Regards,
> Jae
> 
> > > [  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>
> > > Reviewed-by: Eddie James <eajames@linux.ibm.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 55c55a68b016..2dac6d20b180 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 1/7] media: aspeed: fix a kernel warning on clk control

[Benjamin Herrenschmidt]

On Wed, 2019-05-08 at 18:19 -0700, Jae Hyun Yoo wrote:
> I changed that from a bool because the maintainer of this code, Eddie
> doesn't like adding of an additional flag. I'll change it back with
> codes in the first submit:
> https://www.spinics.net/lists/linux-media/msg148955.html
> 
> Eddie,
> Please let me know if you have any objection on that.

Ok, so random flags ... ugh.

Well, you can approach it either way. Have them all be bitops or all be
bool.

The tricky thing however is that if they are bitops you need to ensure
that they are *all* manipulated under the same lock. If not you have to
use the atomic bitops variants.

The reason I don't like that is that experience shows that most uses of
such atomic variants in drivers usually are failed attempts at papering
over broken locking.

If everything is covered by a lock, then using the non-atomic versions
is more efficient, but so is using bool (optionally with :1 bitfield
qualifiers to avoid wasting memory), which from a pure C language
perspective I think is more expressive of what you are doing and more
readable.

Cheers,
Ben.

Re: [PATCH 1/7] media: aspeed: fix a kernel warning on clk control

[Eddie James]

On 5/8/19 9:16 PM, Benjamin Herrenschmidt wrote:
> On Wed, 2019-05-08 at 18:19 -0700, Jae Hyun Yoo wrote:
>> I changed that from a bool because the maintainer of this code, Eddie
>> doesn't like adding of an additional flag. I'll change it back with
>> codes in the first submit:
>> https://www.spinics.net/lists/linux-media/msg148955.html
>>
>> Eddie,
>> Please let me know if you have any objection on that.


Thats fine with me. I merely thought it was more conventional to use 
flags rather than bools, but I may be mistaken.

Thanks!

Eddie


> Ok, so random flags ... ugh.
>
> Well, you can approach it either way. Have them all be bitops or all be
> bool.
>
> The tricky thing however is that if they are bitops you need to ensure
> that they are *all* manipulated under the same lock. If not you have to
> use the atomic bitops variants.
>
> The reason I don't like that is that experience shows that most uses of
> such atomic variants in drivers usually are failed attempts at papering
> over broken locking.
>
> If everything is covered by a lock, then using the non-atomic versions
> is more efficient, but so is using bool (optionally with :1 bitfield
> qualifiers to avoid wasting memory), which from a pure C language
> perspective I think is more expressive of what you are doing and more
> readable.
>
> Cheers,
> Ben.
>

Re: [PATCH 1/7] media: aspeed: fix a kernel warning on clk control

[Jae Hyun Yoo]

On 5/8/2019 7:16 PM, Benjamin Herrenschmidt wrote:
> On Wed, 2019-05-08 at 18:19 -0700, Jae Hyun Yoo wrote:
>> I changed that from a bool because the maintainer of this code, Eddie
>> doesn't like adding of an additional flag. I'll change it back with
>> codes in the first submit:
>> https://www.spinics.net/lists/linux-media/msg148955.html
>>
>> Eddie,
>> Please let me know if you have any objection on that.
> 
> Ok, so random flags ... ugh.
> 
> Well, you can approach it either way. Have them all be bitops or all be
> bool.
> 
> The tricky thing however is that if they are bitops you need to ensure
> that they are *all* manipulated under the same lock. If not you have to
> use the atomic bitops variants.
> 
> The reason I don't like that is that experience shows that most uses of
> such atomic variants in drivers usually are failed attempts at papering
> over broken locking.
> 
> If everything is covered by a lock, then using the non-atomic versions
> is more efficient, but so is using bool (optionally with :1 bitfield
> qualifiers to avoid wasting memory), which from a pure C language
> perspective I think is more expressive of what you are doing and more
> readable.

Okay. Probably I need to add one another patch in this series to address
what you pointed out.

I have one question. I reviewed again all bitops in this driver and
checked that some bitops are protected by a spinlock and some others
are not. In this case, can I mix use atomic and non-atomic bitops
depend on each bitop's protection state by the spinlock? Or, would it be
better to change all of them to bool in this case?

Thanks,
Jae

> 
> Cheers,
> Ben.
> 

Re: [PATCH 1/7] media: aspeed: fix a kernel warning on clk control

[Benjamin Herrenschmidt]

On Thu, 2019-05-09 at 10:19 -0700, Jae Hyun Yoo wrote:
> 
> Okay. Probably I need to add one another patch in this series to address
> what you pointed out.
> 
> I have one question. I reviewed again all bitops in this driver and
> checked that some bitops are protected by a spinlock and some others
> are not. In this case, can I mix use atomic and non-atomic bitops
> depend on each bitop's protection state by the spinlock? Or, would it be
> better to change all of them to bool in this case?

No, if some aren't protected by a lock and some are, then they need to
remain atomic.

The question then becomes whether the unprotected ones are actually
correct or just exposing more races.

Cheers,
Ben.

Re: [PATCH 1/7] media: aspeed: fix a kernel warning on clk control

[Jae Hyun Yoo]

On 5/9/2019 4:19 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2019-05-09 at 10:19 -0700, Jae Hyun Yoo wrote:
>>
>> Okay. Probably I need to add one another patch in this series to address
>> what you pointed out.
>>
>> I have one question. I reviewed again all bitops in this driver and
>> checked that some bitops are protected by a spinlock and some others
>> are not. In this case, can I mix use atomic and non-atomic bitops
>> depend on each bitop's protection state by the spinlock? Or, would it be
>> better to change all of them to bool in this case?
> 
> No, if some aren't protected by a lock and some are, then they need to
> remain atomic.
> 
> The question then becomes whether the unprotected ones are actually
> correct or just exposing more races.

Got it. Not sure yet but I think the protected bitops could be moved out
from the spinlock scope then all bitops could be kept as atomic. I'll
look at and test this driver code more deeply again, and will submit v2
soon.

Again, thanks a lot for your review.

Regards,
Jae

Re: [PATCH 1/7] media: aspeed: fix a kernel warning on clk control

[Benjamin Herrenschmidt]

On Thu, 2019-05-09 at 16:51 -0700, Jae Hyun Yoo wrote:
> On 5/9/2019 4:19 PM, Benjamin Herrenschmidt wrote:
> > On Thu, 2019-05-09 at 10:19 -0700, Jae Hyun Yoo wrote:
> > > 
> > > Okay. Probably I need to add one another patch in this series to
> > > address
> > > what you pointed out.
> > > 
> > > I have one question. I reviewed again all bitops in this driver
> > > and
> > > checked that some bitops are protected by a spinlock and some
> > > others
> > > are not. In this case, can I mix use atomic and non-atomic bitops
> > > depend on each bitop's protection state by the spinlock? Or,
> > > would it be
> > > better to change all of them to bool in this case?
> > 
> > No, if some aren't protected by a lock and some are, then they need
> > to
> > remain atomic.
> > 
> > The question then becomes whether the unprotected ones are actually
> > correct or just exposing more races.
> 
> Got it. Not sure yet but I think the protected bitops could be moved
> out
> from the spinlock scope then all bitops could be kept as atomic.

Which is very likely to be extremely racy... (and gratuitously more
costly) :-)

>  I'll
> look at and test this driver code more deeply again, and will submit
> v2
> soon.
> 
> Again, thanks a lot for your review.
> 
> Regards,
> Jae

Re: [PATCH 1/7] media: aspeed: fix a kernel warning on clk control

[Jae Hyun Yoo]

On 5/9/2019 8:01 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2019-05-09 at 16:51 -0700, Jae Hyun Yoo wrote:
>> On 5/9/2019 4:19 PM, Benjamin Herrenschmidt wrote:
>>> On Thu, 2019-05-09 at 10:19 -0700, Jae Hyun Yoo wrote:
>>>>
>>>> Okay. Probably I need to add one another patch in this series to
>>>> address
>>>> what you pointed out.
>>>>
>>>> I have one question. I reviewed again all bitops in this driver
>>>> and
>>>> checked that some bitops are protected by a spinlock and some
>>>> others
>>>> are not. In this case, can I mix use atomic and non-atomic bitops
>>>> depend on each bitop's protection state by the spinlock? Or,
>>>> would it be
>>>> better to change all of them to bool in this case?
>>>
>>> No, if some aren't protected by a lock and some are, then they need
>>> to
>>> remain atomic.
>>>
>>> The question then becomes whether the unprotected ones are actually
>>> correct or just exposing more races.
>>
>> Got it. Not sure yet but I think the protected bitops could be moved
>> out
>> from the spinlock scope then all bitops could be kept as atomic.
> 
> Which is very likely to be extremely racy... (and gratuitously more
> costly) :-)

Okay then, will try to wrap all bitops using a single spinlock and
change all of them to non-atomic bitops. Is it right approach?

Thanks,
Jae

>>   I'll
>> look at and test this driver code more deeply again, and will submit
>> v2
>> soon.
>>
>> Again, thanks a lot for your review.
>>
>> Regards,
>> Jae
> 

[PATCH 2/7] media: 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>
Reviewed-by: Eddie James <eajames@linux.ibm.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 2dac6d20b180..8f4cac41da14 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,12 +1628,21 @@ 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;
+
 	rc = of_reserved_mem_device_init(dev);
 	if (rc) {
 		dev_err(dev, "Unable to reserve memory\n");
@@ -1643,20 +1652,26 @@ static int aspeed_video_init(struct aspeed_video *video)
 	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)
@@ -1700,6 +1715,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 3/7] media: 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>
Reviewed-by: Eddie James <eajames@linux.ibm.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 8f4cac41da14..e1bdafeed007 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

[PATCH 4/7] media: aspeed: remove IRQF_SHARED flag

[Jae Hyun Yoo]

Video Engine has a dedicated interrupt line so this driver doesn't
need to use IRQF_SHARED flag so remove it. Also, it'd be good for
following what Thomas recommended in the IRQF_ONESHOT support
patch like below:

"Note that for now IRQF_ONESHOT cannot be used with IRQF_SHARED to
avoid complex accounting mechanisms."

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

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

[PATCH 5/7] media: aspeed: reduce noisy log printing outs

[Jae Hyun Yoo]

Currently, this driver prints out too much log messages when a
mode change happens, video turned off by screen saver and etc.
Actually, all cases are reported to user space properly. Also,
these are not critical errors but recoverable things, so this
commit changes the log level of some noisy printing outs.

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

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index d88e8d238c49..6dc66d6e8b8b 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -441,7 +441,7 @@ static int aspeed_video_start_frame(struct aspeed_video *video)
 
 	if (!(seq_ctrl & VE_SEQ_CTRL_COMP_BUSY) ||
 	    !(seq_ctrl & VE_SEQ_CTRL_CAP_BUSY)) {
-		dev_err(video->dev, "Engine busy; don't start frame\n");
+		dev_dbg(video->dev, "Engine busy; don't start frame\n");
 		return -EBUSY;
 	}
 
@@ -771,7 +771,7 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
 						      res_check(video),
 						      MODE_DETECT_TIMEOUT);
 		if (!rc) {
-			dev_err(video->dev, "Timed out; first mode detect\n");
+			dev_dbg(video->dev, "Timed out; first mode detect\n");
 			clear_bit(VIDEO_RES_DETECT, &video->flags);
 			return;
 		}
@@ -789,7 +789,7 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
 						      MODE_DETECT_TIMEOUT);
 		clear_bit(VIDEO_RES_DETECT, &video->flags);
 		if (!rc) {
-			dev_err(video->dev, "Timed out; second mode detect\n");
+			dev_dbg(video->dev, "Timed out; second mode detect\n");
 			return;
 		}
 
@@ -823,7 +823,7 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
 	} while (invalid_resolution && (tries++ < INVALID_RESOLUTION_RETRIES));
 
 	if (invalid_resolution) {
-		dev_err(video->dev, "Invalid resolution detected\n");
+		dev_dbg(video->dev, "Invalid resolution detected\n");
 		return;
 	}
 
@@ -1471,7 +1471,7 @@ static void aspeed_video_stop_streaming(struct vb2_queue *q)
 				!test_bit(VIDEO_FRAME_INPRG, &video->flags),
 				STOP_TIMEOUT);
 	if (!rc) {
-		dev_err(video->dev, "Timed out when stopping streaming\n");
+		dev_dbg(video->dev, "Timed out when stopping streaming\n");
 
 		/*
 		 * Need to force stop any DMA and try and get HW into a good
-- 
2.21.0

[PATCH 6/7] media: aspeed: remove checking of VE_INTERRUPT_CAPTURE_COMPLETE

[Jae Hyun Yoo]

VE_INTERRUPT_CAPTURE_COMPLETE and VE_INTERRUPT_COMP_COMPLETE are
not set at the same time but the current interrupt handling
mechanism of this driver doesn't clear the interrupt flag until
both two are set, and this behavior causes unnecessary interrupt
handler calls. In fact, this driver provides JPEG format only so
taking care of the VE_INTERRUPT_COMP_COMPLETE is enough for getting
compressed image frame so this commit gets rid of the
VE_INTERRUPT_CAPTURE_COMPLETE checking logic to simplify the logic.
Handling of VE_INTERRUPT_CAPTURE_COMPLETE could be added back later
when it's actually needed.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Reviewed-by: Eddie James <eajames@linux.ibm.com>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/media/platform/aspeed-video.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index 6dc66d6e8b8b..8d0bb395e46d 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -463,8 +463,7 @@ static int aspeed_video_start_frame(struct aspeed_video *video)
 	aspeed_video_write(video, VE_COMP_ADDR, addr);
 
 	aspeed_video_update(video, VE_INTERRUPT_CTRL, 0,
-			    VE_INTERRUPT_COMP_COMPLETE |
-			    VE_INTERRUPT_CAPTURE_COMPLETE);
+			    VE_INTERRUPT_COMP_COMPLETE);
 
 	aspeed_video_update(video, VE_SEQ_CTRL, 0,
 			    VE_SEQ_CTRL_TRIG_CAPTURE | VE_SEQ_CTRL_TRIG_COMP);
@@ -570,8 +569,7 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
 		}
 	}
 
-	if ((sts & VE_INTERRUPT_COMP_COMPLETE) &&
-	    (sts & VE_INTERRUPT_CAPTURE_COMPLETE)) {
+	if (sts & VE_INTERRUPT_COMP_COMPLETE) {
 		struct aspeed_video_buffer *buf;
 		u32 frame_size = aspeed_video_read(video,
 						   VE_OFFSET_COMP_STREAM);
@@ -600,11 +598,9 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
 				    VE_SEQ_CTRL_FORCE_IDLE |
 				    VE_SEQ_CTRL_TRIG_COMP, 0);
 		aspeed_video_update(video, VE_INTERRUPT_CTRL,
-				    VE_INTERRUPT_COMP_COMPLETE |
-				    VE_INTERRUPT_CAPTURE_COMPLETE, 0);
+				    VE_INTERRUPT_COMP_COMPLETE, 0);
 		aspeed_video_write(video, VE_INTERRUPT_STATUS,
-				   VE_INTERRUPT_COMP_COMPLETE |
-				   VE_INTERRUPT_CAPTURE_COMPLETE);
+				   VE_INTERRUPT_COMP_COMPLETE);
 
 		if (test_bit(VIDEO_STREAMING, &video->flags) && buf)
 			aspeed_video_start_frame(video);
-- 
2.21.0

[PATCH 7/7] media: aspeed: refine interrupt handling logic

[Jae Hyun Yoo]

There are cases that interrupt bits are cleared by a 500ms delayed
work which causes unnecessary irq calls. Also, the current
interrupt handler returns IRQ_HANDLED always but it should return
IRQ_NONE if there is any unhandled interrupt. So this commit
refines the interrupt handling logic to fix these issues.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/media/platform/aspeed-video.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index 8d0bb395e46d..98944a911998 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);
@@ -556,7 +557,7 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
 					    VE_INTERRUPT_MODE_DETECT, 0);
 			aspeed_video_write(video, VE_INTERRUPT_STATUS,
 					   VE_INTERRUPT_MODE_DETECT);
-
+			sts &= ~VE_INTERRUPT_MODE_DETECT;
 			set_bit(VIDEO_MODE_DETECT_DONE, &video->flags);
 			wake_up_interruptible_all(&video->wait);
 		} else {
@@ -601,12 +602,12 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
 				    VE_INTERRUPT_COMP_COMPLETE, 0);
 		aspeed_video_write(video, VE_INTERRUPT_STATUS,
 				   VE_INTERRUPT_COMP_COMPLETE);
-
+		sts &= ~VE_INTERRUPT_COMP_COMPLETE;
 		if (test_bit(VIDEO_STREAMING, &video->flags) && buf)
 			aspeed_video_start_frame(video);
 	}
 
-	return IRQ_HANDLED;
+	return sts ? IRQ_NONE : IRQ_HANDLED;
 }
 
 static void aspeed_video_check_and_set_polarity(struct aspeed_video *video)
-- 
2.21.0

Re: [PATCH 7/7] media: aspeed: refine interrupt handling logic

[Eddie James]

On 5/2/19 2:13 PM, Jae Hyun Yoo wrote:
> There are cases that interrupt bits are cleared by a 500ms delayed
> work which causes unnecessary irq calls. Also, the current
> interrupt handler returns IRQ_HANDLED always but it should return
> IRQ_NONE if there is any unhandled interrupt. So this commit
> refines the interrupt handling logic to fix these issues.


Thanks Jae, looks good.

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


>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   drivers/media/platform/aspeed-video.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
> index 8d0bb395e46d..98944a911998 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);
> @@ -556,7 +557,7 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
>   					    VE_INTERRUPT_MODE_DETECT, 0);
>   			aspeed_video_write(video, VE_INTERRUPT_STATUS,
>   					   VE_INTERRUPT_MODE_DETECT);
> -
> +			sts &= ~VE_INTERRUPT_MODE_DETECT;
>   			set_bit(VIDEO_MODE_DETECT_DONE, &video->flags);
>   			wake_up_interruptible_all(&video->wait);
>   		} else {
> @@ -601,12 +602,12 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
>   				    VE_INTERRUPT_COMP_COMPLETE, 0);
>   		aspeed_video_write(video, VE_INTERRUPT_STATUS,
>   				   VE_INTERRUPT_COMP_COMPLETE);
> -
> +		sts &= ~VE_INTERRUPT_COMP_COMPLETE;
>   		if (test_bit(VIDEO_STREAMING, &video->flags) && buf)
>   			aspeed_video_start_frame(video);
>   	}
>   
> -	return IRQ_HANDLED;
> +	return sts ? IRQ_NONE : IRQ_HANDLED;
>   }
>   
>   static void aspeed_video_check_and_set_polarity(struct aspeed_video *video)