From d11df1fcec1527854ceeaeeab51b1b28583ce5c7 Mon Sep 17 00:00:00 2001 From: Cho KyongHo Date: Wed, 28 Dec 2016 23:17:15 +0900 Subject: [PATCH] [COMMON] media: scaler: wait device stop running locklessly Since "fec3dab [8895] media: scaler: add mutex lock" patch has been applied, the following problem is reported: BUG: sleeping function called from invalid context at kernel/locking/mutex.c:617 in_atomic(): 1, irqs_disabled(): 128, pid: 0, name: swapper/0 CPU: 0 MPIDR: 80000100 PID: 0 Comm: swapper/0 Tainted: G W 4.4.13-10228462-eng #1 Hardware name: Samsung SM-G955F rev00 board based on EXYNOS8895 (DT) Call trace: [] dump_backtrace+0x0/0x110 [] show_stack+0x14/0x20 [] dump_stack+0xa8/0xe4 [] ___might_sleep+0x244/0x258 [] __might_sleep+0x78/0x8c [] mutex_lock_nested+0x3c/0x3bc [] sc_m2m1shot_device_run+0x30/0xd4 [] m2m1shot_task_schedule+0x90/0xd8 [] m2m1shot_task_finish+0x78/0x90 [] sc_irq_handler+0x4a4/0x4f4 [] handle_irq_event_percpu+0x15c/0x418 [] handle_irq_event+0x44/0x78 [] handle_fasteoi_irq+0xc4/0x138 [] generic_handle_irq+0x1c/0x30 [] __handle_domain_irq+0xcc/0x11c [] gic_handle_irq+0x70/0xcc It is caused by the mutex lock introduced by the above patch and the mutex is required for modifying DEV_SUSPEND and testing DEV_RUN atomically. However, the problem that a new task starts running between testing DEV_SUSPEND and testing DEV_RUN in sc_suspend() and sc_shutdown() is basically caused by the sc_irq_handler() that clears DEV_RUN before testing DEV_SUSPEND. Therefore, the problem of running an unexpected task is simply solved by moving clearing DEV_RUN after testing DEV_SUSPEND. Change-Id: I7deed59fc636868d062a63ba8c5b2bd5b9f3c753 Signed-off-by: Cho KyongHo --- .../platform/exynos/scaler/scaler-core.c | 82 ++++++++----------- 1 file changed, 35 insertions(+), 47 deletions(-) diff --git a/drivers/media/platform/exynos/scaler/scaler-core.c b/drivers/media/platform/exynos/scaler/scaler-core.c index a15cc81ad745..0a2fb06959ea 100644 --- a/drivers/media/platform/exynos/scaler/scaler-core.c +++ b/drivers/media/platform/exynos/scaler/scaler-core.c @@ -2468,37 +2468,46 @@ static int sc_run_next_job(struct sc_dev *sc) unsigned int h_shift, v_shift; int ret; + ret = sc_power_clk_enable(sc); + if (ret) + return ret; + spin_lock_irqsave(&sc->ctxlist_lock, flags); if (sc->current_ctx || list_empty(&sc->context_list)) { /* a job is currently being processed or no job is to run */ + sc_clk_power_disable(sc); + spin_unlock_irqrestore(&sc->ctxlist_lock, flags); + return 0; + } + + /* + * sc_run_next_job() must not reenter while sc->state is DEV_RUN. + * DEV_RUN is cleared when an operation is finished. + */ + BUG_ON(test_bit(DEV_RUN, &sc->state)); + + set_bit(DEV_RUN, &sc->state); + + if (test_bit(DEV_SUSPEND, &sc->state)) { + clear_bit(DEV_RUN, &sc->state); + sc_clk_power_disable(sc); spin_unlock_irqrestore(&sc->ctxlist_lock, flags); return 0; } ctx = list_first_entry(&sc->context_list, struct sc_ctx, node); + set_bit(CTX_RUN, &ctx->flags); list_del_init(&ctx->node); sc->current_ctx = ctx; spin_unlock_irqrestore(&sc->ctxlist_lock, flags); - /* - * sc_run_next_job() must not reenter while sc->state is DEV_RUN. - * DEV_RUN is cleared when an operation is finished. - */ - BUG_ON(test_bit(DEV_RUN, &sc->state)); - s_frame = &ctx->s_frame; d_frame = &ctx->d_frame; - ret = sc_power_clk_enable(sc); - if (ret) { - pm_runtime_put(sc->dev); - return ret; - } - sc_hwset_init(sc); if (ctx->i_frame) { @@ -2588,9 +2597,6 @@ static int sc_run_next_job(struct sc_dev *sc) sc_hwset_int_en(sc); - set_bit(DEV_RUN, &sc->state); - set_bit(CTX_RUN, &ctx->flags); - sc_set_prefetch_buffers(sc->dev, ctx); mod_timer(&sc->wdt.timer, jiffies + SC_TIMEOUT); @@ -2625,6 +2631,11 @@ static int sc_add_context_and_run(struct sc_dev *sc, struct sc_ctx *ctx) { unsigned long flags; + if (test_bit(CTX_ABORT, &ctx->flags)) { + dev_err(sc->dev, "aborted scaler device run\n"); + return -EAGAIN; + } + spin_lock_irqsave(&sc->ctxlist_lock, flags); list_add_tail(&ctx->node, &sc->context_list); spin_unlock_irqrestore(&sc->ctxlist_lock, flags); @@ -2641,8 +2652,6 @@ static irqreturn_t sc_irq_handler(int irq, void *priv) spin_lock(&sc->slock); - clear_bit(DEV_RUN, &sc->state); - /* * ok to access sc->current_ctx withot ctxlist_lock held * because it is not modified until sc_run_next_job() is called. @@ -2667,10 +2676,11 @@ static irqreturn_t sc_irq_handler(int irq, void *priv) if (!SCALER_INT_OK(irq_status)) sc_hwset_soft_reset(sc); - sc_clk_power_disable(sc); - + clear_bit(DEV_RUN, &sc->state); clear_bit(CTX_RUN, &ctx->flags); + sc_clk_power_disable(sc); + if (ctx->context_type == SC_CTX_V4L2_TYPE) { BUG_ON(ctx != v4l2_m2m_get_curr_priv(sc->m2m.m2m_dev)); @@ -2696,16 +2706,10 @@ static irqreturn_t sc_irq_handler(int irq, void *priv) SCALER_INT_OK(irq_status) ? VB2_BUF_STATE_DONE : VB2_BUF_STATE_ERROR); - if (test_bit(DEV_SUSPEND, &sc->state)) { - sc_dbg("wake up blocked process by suspend\n"); - wake_up(&sc->wait); - } else { - v4l2_m2m_job_finish(sc->m2m.m2m_dev, ctx->m2m_ctx); - } + v4l2_m2m_job_finish(sc->m2m.m2m_dev, ctx->m2m_ctx); /* Wake up from CTX_ABORT state */ - if (test_and_clear_bit(CTX_ABORT, &ctx->flags)) - wake_up(&sc->wait); + clear_bit(CTX_ABORT, &ctx->flags); } else { struct m2m1shot_task *task = m2m1shot_get_current_task(sc->m21dev); @@ -2725,6 +2729,8 @@ static irqreturn_t sc_irq_handler(int irq, void *priv) sc->current_ctx = NULL; spin_unlock(&sc->ctxlist_lock); + wake_up(&sc->wait); + sc_run_next_job(sc); isr_unlock: @@ -2847,16 +2853,6 @@ static void sc_m2m_device_run(void *priv) struct sc_dev *sc = ctx->sc_dev; struct sc_frame *s_frame, *d_frame; - if (test_bit(DEV_SUSPEND, &sc->state)) { - dev_err(sc->dev, "Scaler is in suspend state\n"); - return; - } - - if (test_bit(CTX_ABORT, &ctx->flags)) { - dev_err(sc->dev, "aborted scaler device run\n"); - return; - } - s_frame = &ctx->s_frame; d_frame = &ctx->d_frame; @@ -3277,13 +3273,6 @@ static int sc_m2m1shot_device_run(struct m2m1shot_context *m21ctx, struct sc_dev *sc = ctx->sc_dev; struct sc_frame *s_frame, *d_frame; - if (test_bit(DEV_SUSPEND, &sc->state)) { - dev_err(sc->dev, "Scaler is in suspend state\n"); - return -EAGAIN; - } - - /* no aborted state is required for m2m1shot */ - s_frame = &ctx->s_frame; d_frame = &ctx->d_frame; @@ -3424,6 +3413,8 @@ static int sc_resume(struct device *dev) clear_bit(DEV_SUSPEND, &sc->state); + sc_run_next_job(sc); + return 0; } #endif @@ -3650,11 +3641,8 @@ static int sc_remove(struct platform_device *pdev) static void sc_shutdown(struct platform_device *pdev) { struct sc_dev *sc = platform_get_drvdata(pdev); - unsigned long flags; - spin_lock_irqsave(&sc->slock, flags); set_bit(DEV_SUSPEND, &sc->state); - spin_unlock_irqrestore(&sc->slock, flags); wait_event(sc->wait, !test_bit(DEV_RUN, &sc->state)); -- 2.20.1