Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <07cfbe4d-35e9-b7d2-ca0f-7c38eac2e4e6@samsung.com>
Date:   Fri, 29 Sep 2023 17:42:45 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0)
        Gecko/20100101 Thunderbird/102.15.1
Subject: Re: [PATCH] media: s5p-mfc: Fix potential deadlock on condlock
Content-Language: en-US
To:     Chengfeng Ye <dg573847474@gmail.com>, andrzej.hajda@intel.com,
        mchehab@kernel.org
Cc:     linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org,
        linux-kernel@vger.kernel.org
From:   Marek Szyprowski <m.szyprowski@samsung.com>
In-Reply-To: <20230926105330.10281-1-dg573847474@gmail.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <CGME20230926105338eucas1p286ab56d3177817fbb32713228af9f9ba@eucas1p2.samsung.com>
        <20230926105330.10281-1-dg573847474@gmail.com>
Precedence: bulk

On 26.09.2023 12:53, Chengfeng Ye wrote:
> As &dev->condlock is acquired under irq context along the following
> call chain from s5p_mfc_irq(), other acquisition of the same lock
> inside process context or softirq context should disable irq avoid double
> lock. enc_post_frame_start() seems to be one such function that execute
> under process context or softirq context.
>
> <deadlock #1>
>
> enc_post_frame_start()
> --> clear_work_bit()
> --> spin_loc(&dev->condlock)
> <interrupt>
>     --> s5p_mfc_irq()
>     --> s5p_mfc_handle_frame()
>     --> clear_work_bit()
>     --> spin_lock(&dev->condlock)
>
> This flaw was found by an experimental static analysis tool I am
> developing for irq-related deadlock.
>
> To prevent the potential deadlock, the patch change clear_work_bit()
> inside enc_post_frame_start() to clear_work_bit_irqsave().
>
> Signed-off-by: Chengfeng Ye <dg573847474@gmail.com>
Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>   drivers/media/platform/samsung/s5p-mfc/s5p_mfc_enc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_enc.c
> index f62703cebb77..4b4c129c09e7 100644
> --- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_enc.c
> +++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_enc.c
> @@ -1297,7 +1297,7 @@ static int enc_post_frame_start(struct s5p_mfc_ctx *ctx)
>   	if (ctx->state == MFCINST_FINISHING && ctx->ref_queue_cnt == 0)
>   		src_ready = false;
>   	if (!src_ready || ctx->dst_queue_cnt == 0)
> -		clear_work_bit(ctx);
> +		clear_work_bit_irqsave(ctx);
>   
>   	return 0;
>   }

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland