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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx>
> > > Reviewed-by: Eddie James <eajames@xxxxxxxxxxxxx>
> > > ---
> > >   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 ||




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux