| Message ID | 20240420-mdp4-fixes-v1-1-96a70f64fa85@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/mdp4: fix probe deferral issues | expand |
On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote: > MSM display drivers provide kms structure allocated during probe(). > Don't clean up priv->kms field in case of an error. Otherwise probe > functions might fail after KMS probe deferral. > So just to understand this more, this will happen when master component probe (dpu) succeeded but other sub-component probe (dsi) deferred? Because if master component probe itself deferred it will allocate priv->kms again isnt it and we will not even hit here. > Fixes: a2ab5d5bb6b1 ("drm/msm: allow passing struct msm_kms to msm_drv_probe()") > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/msm_kms.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c > index af6a6fcb1173..6749f0fbca96 100644 > --- a/drivers/gpu/drm/msm/msm_kms.c > +++ b/drivers/gpu/drm/msm/msm_kms.c > @@ -244,7 +244,6 @@ int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv) > ret = priv->kms_init(ddev); > if (ret) { > DRM_DEV_ERROR(dev, "failed to load kms\n"); > - priv->kms = NULL; > return ret; > } > >
On Sat, Apr 20, 2024 at 04:02:00PM -0700, Abhinav Kumar wrote: > > > On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote: > > MSM display drivers provide kms structure allocated during probe(). > > Don't clean up priv->kms field in case of an error. Otherwise probe > > functions might fail after KMS probe deferral. > > > > So just to understand this more, this will happen when master component > probe (dpu) succeeded but other sub-component probe (dsi) deferred? > > Because if master component probe itself deferred it will allocate priv->kms > again isnt it and we will not even hit here. Master probing succeeds (so priv->kms is set), then kms_init fails at runtime, during binding of the master device. This results in probe deferral from the last component's component_add() function and reprobe attempt when possible (once the next device is added or probed). However as priv->kms is NULL, probe crashes. > > > Fixes: a2ab5d5bb6b1 ("drm/msm: allow passing struct msm_kms to msm_drv_probe()") > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/msm/msm_kms.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c > > index af6a6fcb1173..6749f0fbca96 100644 > > --- a/drivers/gpu/drm/msm/msm_kms.c > > +++ b/drivers/gpu/drm/msm/msm_kms.c > > @@ -244,7 +244,6 @@ int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv) > > ret = priv->kms_init(ddev); > > if (ret) { > > DRM_DEV_ERROR(dev, "failed to load kms\n"); > > - priv->kms = NULL; > > return ret; > > } > >
On 4/21/2024 3:35 PM, Dmitry Baryshkov wrote: > On Sat, Apr 20, 2024 at 04:02:00PM -0700, Abhinav Kumar wrote: >> >> >> On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote: >>> MSM display drivers provide kms structure allocated during probe(). >>> Don't clean up priv->kms field in case of an error. Otherwise probe >>> functions might fail after KMS probe deferral. >>> >> >> So just to understand this more, this will happen when master component >> probe (dpu) succeeded but other sub-component probe (dsi) deferred? >> >> Because if master component probe itself deferred it will allocate priv->kms >> again isnt it and we will not even hit here. > > Master probing succeeds (so priv->kms is set), then kms_init fails at > runtime, during binding of the master device. This results in probe > deferral from the last component's component_add() function and reprobe > attempt when possible (once the next device is added or probed). However > as priv->kms is NULL, probe crashes. > Got it, a better commit text would have helped here. Either way, Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
On 4/21/2024 3:35 PM, Dmitry Baryshkov wrote: > On Sat, Apr 20, 2024 at 04:02:00PM -0700, Abhinav Kumar wrote: >> >> >> On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote: >>> MSM display drivers provide kms structure allocated during probe(). >>> Don't clean up priv->kms field in case of an error. Otherwise probe >>> functions might fail after KMS probe deferral. >>> >> >> So just to understand this more, this will happen when master component >> probe (dpu) succeeded but other sub-component probe (dsi) deferred? >> >> Because if master component probe itself deferred it will allocate priv->kms >> again isnt it and we will not even hit here. > > Master probing succeeds (so priv->kms is set), then kms_init fails at > runtime, during binding of the master device. This results in probe > deferral from the last component's component_add() function and reprobe > attempt when possible (once the next device is added or probed). However > as priv->kms is NULL, probe crashes. > >> >>> Fixes: a2ab5d5bb6b1 ("drm/msm: allow passing struct msm_kms to msm_drv_probe()") Actually, Is this Fixes tag correct? OR is this one better Fixes: 506efcba3129 ("drm/msm: carve out KMS code from msm_drv.c") >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/msm_kms.c | 1 - >>> 1 file changed, 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c >>> index af6a6fcb1173..6749f0fbca96 100644 >>> --- a/drivers/gpu/drm/msm/msm_kms.c >>> +++ b/drivers/gpu/drm/msm/msm_kms.c >>> @@ -244,7 +244,6 @@ int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv) >>> ret = priv->kms_init(ddev); >>> if (ret) { >>> DRM_DEV_ERROR(dev, "failed to load kms\n"); >>> - priv->kms = NULL; >>> return ret; >>> } >>> >
On Mon, Apr 22, 2024 at 10:23:18AM -0700, Abhinav Kumar wrote: > > > On 4/21/2024 3:35 PM, Dmitry Baryshkov wrote: > > On Sat, Apr 20, 2024 at 04:02:00PM -0700, Abhinav Kumar wrote: > > > > > > > > > On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote: > > > > MSM display drivers provide kms structure allocated during probe(). > > > > Don't clean up priv->kms field in case of an error. Otherwise probe > > > > functions might fail after KMS probe deferral. > > > > > > > > > > So just to understand this more, this will happen when master component > > > probe (dpu) succeeded but other sub-component probe (dsi) deferred? > > > > > > Because if master component probe itself deferred it will allocate priv->kms > > > again isnt it and we will not even hit here. > > > > Master probing succeeds (so priv->kms is set), then kms_init fails at > > runtime, during binding of the master device. This results in probe > > deferral from the last component's component_add() function and reprobe > > attempt when possible (once the next device is added or probed). However > > as priv->kms is NULL, probe crashes. > > > > > > > > > Fixes: a2ab5d5bb6b1 ("drm/msm: allow passing struct msm_kms to msm_drv_probe()") > > Actually, Is this Fixes tag correct? > > OR is this one better > > Fixes: 506efcba3129 ("drm/msm: carve out KMS code from msm_drv.c") No. The issue existed even before the carve-out. > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > --- > > > > drivers/gpu/drm/msm/msm_kms.c | 1 - > > > > 1 file changed, 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c > > > > index af6a6fcb1173..6749f0fbca96 100644 > > > > --- a/drivers/gpu/drm/msm/msm_kms.c > > > > +++ b/drivers/gpu/drm/msm/msm_kms.c > > > > @@ -244,7 +244,6 @@ int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv) > > > > ret = priv->kms_init(ddev); > > > > if (ret) { > > > > DRM_DEV_ERROR(dev, "failed to load kms\n"); > > > > - priv->kms = NULL; > > > > return ret; > > > > } > > > > > >
On Mon, Apr 22, 2024 at 09:12:20AM -0700, Abhinav Kumar wrote: > > > On 4/21/2024 3:35 PM, Dmitry Baryshkov wrote: > > On Sat, Apr 20, 2024 at 04:02:00PM -0700, Abhinav Kumar wrote: > > > > > > > > > On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote: > > > > MSM display drivers provide kms structure allocated during probe(). > > > > Don't clean up priv->kms field in case of an error. Otherwise probe > > > > functions might fail after KMS probe deferral. > > > > > > > > > > So just to understand this more, this will happen when master component > > > probe (dpu) succeeded but other sub-component probe (dsi) deferred? > > > > > > Because if master component probe itself deferred it will allocate priv->kms > > > again isnt it and we will not even hit here. > > > > Master probing succeeds (so priv->kms is set), then kms_init fails at > > runtime, during binding of the master device. This results in probe > > deferral from the last component's component_add() function and reprobe > > attempt when possible (once the next device is added or probed). However > > as priv->kms is NULL, probe crashes. > > > > Got it, a better commit text would have helped here. Either way, I'll update the commit text with the text above. > > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c index af6a6fcb1173..6749f0fbca96 100644 --- a/drivers/gpu/drm/msm/msm_kms.c +++ b/drivers/gpu/drm/msm/msm_kms.c @@ -244,7 +244,6 @@ int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv) ret = priv->kms_init(ddev); if (ret) { DRM_DEV_ERROR(dev, "failed to load kms\n"); - priv->kms = NULL; return ret; }
MSM display drivers provide kms structure allocated during probe(). Don't clean up priv->kms field in case of an error. Otherwise probe functions might fail after KMS probe deferral. Fixes: a2ab5d5bb6b1 ("drm/msm: allow passing struct msm_kms to msm_drv_probe()") Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/msm_kms.c | 1 - 1 file changed, 1 deletion(-)