linux-kernel - Re: [PATCH 2/6] drm/exynos: exynos7_drm_decon: fix suspended condition in decon

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <206c17ac8f79bbd51bf94b8f1f72fbb9@disroot.org>
Date: Wed, 25 Sep 2024 19:22:26 +0000
From: Kaustabh Chakraborty <kauschluss@...root.org>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Inki Dae <inki.dae@...sung.com>, Seung-Woo Kim <sw0312.kim@...sung.com>,
 Kyungmin Park <kyungmin.park@...sung.com>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>, Alim Akhtar <alim.akhtar@...sung.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard
 <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, Rob Herring
 <robh@...nel.org>, Conor Dooley <conor@...nel.org>,
 dri-devel@...ts.freedesktop.org, linux-arm-kernel@...ts.infradead.org,
 linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, kauschluss@...root.org
Subject: Re: [PATCH 2/6] drm/exynos: exynos7_drm_decon: fix suspended
 condition in decon_commit()

On 2024-09-20 12:40, Krzysztof Kozlowski wrote:
> On 19/09/2024 17:11, Kaustabh Chakraborty wrote:
>> decon_commit() gets called during atomic_enable. At this stage, DECON is
>> suspended, and thus the function refuses to run. Fix the suspended
>> condition checking in decon_commit().
>> 
>> Signed-off-by: Kaustabh Chakraborty <kauschluss@...root.org>
>> ---
> 
> If this is a fix, then you miss fixes tag and cc-stable. However the
> explanation seems just incomplete. This looked like a intentional code,
> so you should explain really why original approach was wrong.

Fixes: 96976c3d9aff ("drm/exynos: Add DECON driver")

Now that I read the commit description of the above commit, which mentions
that the DECON driver is based on the FIMD driver, I think it makes more
sense to rewrite the suspend logic exactly as done in the FIMD driver.
Will do it in v2.

Here's a commit description which may be better suited, let me know:

A flag variable in struct decon_context, called 'suspended' is set to false
at the end of decon_atomic_enable() and is set back to true at the end of
decon_atomic_disable().

Functions called in decon_atomic_enable(), such as decon_enable_vblank()
and decon_commit() are guarded by suspend condition checking, where it
refuses to proceed if 'suspended' is set to true. Since 'suspended' isn't
set to true until the end of the calling function, the called functions
aren't even executed.

The original commit, 96976c3d9aff ("drm/exynos: Add DECON driver")
implementing the DECON driver, is based on the FIMD driver, but changes
the suspend flag logic which causes this issue. Implement the suspend
logic present in FIMD, which changes the flag at the beginning of
atomic_enable and atomic_disable instead.

> 
> Best regards,
> Krzysztof