linux-kernel - Re: [PATCH 1/2] drm/bridge: ps8640: Skip redundant bridge enable

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]

Date:   Tue, 14 Mar 2023 14:30:38 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Pin-yen Lin <treapking@...omium.org>
Cc:     Andrzej Hajda <andrzej.hajda@...el.com>,
        Neil Armstrong <neil.armstrong@...aro.org>,
        Robert Foss <rfoss@...nel.org>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Jonas Karlman <jonas@...boo.se>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH 1/2] drm/bridge: ps8640: Skip redundant bridge enable

Hi,

On Tue, Mar 14, 2023 at 4:00 AM Pin-yen Lin <treapking@...omium.org> wrote:
>
> Skip the drm_bridge_chain_pre_enable call when the bridge is already
> pre_enabled. This make pre_enable and post_disable (thus
> pm_runtime_get/put) symmetric.
>
> Fixes: 46f206304db0 ("drm/bridge: ps8640: Rework power state handling")
> Signed-off-by: Pin-yen Lin <treapking@...omium.org>
> ---
>
>  drivers/gpu/drm/bridge/parade-ps8640.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 4b361d7d5e44..08de501c436e 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -557,7 +557,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
>          * EDID, for this chip, we need to do a full poweron, otherwise it will
>          * fail.
>          */
> -       drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
> +       if (poweroff)
> +               drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);

It always seemed weird to me that this function was asymmetric, so I
like this change, thanks!

I also remember wondering before how this function was safe, though.
The callpath for getting here from the ioctl is documented in the
function and when I look at it I wonder if anything is preventing the
bridge from being enabled / disabled through normal means at the same
time your function is running. That could cause all sorts of badness
if it is indeed possible. Does anyone reading this know if that's
indeed a problem?

I suppose that, if this is unsafe, it's no more unsafe now than it was
before your patch, so I guess:

Reviewed-by: Douglas Anderson <dianders@...omium.org>

If there are no issues, I'll plan to land this patch and the next one
to drm-misc-next sometime late-ish next week.