linux-kernel - Re: [PATCH v2] drm/bridge: analogix_dp: Grab runtime PM reference for DP-AUX

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: <cd453cd2-e23f-84b9-e7d3-667df2397c45@intel.com>
Date:   Tue, 11 Jan 2022 14:26:38 +0100
From:   Andrzej Hajda <andrzej.hajda@...el.com>
To:     Brian Norris <briannorris@...omium.org>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Neil Armstrong <narmstrong@...libre.com>
CC:     Sean Paul <sean@...rly.run>, Jonas Karlman <jonas@...boo.se>,
        <linux-kernel@...r.kernel.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        <dri-devel@...ts.freedesktop.org>,
        <linux-rockchip@...ts.infradead.org>, <stable@...r.kernel.org>,
        Tomeu Vizoso <tomeu.vizoso@...labora.com>,
        "Lyude Paul" <lyude@...hat.com>, Dave Airlie <airlied@...hat.com>,
        Ville Syrjälä <ville.syrjala@...ux.intel.com>
Subject: Re: [PATCH v2] drm/bridge: analogix_dp: Grab runtime PM reference for
 DP-AUX

Hi Brian,


I am not DP specialist so CC-ed people working with DP

On 01.10.2021 23:42, Brian Norris wrote:
> If the display is not enable()d, then we aren't holding a runtime PM
> reference here. Thus, it's easy to accidentally cause a hang, if user
> space is poking around at /dev/drm_dp_aux0 at the "wrong" time.
>
> Let's get the panel and PM state right before trying to talk AUX.
>
> Fixes: 0d97ad03f422 ("drm/bridge: analogix_dp: Remove duplicated code")
> Cc: <stable@...r.kernel.org>
> Cc: Tomeu Vizoso <tomeu.vizoso@...labora.com>
> Signed-off-by: Brian Norris <briannorris@...omium.org>


Few questions/issues here:

1. If it is just to avoid accidental 'hangs' it would be better to just 
check if the panel is working before transfer, if not, return error 
code. If there is better reason for this pm dance, please provide it  in 
description.

2. Again I see an assumption that panel-prepare enables power for 
something different than video transmission, accidentally it is true for 
most devices, but devices having more fine grained power management will 
break, or at least will be used inefficiently - but maybe in case of dp 
it is OK ???

3. More general issue - I am not sure if this should not be handled 
uniformly for all drm_dp devices.


Regards

Andrzej


> ---
>
> Changes in v2:
> - Fix spelling in Subject
> - DRM_DEV_ERROR() -> drm_err()
> - Propagate errors from un-analogix_dp_prepare_panel()
>
>   .../drm/bridge/analogix/analogix_dp_core.c    | 21 ++++++++++++++++++-
>   1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index b7d2e4449cfa..6fc46ac93ef8 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1632,8 +1632,27 @@ static ssize_t analogix_dpaux_transfer(struct drm_dp_aux *aux,
>   				       struct drm_dp_aux_msg *msg)
>   {
>   	struct analogix_dp_device *dp = to_dp(aux);
> +	int ret, ret2;
>   
> -	return analogix_dp_transfer(dp, msg);
> +	ret = analogix_dp_prepare_panel(dp, true, false);
> +	if (ret) {
> +		drm_err(dp->drm_dev, "Failed to prepare panel (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	pm_runtime_get_sync(dp->dev);
> +	ret = analogix_dp_transfer(dp, msg);
> +	pm_runtime_put(dp->dev);
> +
> +	ret2 = analogix_dp_prepare_panel(dp, false, false);
> +	if (ret2) {
> +		drm_err(dp->drm_dev, "Failed to unprepare panel (%d)\n", ret2);
> +		/* Prefer the analogix_dp_transfer() error, if it exists. */
> +		if (!ret)
> +			ret = ret2;
> +	}
> +
> +	return ret;
>   }
>   
>   struct analogix_dp_device *