Re: [PATCH v3 1/2] drm/msm/mdp5: Return error code in mdp5_pipe_release when deadlock is detected

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 06/05/2022 01:01, Rob Clark wrote:
On Thu, May 5, 2022 at 2:41 PM Jessica Zhang <quic_jesszhan@xxxxxxxxxxx> wrote:

mdp5_get_global_state runs the risk of hitting a -EDEADLK when acquiring
the modeset lock, but currently mdp5_pipe_release doesn't check for if
an error is returned. Because of this, there is a possibility of
mdp5_pipe_release hitting a NULL dereference error.

To avoid this, let's have mdp5_pipe_release check if
mdp5_get_global_state returns an error and propogate that error.

Changes since v1:
- Separated declaration and initialization of *new_state to avoid
   compiler warning
- Fixed some spelling mistakes in commit message

Changes since v2:
- Return 0 in case where hwpipe is NULL as this is considered normal
   behavior
- Added 2nd patch in series to fix a similar NULL dereference issue in
   mdp5_mixer_release

Reported-by: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>

Fixes: 7907a0d77cb4 ("drm/msm/mdp5: Use the new private_obj state")
Reviewed-by: Rob Clark <robdclark@xxxxxxxxx>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>

---
  drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c  | 15 +++++++++++----
  drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h  |  2 +-
  drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 20 ++++++++++++++++----
  3 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
index ba6695963aa6..a4f5cb90f3e8 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
@@ -119,18 +119,23 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct drm_plane *plane,
         return 0;
  }

-void mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe *hwpipe)
+int mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe *hwpipe)
  {
         struct msm_drm_private *priv = s->dev->dev_private;
         struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
         struct mdp5_global_state *state = mdp5_get_global_state(s);
-       struct mdp5_hw_pipe_state *new_state = &state->hwpipe;
+       struct mdp5_hw_pipe_state *new_state;

         if (!hwpipe)
-               return;
+               return 0;
+
+       if (IS_ERR(state))
+               return PTR_ERR(state);
+
+       new_state = &state->hwpipe;

         if (WARN_ON(!new_state->hwpipe_to_plane[hwpipe->idx]))
-               return;
+               return -EINVAL;

         DBG("%s: release from plane %s", hwpipe->name,
                 new_state->hwpipe_to_plane[hwpipe->idx]->name);
@@ -141,6 +146,8 @@ void mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe *hwpipe)
         }

         new_state->hwpipe_to_plane[hwpipe->idx] = NULL;
+
+       return 0;
  }

  void mdp5_pipe_destroy(struct mdp5_hw_pipe *hwpipe)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h
index 9b26d0761bd4..cca67938cab2 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h
@@ -37,7 +37,7 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct drm_plane *plane,
                      uint32_t caps, uint32_t blkcfg,
                      struct mdp5_hw_pipe **hwpipe,
                      struct mdp5_hw_pipe **r_hwpipe);
-void mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe *hwpipe);
+int mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe *hwpipe);

  struct mdp5_hw_pipe *mdp5_pipe_init(enum mdp5_pipe pipe,
                 uint32_t reg_offset, uint32_t caps);
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
index 228b22830970..979458482841 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
@@ -311,12 +311,24 @@ static int mdp5_plane_atomic_check_with_state(struct drm_crtc_state *crtc_state,
                                 mdp5_state->r_hwpipe = NULL;


-                       mdp5_pipe_release(state->state, old_hwpipe);
-                       mdp5_pipe_release(state->state, old_right_hwpipe);
+                       ret = mdp5_pipe_release(state->state, old_hwpipe);
+                       if (ret)
+                               return ret;
+
+                       ret = mdp5_pipe_release(state->state, old_right_hwpipe);
+                       if (ret)
+                               return ret;
+
                 }
         } else {
-               mdp5_pipe_release(state->state, mdp5_state->hwpipe);
-               mdp5_pipe_release(state->state, mdp5_state->r_hwpipe);
+               ret = mdp5_pipe_release(state->state, mdp5_state->hwpipe);
+               if (ret)
+                       return ret;
+
+               ret = mdp5_pipe_release(state->state, mdp5_state->r_hwpipe);
+               if (ret)
+                       return ret;
+
                 mdp5_state->hwpipe = mdp5_state->r_hwpipe = NULL;
         }

--
2.35.1



--
With best wishes
Dmitry



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux