Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230710085922.1871465-1-wenst@chromium.org>
In-Reply-To: <20230710085922.1871465-1-wenst@chromium.org>
From:   Robert Foss <rfoss@kernel.org>
Date:   Mon, 17 Jul 2023 17:57:20 +0200
Message-ID: <CAN6tsi508WALxzza5VEjm5wiwZrafJa32=+R8SS4vmg-ybqHig@mail.gmail.com>
Subject: Re: [PATCH v2] drm/bridge: anx7625: Drop device lock before drm_helper_hpd_irq_event()
To:     Chen-Yu Tsai <wenst@chromium.org>
Cc:     Andrzej Hajda <andrzej.hajda@intel.com>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        Xin Ji <xji@analogixsemi.com>,
        =?UTF-8?B?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= 
        <nfraprado@collabora.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jul 10, 2023 at 10:59=E2=80=AFAM Chen-Yu Tsai <wenst@chromium.org> =
wrote:
>
> The device lock is used to serialize the low level power sequencing
> operations. Since drm_helper_hpd_irq_event() could end up calling
> .atomic_enable, which also calls power sequencing functions through
> runtime PM, this results in a real deadlock. This was observed on an
> MT8192-based Chromebook's external display (with appropriate patches [1]
> and DT changes applied).
>
> Move the drm_helper_hpd_irq_event() call outside of the lock range. The
> lock only needs to be held so that the device status can be read back.
> This is the bare minimum change to avoid the deadlock. The lock could
> be dropped completely and have pm_runtime_get_if_in_use() increase the
> reference count, but this is not the same as pm_runtime_suspended().
>
> Dropping the lock completely also causes the internal display of the
> same device to not function correctly if the internal bridge's
> interrupt line is added in the device tree. Both the internal and
> external display of said device each use one anx7625 bridge.
>
> [1] https://lore.kernel.org/dri-devel/20230112042104.4107253-1-treapking@=
chromium.org/
>
> Fixes: 60487584a79a ("drm/bridge: anx7625: refactor power control to use =
runtime PM framework")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> Changes since v1:
> - restore early return if event < 0
>
>  drivers/gpu/drm/bridge/analogix/anx7625.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/=
bridge/analogix/anx7625.c
> index 866d018f4bb1..e93eba89d5ee 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -1593,18 +1593,20 @@ static void anx7625_work_func(struct work_struct =
*work)
>
>         mutex_lock(&ctx->lock);
>
> -       if (pm_runtime_suspended(&ctx->client->dev))
> -               goto unlock;
> +       if (pm_runtime_suspended(&ctx->client->dev)) {
> +               mutex_unlock(&ctx->lock);
> +               return;
> +       }
>
>         event =3D anx7625_hpd_change_detect(ctx);
> +
> +       mutex_unlock(&ctx->lock);
> +
>         if (event < 0)
> -               goto unlock;
> +               return;
>
>         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)
> --
> 2.41.0.255.g8b1d071c50-goog
>

LGTM, let's snooze this until next week, incase someone comes up with an is=
sue.

Reviewed-by: Robert Foss <rfoss@kernel.org>