| 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: <CAJMQK-jehJf+DE+bZxfA=zqUC-=mr6Q15oNcDp9j-7ER_dbFog@mail.gmail.com>
Date: Fri, 7 May 2021 20:15:04 +0800
From: Hsin-Yi Wang <hsinyi@...omium.org>
To: Pi-Hsun Shih <pihsun@...omium.org>
Cc: Tzung-Bi Shih <tzungbi@...gle.com>,
Andrzej Hajda <a.hajda@...sung.com>,
Neil Armstrong <narmstrong@...libre.com>,
Robert Foss <robert.foss@...aro.org>,
Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Jonas Karlman <jonas@...boo.se>,
Jernej Skrabec <jernej.skrabec@...l.net>,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>, Xin Ji <xji@...logixsemi.com>,
Sam Ravnborg <sam@...nborg.org>,
"open list:DRM DRIVERS" <dri-devel@...ts.freedesktop.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] drm/bridge: anx7625: refactor power control to use
runtime PM framework
On Fri, May 7, 2021 at 2:44 PM Pi-Hsun Shih <pihsun@...omium.org> wrote:
>
> The driver originally use an atomic_t for keep track of the power
> status, which makes the driver more complicated than needed, and has
> some race condition as it's possible to have the power on and power off
> sequence going at the same time.
>
> This patch remove the usage of the atomic_t power_status, and use the
> kernel runtime power management framework instead.
>
> Signed-off-by: Pi-Hsun Shih <pihsun@...omium.org>
> ---
> drivers/gpu/drm/bridge/analogix/anx7625.c | 148 +++++++++-------------
> drivers/gpu/drm/bridge/analogix/anx7625.h | 1 -
> 2 files changed, 63 insertions(+), 86 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index 23283ba0c4f9..f56f8cf1f3bd 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -11,6 +11,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> +#include <linux/pm_runtime.h>
> #include <linux/regulator/consumer.h>
> #include <linux/slab.h>
> #include <linux/types.h>
> @@ -1005,33 +1006,6 @@ static void anx7625_power_on_init(struct anx7625_data *ctx)
> }
> }
>
> -static void anx7625_chip_control(struct anx7625_data *ctx, int state)
> -{
> - struct device *dev = &ctx->client->dev;
> -
> - DRM_DEV_DEBUG_DRIVER(dev, "before set, power_state(%d).\n",
> - atomic_read(&ctx->power_status));
> -
> - if (!ctx->pdata.low_power_mode)
> - return;
> -
> - if (state) {
> - atomic_inc(&ctx->power_status);
> - if (atomic_read(&ctx->power_status) == 1)
> - anx7625_power_on_init(ctx);
> - } else {
> - if (atomic_read(&ctx->power_status)) {
> - atomic_dec(&ctx->power_status);
> -
> - if (atomic_read(&ctx->power_status) == 0)
> - anx7625_power_standby(ctx);
> - }
> - }
> -
> - DRM_DEV_DEBUG_DRIVER(dev, "after set, power_state(%d).\n",
> - atomic_read(&ctx->power_status));
> -}
> -
> static void anx7625_init_gpio(struct anx7625_data *platform)
> {
> struct device *dev = &platform->client->dev;
> @@ -1061,9 +1035,6 @@ static void anx7625_stop_dp_work(struct anx7625_data *ctx)
> ctx->hpd_status = 0;
> ctx->hpd_high_cnt = 0;
> ctx->display_timing_valid = 0;
> -
> - if (ctx->pdata.low_power_mode == 0)
> - anx7625_disable_pd_protocol(ctx);
> }
>
> static void anx7625_start_dp_work(struct anx7625_data *ctx)
> @@ -1105,49 +1076,26 @@ static void anx7625_hpd_polling(struct anx7625_data *ctx)
> int ret, val;
> struct device *dev = &ctx->client->dev;
>
> - if (atomic_read(&ctx->power_status) != 1) {
> - DRM_DEV_DEBUG_DRIVER(dev, "No need to poling HPD status.\n");
> - return;
> - }
> -
> ret = readx_poll_timeout(anx7625_read_hpd_status_p0,
> ctx, val,
> ((val & HPD_STATUS) || (val < 0)),
> 5000,
> 5000 * 100);
> if (ret) {
> - DRM_DEV_ERROR(dev, "HPD polling timeout!\n");
> - } else {
> - DRM_DEV_DEBUG_DRIVER(dev, "HPD raise up.\n");
> - anx7625_reg_write(ctx, ctx->i2c.tcpc_client,
> - INTR_ALERT_1, 0xFF);
> - anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> - INTERFACE_CHANGE_INT, 0);
> + DRM_DEV_ERROR(dev, "no hpd.\n");
> + return;
> }
>
> - anx7625_start_dp_work(ctx);
> -}
> -
> -static void anx7625_disconnect_check(struct anx7625_data *ctx)
> -{
> - if (atomic_read(&ctx->power_status) == 0)
> - anx7625_stop_dp_work(ctx);
> -}
> -
> -static void anx7625_low_power_mode_check(struct anx7625_data *ctx,
> - int state)
> -{
> - struct device *dev = &ctx->client->dev;
> + DRM_DEV_DEBUG_DRIVER(dev, "system status: 0x%x. HPD raise up.\n", val);
> + anx7625_reg_write(ctx, ctx->i2c.tcpc_client,
> + INTR_ALERT_1, 0xFF);
> + anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> + INTERFACE_CHANGE_INT, 0);
>
> - DRM_DEV_DEBUG_DRIVER(dev, "low power mode check, state(%d).\n", state);
> + anx7625_start_dp_work(ctx);
>
> - if (ctx->pdata.low_power_mode) {
> - anx7625_chip_control(ctx, state);
> - if (state)
> - anx7625_hpd_polling(ctx);
> - else
> - anx7625_disconnect_check(ctx);
> - }
> + if (!ctx->pdata.panel_bridge && ctx->bridge_attached)
> + drm_helper_hpd_irq_event(ctx->bridge.dev);
> }
>
> static void anx7625_remove_edid(struct anx7625_data *ctx)
> @@ -1180,9 +1128,6 @@ static int anx7625_hpd_change_detect(struct anx7625_data *ctx)
> int intr_vector, status;
> struct device *dev = &ctx->client->dev;
>
> - DRM_DEV_DEBUG_DRIVER(dev, "power_status=%d\n",
> - (u32)atomic_read(&ctx->power_status));
> -
> status = anx7625_reg_write(ctx, ctx->i2c.tcpc_client,
> INTR_ALERT_1, 0xFF);
> if (status < 0) {
> @@ -1228,22 +1173,25 @@ static void anx7625_work_func(struct work_struct *work)
> struct anx7625_data, work);
>
> mutex_lock(&ctx->lock);
> +
> + if (pm_runtime_suspended(&ctx->client->dev))
> + goto unlock;
> +
> event = anx7625_hpd_change_detect(ctx);
> - mutex_unlock(&ctx->lock);
> if (event < 0)
> - return;
> + goto unlock;
>
> if (ctx->bridge_attached)
> drm_helper_hpd_irq_event(ctx->bridge.dev);
> +
> +unlock:
> + mutex_unlock(&ctx->lock);
> }
>
> static irqreturn_t anx7625_intr_hpd_isr(int irq, void *data)
> {
> struct anx7625_data *ctx = (struct anx7625_data *)data;
>
> - if (atomic_read(&ctx->power_status) != 1)
> - return IRQ_NONE;
> -
> queue_work(ctx->workqueue, &ctx->work);
>
> return IRQ_HANDLED;
> @@ -1305,9 +1253,9 @@ static struct edid *anx7625_get_edid(struct anx7625_data *ctx)
> return (struct edid *)edid;
> }
>
> - anx7625_low_power_mode_check(ctx, 1);
> + pm_runtime_get_sync(dev);
> edid_num = sp_tx_edid_read(ctx, p_edid->edid_raw_data);
> - anx7625_low_power_mode_check(ctx, 0);
> + pm_runtime_put(dev);
>
> if (edid_num < 1) {
> DRM_DEV_ERROR(dev, "Fail to read EDID: %d\n", edid_num);
> @@ -1611,10 +1559,7 @@ static void anx7625_bridge_enable(struct drm_bridge *bridge)
>
> DRM_DEV_DEBUG_DRIVER(dev, "drm enable\n");
>
> - anx7625_low_power_mode_check(ctx, 1);
> -
> - if (WARN_ON(!atomic_read(&ctx->power_status)))
> - return;
> + pm_runtime_get_sync(dev);
>
> anx7625_dp_start(ctx);
> }
> @@ -1624,14 +1569,11 @@ static void anx7625_bridge_disable(struct drm_bridge *bridge)
> struct anx7625_data *ctx = bridge_to_anx7625(bridge);
> struct device *dev = &ctx->client->dev;
>
> - if (WARN_ON(!atomic_read(&ctx->power_status)))
> - return;
> -
> DRM_DEV_DEBUG_DRIVER(dev, "drm disable\n");
>
> anx7625_dp_stop(ctx);
>
> - anx7625_low_power_mode_check(ctx, 0);
> + pm_runtime_put(dev);
> }
>
> static enum drm_connector_status
> @@ -1735,6 +1677,39 @@ static void anx7625_unregister_i2c_dummy_clients(struct anx7625_data *ctx)
> i2c_unregister_device(ctx->i2c.tcpc_client);
> }
>
> +static int __maybe_unused anx7625_runtime_pm_suspend(struct device *dev)
> +{
> + struct anx7625_data *ctx = dev_get_drvdata(dev);
> +
> + mutex_lock(&ctx->lock);
> +
> + anx7625_stop_dp_work(ctx);
> + anx7625_power_standby(ctx);
> +
> + mutex_unlock(&ctx->lock);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused anx7625_runtime_pm_resume(struct device *dev)
> +{
> + struct anx7625_data *ctx = dev_get_drvdata(dev);
> +
> + mutex_lock(&ctx->lock);
> +
> + anx7625_power_on_init(ctx);
> + anx7625_hpd_polling(ctx);
> +
> + mutex_unlock(&ctx->lock);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops anx7625_pm_ops = {
> + SET_RUNTIME_PM_OPS(anx7625_runtime_pm_suspend,
> + anx7625_runtime_pm_resume, NULL)
> +};
> +
.pm = &anx7625_pm_ops, is missing in static struct i2c_driver
anx7625_driver{...}
> static int anx7625_i2c_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -1778,8 +1753,6 @@ static int anx7625_i2c_probe(struct i2c_client *client,
> }
> anx7625_init_gpio(platform);
>
> - atomic_set(&platform->power_status, 0);
> -
> mutex_init(&platform->lock);
>
> platform->pdata.intp_irq = client->irq;
> @@ -1809,9 +1782,11 @@ static int anx7625_i2c_probe(struct i2c_client *client,
> goto free_wq;
> }
>
> - if (platform->pdata.low_power_mode == 0) {
> + pm_runtime_enable(dev);
> +
> + if (!platform->pdata.low_power_mode) {
> anx7625_disable_pd_protocol(platform);
> - atomic_set(&platform->power_status, 1);
> + pm_runtime_get_sync(dev);
> }
>
> /* Add work function */
> @@ -1847,6 +1822,9 @@ static int anx7625_i2c_remove(struct i2c_client *client)
> if (platform->pdata.intp_irq)
> destroy_workqueue(platform->workqueue);
>
> + if (!platform->pdata.low_power_mode)
> + pm_runtime_put_sync_suspend(&client->dev);
> +
> anx7625_unregister_i2c_dummy_clients(platform);
>
> kfree(platform);
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
> index e4a086b3a3d7..034c3840028f 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> @@ -369,7 +369,6 @@ struct anx7625_i2c_client {
>
> struct anx7625_data {
> struct anx7625_platform_data pdata;
> - atomic_t power_status;
> int hpd_status;
> int hpd_high_cnt;
> /* Lock for work queue */
>
> base-commit: e48661230cc35b3d0f4367eddfc19f86463ab917
> --
> 2.31.1.607.g51e8a6a459-goog
>
Powered by blists - more mailing lists