Hello Maxime,
On 10/24/22 11:36, maxime@xxxxxxxxxx wrote:
> We access some fields protected by our internal mutex in
> vc4_hdmi_reset_link() (saved_adjusted_mode, output_bpc, output_format)
> and are calling functions that need to have that lock taken
> (vc4_hdmi_supports_scrambling()).
>
> However, the current code doesn't lock that mutex. Let's make sure it
> does.
>
> Fixes: 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug")
> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> ---
Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
Just a trivial nit below:
> drivers/gpu/drm/vc4/vc4_hdmi.c | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 596e311d6e58..9e549fa07467 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -349,27 +349,40 @@ static int vc4_hdmi_reset_link(struct drm_connector *connector,
> if (!crtc_state->active)
> return 0;
>
> - if (!vc4_hdmi_supports_scrambling(encoder))
> + mutex_lock(&vc4_hdmi->mutex);
> +
> + if (!vc4_hdmi_supports_scrambling(encoder)) {
> + mutex_unlock(&vc4_hdmi->mutex);
> return 0;
> + }
>
> scrambling_needed = vc4_hdmi_mode_needs_scrambling(&vc4_hdmi->saved_adjusted_mode,
> vc4_hdmi->output_bpc,
> vc4_hdmi->output_format);
> - if (!scrambling_needed)
> + if (!scrambling_needed) {
> + mutex_unlock(&vc4_hdmi->mutex);
> return 0;
> + }
>
> if (conn_state->commit &&
> - !try_wait_for_completion(&conn_state->commit->hw_done))
> + !try_wait_for_completion(&conn_state->commit->hw_done)) {
> + mutex_unlock(&vc4_hdmi->mutex);
> return 0;
> + }
I think the convention is to have a ret_unlock label followed by the
mutex_unlock() and ret 0, to avoid duplicating this. You could have
that label after the return reset_pipe(crtc, ctx).
But it's OK if you prefer to land this version too.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
