Re: [PATCH v2] media: platform: fix a kernel warning on clk control

[Jae Hyun Yoo <jae.hyun.yoo@xxxxxxxxxxxxxxx>]

On 4/12/2019 7:33 AM, Eddie James wrote:
> On 4/12/19 7:17 AM, Herrenschmidt, Benjamin wrote:
> > On Fri, 2019-04-12 at 13:00 +0200, Hans Verkuil wrote:
> > > Eddie, can you review this?
>
>
> Yes, this looks good.
>
> Thanks,
>
> Reviewed-by: Eddie James <eajames@xxxxxxxxxxxxx>

Thanks Eddie!

I'll submit a new patch on top of your upstreaming patch. It would be
better to match the merge order with our downstream tree.

Regards,
Jae

> > > Jae Hyun Yoo, for future reference: please add the driver name in the
> > > subject.
> > > I.e. something like this: [PATCH v2] media: aspeed: fix a kernel
> > > warning on clk control
> > >
> > > That helps maintainers figuring out who should look at which patch.
> > test_bit/clear_bit/set_bit are atomic. The way they are used here is
> > either a waste of cycles (there's already a lock) or racy. Not too sure
>
>
> Yes, it's conceivable that this could be done locklessly, but I'm fine 
> with being cautious. I asked Jae to use the bit functions and flag for 
> code cleanliness instead of an additional bool... are the additional 
> cycles that bad here?
>
>
> > :)
> >
> > Cheers,
> > Ben.
> >
> > > Regards,
> > >
> > >     Hans
> > >
> > > On 3/29/19 10:27 PM, Jae Hyun Yoo wrote:
> > > > video_on and video_off functions in the Aspeed video engine driver
> > > > are
> > > > being called from multiple contexts without any protection so video
> > > > clocks
> > > > can be disabled twice and eventually it causes a kernel warning
> > > > with stack
> > > > dump printing out like below:
> > > >
> > > > [  120.034729] WARNING: CPU: 0 PID: 1334 at drivers/clk/clk.c:684
> > > > clk_core_unprepare+0x13c/0x170
> > > > [  120.043252] eclk-gate already unprepared
> > > > [  120.047283] CPU: 0 PID: 1334 Comm: obmc-ikvm Tainted:
> > > > G        W         5.0.3-b94b74e8b52db91fe4e99e0bb481ec8bf2b5b47c
> > > > #1
> > > > [  120.058417] Hardware name: Generic DT based system
> > > > [  120.063219] Backtrace:
> > > > [  120.065787] [<80107cdc>] (dump_backtrace) from [<80107f10>]
> > > > (show_stack+0x20/0x24)
> > > > [  120.073371]  r7:803a4ff0 r6:00000009 r5:00000000 r4:96197e1c
> > > > [  120.079152] [<80107ef0>] (show_stack) from [<8068f7d8>]
> > > > (dump_stack+0x20/0x28)
> > > > [  120.086479] [<8068f7b8>] (dump_stack) from [<8011604c>]
> > > > (__warn.part.3+0xb4/0xdc)
> > > > [  120.094068] [<80115f98>] (__warn.part.3) from [<801160e0>]
> > > > (warn_slowpath_fmt+0x6c/0x90)
> > > > [  120.102164]  r6:000002ac r5:8080c0b8 r4:80a07008
> > > > [  120.106893] [<80116078>] (warn_slowpath_fmt) from [<803a4ff0>]
> > > > (clk_core_unprepare+0x13c/0x170)
> > > > [  120.115686]  r3:8080cf8c r2:8080c17c
> > > > [  120.119276]  r7:97d68e58 r6:9df23200 r5:9668c260 r4:96459260
> > > > [  120.125046] [<803a4eb4>] (clk_core_unprepare) from [<803a707c>]
> > > > (clk_unprepare+0x34/0x3c)
> > > > [  120.133226]  r5:9668c260 r4:96459260
> > > > [  120.136932] [<803a7048>] (clk_unprepare) from [<804f34bc>]
> > > > (aspeed_video_off+0x44/0x48)
> > > > [  120.145031]  r5:9668c260 r4:9668cbc0
> > > > [  120.148647] [<804f3478>] (aspeed_video_off) from [<804f3fd0>]
> > > > (aspeed_video_release+0x94/0x118)
> > > > [  120.157435]  r5:966a0cb8 r4:966a0800
> > > > [  120.161049] [<804f3f3c>] (aspeed_video_release) from
> > > > [<804d2c58>] (v4l2_release+0xd4/0xe8)
> > > > [  120.169404]  r7:97d68e58 r6:9d087810 r5:9df23200 r4:966a0b20
> > > > [  120.175168] [<804d2b84>] (v4l2_release) from [<80236224>]
> > > > (__fput+0x98/0x1c4)
> > > > [  120.182316]  r5:96698e78 r4:9df23200
> > > > [  120.185994] [<8023618c>] (__fput) from [<802363b8>]
> > > > (____fput+0x18/0x1c)
> > > > [  120.192712]  r9:80a0700c r8:801011e4 r7:00000000 r6:80a64bbc
> > > > r5:961dd560 r4:961dd89c
> > > > [  120.200562] [<802363a0>] (____fput) from [<80131c08>]
> > > > (task_work_run+0x7c/0xa4)
> > > > [  120.207994] [<80131b8c>] (task_work_run) from [<80106884>]
> > > > (do_work_pending+0x4a8/0x578)
> > > > [  120.216163]  r7:801011e4 r6:80a07008 r5:96197fb0 r4:ffffe000
> > > > [  120.221856] [<801063dc>] (do_work_pending) from [<8010106c>]
> > > > (slow_work_pending+0xc/0x20)
> > > > [  120.230116] Exception stack(0x96197fb0 to 0x96197ff8)
> > > > [  120.235254] 7fa0:                                     00000000
> > > > 76ccf094 00000000 00000000
> > > > [  120.243438] 7fc0: 00000008 00a11978 7eab3c30 00000006 00000000
> > > > 00000000 475b0fa4 00000000
> > > > [  120.251692] 7fe0: 00000002 7eab3a40 00000000 47720e38 80000010
> > > > 00000008
> > > > [  120.258396]  r10:00000000 r9:96196000 r8:801011e4 r7:00000006
> > > > r6:7eab3c30 r5:00a11978
> > > > [  120.266291]  r4:00000008
> > > >
> > > > To prevent this issue, this commit adds spinlock protection and
> > > > clock
> > > > status checking logic into the Aspeed video engine driver.
> > > >
> > > > Fixes: d2b4387f3bdf ("media: platform: Add Aspeed Video Engine
> > > > driver")
> > > > Signed-off-by: Jae Hyun Yoo<jae.hyun.yoo@xxxxxxxxxxxxxxx>
> > > > Cc: Eddie James<eajames@xxxxxxxxxxxxx>
> > > > Cc: Mauro Carvalho Chehab<mchehab@xxxxxxxxxx>
> > > > Cc: Joel Stanley<joel@xxxxxxxxx>
> > > > Cc: Andrew Jeffery<andrew@xxxxxxxx>
> > > > ---
> > > >   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 692e08ef38c0..744d22ecc620 100644
> > > > --- a/drivers/media/platform/aspeed-video.c
> > > > +++ b/drivers/media/platform/aspeed-video.c
> > > > @@ -188,6 +188,7 @@ enum {
> > > >       VIDEO_STREAMING,
> > > >       VIDEO_FRAME_INPRG,
> > > >       VIDEO_STOPPED,
> > > > +    VIDEO_CLOCKS_ON,
> > > >   };
> > > >   struct aspeed_video_addr {
> > > > @@ -495,20 +496,30 @@ static void aspeed_video_reset(struct
> > > > aspeed_video *video)
> > > >   static void aspeed_video_off(struct aspeed_video *video)
> > > >   {
> > > > +    if (!test_bit(VIDEO_CLOCKS_ON, &video->flags))
> > > > +        return;
> > > > +
> > > >       aspeed_video_reset(video);
> > > >       /* 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);
> > > >       aspeed_video_reset(video);
> > > > +
> > > > +    set_bit(VIDEO_CLOCKS_ON, &video->flags);
> > > >   }
> > > >   static void aspeed_video_bufs_done(struct aspeed_video *video,
> > > > @@ -526,12 +537,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);
> > > > @@ -951,9 +964,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);
> > > > @@ -969,6 +986,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);
> > > > @@ -982,6 +1002,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,
> > > > @@ -1319,16 +1340,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 ||