| Message ID | 20220505214051.155-2-quic_jesszhan@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v3,1/2] drm/msm/mdp5: Return error code in mdp5_pipe_release when deadlock is detected | expand |
On Thu, May 5, 2022 at 2:41 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > > There is a possibility for mdp5_get_global_state to return > -EDEADLK when acquiring the modeset lock, but currently global_state in > mdp5_mixer_release doesn't check for if an error is returned. > > To avoid a NULL dereference error, let's have mdp5_mixer_release > check if an error is returned and propagate that error. > > Reported-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> Fixes: 7907a0d77cb4 ("drm/msm/mdp5: Use the new private_obj state") Reviewed-by: Rob Clark <robdclark@gmail.com> > --- > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 10 ++++++++-- > drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c | 15 +++++++++++---- > drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h | 4 ++-- > 3 files changed, 21 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > index b966cd69f99d..fe2922c8d21b 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > @@ -612,9 +612,15 @@ static int mdp5_crtc_setup_pipeline(struct drm_crtc *crtc, > if (ret) > return ret; > > - mdp5_mixer_release(new_crtc_state->state, old_mixer); > + ret = mdp5_mixer_release(new_crtc_state->state, old_mixer); > + if (ret) > + return ret; > + > if (old_r_mixer) { > - mdp5_mixer_release(new_crtc_state->state, old_r_mixer); > + ret = mdp5_mixer_release(new_crtc_state->state, old_r_mixer); > + if (ret) > + return ret; > + > if (!need_right_mixer) > pipeline->r_mixer = NULL; > } > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c > index 954db683ae44..2536def2a000 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c > @@ -116,21 +116,28 @@ int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc, > return 0; > } > > -void mdp5_mixer_release(struct drm_atomic_state *s, struct mdp5_hw_mixer *mixer) > +int mdp5_mixer_release(struct drm_atomic_state *s, struct mdp5_hw_mixer *mixer) > { > struct mdp5_global_state *global_state = mdp5_get_global_state(s); > - struct mdp5_hw_mixer_state *new_state = &global_state->hwmixer; > + struct mdp5_hw_mixer_state *new_state; > > if (!mixer) > - return; > + return 0; > + > + if (IS_ERR(global_state)) > + return PTR_ERR(global_state); > + > + new_state = &global_state->hwmixer; > > if (WARN_ON(!new_state->hwmixer_to_crtc[mixer->idx])) > - return; > + return -EINVAL; > > DBG("%s: release from crtc %s", mixer->name, > new_state->hwmixer_to_crtc[mixer->idx]->name); > > new_state->hwmixer_to_crtc[mixer->idx] = NULL; > + > + return 0; > } > > void mdp5_mixer_destroy(struct mdp5_hw_mixer *mixer) > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h > index 43c9ba43ce18..545ee223b9d7 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h > @@ -30,7 +30,7 @@ void mdp5_mixer_destroy(struct mdp5_hw_mixer *lm); > int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc, > uint32_t caps, struct mdp5_hw_mixer **mixer, > struct mdp5_hw_mixer **r_mixer); > -void mdp5_mixer_release(struct drm_atomic_state *s, > - struct mdp5_hw_mixer *mixer); > +int mdp5_mixer_release(struct drm_atomic_state *s, > + struct mdp5_hw_mixer *mixer); > > #endif /* __MDP5_LM_H__ */ > -- > 2.35.1 >
On 06/05/2022 00:40, Jessica Zhang wrote: > There is a possibility for mdp5_get_global_state to return > -EDEADLK when acquiring the modeset lock, but currently global_state in > mdp5_mixer_release doesn't check for if an error is returned. > > To avoid a NULL dereference error, let's have mdp5_mixer_release > check if an error is returned and propagate that error. > > Reported-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 10 ++++++++-- > drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c | 15 +++++++++++---- > drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h | 4 ++-- > 3 files changed, 21 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > index b966cd69f99d..fe2922c8d21b 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > @@ -612,9 +612,15 @@ static int mdp5_crtc_setup_pipeline(struct drm_crtc *crtc, > if (ret) > return ret; > > - mdp5_mixer_release(new_crtc_state->state, old_mixer); > + ret = mdp5_mixer_release(new_crtc_state->state, old_mixer); > + if (ret) > + return ret; > + > if (old_r_mixer) { > - mdp5_mixer_release(new_crtc_state->state, old_r_mixer); > + ret = mdp5_mixer_release(new_crtc_state->state, old_r_mixer); > + if (ret) > + return ret; > + > if (!need_right_mixer) > pipeline->r_mixer = NULL; > } > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c > index 954db683ae44..2536def2a000 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c > @@ -116,21 +116,28 @@ int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc, > return 0; > } > > -void mdp5_mixer_release(struct drm_atomic_state *s, struct mdp5_hw_mixer *mixer) > +int mdp5_mixer_release(struct drm_atomic_state *s, struct mdp5_hw_mixer *mixer) > { > struct mdp5_global_state *global_state = mdp5_get_global_state(s); > - struct mdp5_hw_mixer_state *new_state = &global_state->hwmixer; > + struct mdp5_hw_mixer_state *new_state; > > if (!mixer) > - return; > + return 0; > + > + if (IS_ERR(global_state)) > + return PTR_ERR(global_state); > + > + new_state = &global_state->hwmixer; > > if (WARN_ON(!new_state->hwmixer_to_crtc[mixer->idx])) > - return; > + return -EINVAL; > > DBG("%s: release from crtc %s", mixer->name, > new_state->hwmixer_to_crtc[mixer->idx]->name); > > new_state->hwmixer_to_crtc[mixer->idx] = NULL; > + > + return 0; > } > > void mdp5_mixer_destroy(struct mdp5_hw_mixer *mixer) > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h > index 43c9ba43ce18..545ee223b9d7 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h > @@ -30,7 +30,7 @@ void mdp5_mixer_destroy(struct mdp5_hw_mixer *lm); > int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc, > uint32_t caps, struct mdp5_hw_mixer **mixer, > struct mdp5_hw_mixer **r_mixer); > -void mdp5_mixer_release(struct drm_atomic_state *s, > - struct mdp5_hw_mixer *mixer); > +int mdp5_mixer_release(struct drm_atomic_state *s, > + struct mdp5_hw_mixer *mixer); > > #endif /* __MDP5_LM_H__ */
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c index b966cd69f99d..fe2922c8d21b 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c @@ -612,9 +612,15 @@ static int mdp5_crtc_setup_pipeline(struct drm_crtc *crtc, if (ret) return ret; - mdp5_mixer_release(new_crtc_state->state, old_mixer); + ret = mdp5_mixer_release(new_crtc_state->state, old_mixer); + if (ret) + return ret; + if (old_r_mixer) { - mdp5_mixer_release(new_crtc_state->state, old_r_mixer); + ret = mdp5_mixer_release(new_crtc_state->state, old_r_mixer); + if (ret) + return ret; + if (!need_right_mixer) pipeline->r_mixer = NULL; } diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c index 954db683ae44..2536def2a000 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c @@ -116,21 +116,28 @@ int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc, return 0; } -void mdp5_mixer_release(struct drm_atomic_state *s, struct mdp5_hw_mixer *mixer) +int mdp5_mixer_release(struct drm_atomic_state *s, struct mdp5_hw_mixer *mixer) { struct mdp5_global_state *global_state = mdp5_get_global_state(s); - struct mdp5_hw_mixer_state *new_state = &global_state->hwmixer; + struct mdp5_hw_mixer_state *new_state; if (!mixer) - return; + return 0; + + if (IS_ERR(global_state)) + return PTR_ERR(global_state); + + new_state = &global_state->hwmixer; if (WARN_ON(!new_state->hwmixer_to_crtc[mixer->idx])) - return; + return -EINVAL; DBG("%s: release from crtc %s", mixer->name, new_state->hwmixer_to_crtc[mixer->idx]->name); new_state->hwmixer_to_crtc[mixer->idx] = NULL; + + return 0; } void mdp5_mixer_destroy(struct mdp5_hw_mixer *mixer) diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h index 43c9ba43ce18..545ee223b9d7 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h @@ -30,7 +30,7 @@ void mdp5_mixer_destroy(struct mdp5_hw_mixer *lm); int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc, uint32_t caps, struct mdp5_hw_mixer **mixer, struct mdp5_hw_mixer **r_mixer); -void mdp5_mixer_release(struct drm_atomic_state *s, - struct mdp5_hw_mixer *mixer); +int mdp5_mixer_release(struct drm_atomic_state *s, + struct mdp5_hw_mixer *mixer); #endif /* __MDP5_LM_H__ */
There is a possibility for mdp5_get_global_state to return -EDEADLK when acquiring the modeset lock, but currently global_state in mdp5_mixer_release doesn't check for if an error is returned. To avoid a NULL dereference error, let's have mdp5_mixer_release check if an error is returned and propagate that error. Reported-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> --- drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 10 ++++++++-- drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c | 15 +++++++++++---- drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h | 4 ++-- 3 files changed, 21 insertions(+), 8 deletions(-)