| 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
| ||
|
Message-ID: <21041738-e23f-45bc-580b-4139c0cb87d9@linaro.org>
Date: Wed, 24 May 2023 11:52:14 +0200
From: Neil Armstrong <neil.armstrong@...aro.org>
To: Douglas Anderson <dianders@...omium.org>,
Jiri Kosina <jikos@...nel.org>,
Benjamin Tissoires <benjamin.tissoires@...hat.com>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Sam Ravnborg <sam@...nborg.org>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>
Cc: dri-devel@...ts.freedesktop.org, linux-input@...r.kernel.org,
Dmitry Torokhov <dmitry.torokhov@...il.com>, hsinyi@...gle.com,
devicetree@...r.kernel.org,
yangcong5@...qin.corp-partner.google.com,
linux-kernel@...r.kernel.org, Daniel Vetter <daniel@...ll.ch>,
linux-arm-msm@...r.kernel.org, cros-qcom-dts-watchers@...omium.org
Subject: Re: [PATCH 2/9] drm/panel: Check for already prepared/enabled in
drm_panel
Hi,
On 23/05/2023 21:27, Douglas Anderson wrote:
> In a whole pile of panel drivers, we have code to make the
> prepare/unprepare/enable/disable callbacks behave as no-ops if they've
> already been called. It's silly to have this code duplicated
> everywhere. Add it to the core instead so that we can eventually
> delete it from all the drivers. Note: to get some idea of the
> duplicated code, try:
> git grep 'if.*>prepared' -- drivers/gpu/drm/panel
> git grep 'if.*>enabled' -- drivers/gpu/drm/panel
>
> NOTE: arguably, the right thing to do here is actually to skip this
> patch and simply remove all the extra checks from the individual
> drivers. Perhaps the checks were needed at some point in time in the
> past but maybe they no longer are? Certainly as we continue
> transitioning over to "panel_bridge" then we expect there to be much
> less variety in how these calls are made. When we're called as part of
> the bridge chain, things should be pretty simple. In fact, there was
> some discussion in the past about these checks [1], including a
> discussion about whether the checks were needed and whether the calls
> ought to be refcounted. At the time, I decided not to mess with it
> because it felt too risky.
>
> Looking closer at it now, I'm fairly certain that nothing in the
> existing codebase is expecting these calls to be refcounted. The only
> real question is whether someone is already doing something to ensure
> prepare()/unprepare() match and enabled()/disable() match. I would say
> that, even if there is something else ensuring that things match,
> there's enough complexity that adding an extra bool and an extra
> double-check here is a good idea. Let's add a drm_warn() to let people
> know that it's considered a minor error to take advantage of
> drm_panel's double-checking but we'll still make things work fine.
>
> [1] https://lore.kernel.org/r/20210416153909.v4.27.I502f2a92ddd36c3d28d014dd75e170c2d405a0a5@changeid
>
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---
>
> drivers/gpu/drm/drm_panel.c | 49 ++++++++++++++++++++++++++++++++-----
> include/drm/drm_panel.h | 14 +++++++++++
> 2 files changed, 57 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index f634371c717a..4e1c4e42575b 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -105,11 +105,22 @@ EXPORT_SYMBOL(drm_panel_remove);
> */
> int drm_panel_prepare(struct drm_panel *panel)
> {
> + int ret;
> +
> if (!panel)
> return -EINVAL;
>
> - if (panel->funcs && panel->funcs->prepare)
> - return panel->funcs->prepare(panel);
> + if (panel->prepared) {
> + dev_warn(panel->dev, "Skipping prepare of already prepared panel\n");
> + return 0;
> + }
> +
> + if (panel->funcs && panel->funcs->prepare) {
> + ret = panel->funcs->prepare(panel);
> + if (ret < 0)
> + return ret;
> + }
> + panel->prepared = true;
>
> return 0;
> }
> @@ -128,11 +139,22 @@ EXPORT_SYMBOL(drm_panel_prepare);
> */
> int drm_panel_unprepare(struct drm_panel *panel)
> {
> + int ret;
> +
> if (!panel)
> return -EINVAL;
>
> - if (panel->funcs && panel->funcs->unprepare)
> - return panel->funcs->unprepare(panel);
> + if (!panel->prepared) {
> + dev_warn(panel->dev, "Skipping unprepare of already unprepared panel\n");
> + return 0;
> + }
> +
> + if (panel->funcs && panel->funcs->unprepare) {
> + ret = panel->funcs->unprepare(panel);
> + if (ret < 0)
> + return ret;
> + }
> + panel->prepared = false;
>
> return 0;
> }
> @@ -155,11 +177,17 @@ int drm_panel_enable(struct drm_panel *panel)
> if (!panel)
> return -EINVAL;
>
> + if (panel->enabled) {
> + dev_warn(panel->dev, "Skipping enable of already enabled panel\n");
> + return 0;
> + }
> +
> if (panel->funcs && panel->funcs->enable) {
> ret = panel->funcs->enable(panel);
> if (ret < 0)
> return ret;
> }
> + panel->enabled = true;
>
> ret = backlight_enable(panel->backlight);
> if (ret < 0)
> @@ -187,13 +215,22 @@ int drm_panel_disable(struct drm_panel *panel)
> if (!panel)
> return -EINVAL;
>
> + if (!panel->enabled) {
> + dev_warn(panel->dev, "Skipping disable of already disabled panel\n");
> + return 0;
> + }
> +
> ret = backlight_disable(panel->backlight);
> if (ret < 0)
> DRM_DEV_INFO(panel->dev, "failed to disable backlight: %d\n",
> ret);
>
> - if (panel->funcs && panel->funcs->disable)
> - return panel->funcs->disable(panel);
> + if (panel->funcs && panel->funcs->disable) {
> + ret = panel->funcs->disable(panel);
> + if (ret < 0)
> + return ret;
> + }
> + panel->enabled = false;
>
> return 0;
> }
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 432fab2347eb..c6cf75909389 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h
> @@ -198,6 +198,20 @@ struct drm_panel {
> * the panel is powered up.
> */
> bool prepare_prev_first;
> +
> + /**
> + * @prepared:
> + *
> + * If true then the panel has been prepared.
> + */
> + bool prepared;
> +
> + /**
> + * @enabled:
> + *
> + * If true then the panel has been enabled.
> + */
> + bool enabled;
> };
>
> void drm_panel_init(struct drm_panel *panel, struct device *dev,
LGTM and let's cleanup the panel drivers
Acked-by: Neil Armstrong <neil.armstrong@...aro.org>
Powered by blists - more mailing lists