| Message ID | 20200221210319.2245170-42-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm managed resources, v2 | expand |
On 21/02/2020 23:03, Daniel Vetter wrote: > It's right above the drm_dev_put(). > > This is made possible by a preceeding patch which added a drmm_ > cleanup action to drm_mode_config_init(), hence all we need to do to > ensure that drm_mode_config_cleanup() is run on final drm_device > cleanup is check the new error code for _init(). > > Aside: Another driver with a bit much devm_kzalloc, which should > probably use drmm_kzalloc instead ... > > I'm pretty sure this one blows up already under KASAN because it's > using devm_drm_dev_init, and later on devm_kzalloc. Hence the memory > will get freed before the final drm_dev_put (all from the devres > code), but the cleanup in that final drm_dev_put will access the just > freed memory. > > Unfortunately fixing this properly needs slightly more work, namely > drmm_ versions for all the drm objects (planes, crtc, ...), so that > the cleanup actually happens before even drmm_kzalloc would release > the underlying memory. Not quite there yet. > > v2: Explain why this cleanup is possible (Laurent). > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Jyri Sarha <jsarha@ti.com> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> Acked-by: Jyri Sarha <jsarha@ti.com> > --- > drivers/gpu/drm/tidss/tidss_drv.c | 4 ---- > drivers/gpu/drm/tidss/tidss_kms.c | 19 +++++-------------- > drivers/gpu/drm/tidss/tidss_kms.h | 1 - > 3 files changed, 5 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c > index 460d5e9d0cf4..ad449d104306 100644 > --- a/drivers/gpu/drm/tidss/tidss_drv.c > +++ b/drivers/gpu/drm/tidss/tidss_drv.c > @@ -103,11 +103,7 @@ static const struct dev_pm_ops tidss_pm_ops = { > > static void tidss_release(struct drm_device *ddev) > { > - struct tidss_device *tidss = ddev->dev_private; > - > drm_kms_helper_poll_fini(ddev); > - > - tidss_modeset_cleanup(tidss); > } > > DEFINE_DRM_GEM_CMA_FOPS(tidss_fops); > diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c > index 5311e0f1c551..87e07e0e4eae 100644 > --- a/drivers/gpu/drm/tidss/tidss_kms.c > +++ b/drivers/gpu/drm/tidss/tidss_kms.c > @@ -208,7 +208,9 @@ int tidss_modeset_init(struct tidss_device *tidss) > > dev_dbg(tidss->dev, "%s\n", __func__); > > - drm_mode_config_init(ddev); > + ret = drm_mode_config_init(ddev); > + if (ret) > + return ret; > > ddev->mode_config.min_width = 8; > ddev->mode_config.min_height = 8; > @@ -220,11 +222,11 @@ int tidss_modeset_init(struct tidss_device *tidss) > > ret = tidss_dispc_modeset_init(tidss); > if (ret) > - goto err_mode_config_cleanup; > + return ret; > > ret = drm_vblank_init(ddev, tidss->num_crtcs); > if (ret) > - goto err_mode_config_cleanup; > + return ret; > > /* Start with vertical blanking interrupt reporting disabled. */ > for (i = 0; i < tidss->num_crtcs; ++i) > @@ -235,15 +237,4 @@ int tidss_modeset_init(struct tidss_device *tidss) > dev_dbg(tidss->dev, "%s done\n", __func__); > > return 0; > - > -err_mode_config_cleanup: > - drm_mode_config_cleanup(ddev); > - return ret; > -} > - > -void tidss_modeset_cleanup(struct tidss_device *tidss) > -{ > - struct drm_device *ddev = &tidss->ddev; > - > - drm_mode_config_cleanup(ddev); > } > diff --git a/drivers/gpu/drm/tidss/tidss_kms.h b/drivers/gpu/drm/tidss/tidss_kms.h > index dda5625d0128..99aaff099f22 100644 > --- a/drivers/gpu/drm/tidss/tidss_kms.h > +++ b/drivers/gpu/drm/tidss/tidss_kms.h > @@ -10,6 +10,5 @@ > struct tidss_device; > > int tidss_modeset_init(struct tidss_device *tidss); > -void tidss_modeset_cleanup(struct tidss_device *tidss); > > #endif >
diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c index 460d5e9d0cf4..ad449d104306 100644 --- a/drivers/gpu/drm/tidss/tidss_drv.c +++ b/drivers/gpu/drm/tidss/tidss_drv.c @@ -103,11 +103,7 @@ static const struct dev_pm_ops tidss_pm_ops = { static void tidss_release(struct drm_device *ddev) { - struct tidss_device *tidss = ddev->dev_private; - drm_kms_helper_poll_fini(ddev); - - tidss_modeset_cleanup(tidss); } DEFINE_DRM_GEM_CMA_FOPS(tidss_fops); diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c index 5311e0f1c551..87e07e0e4eae 100644 --- a/drivers/gpu/drm/tidss/tidss_kms.c +++ b/drivers/gpu/drm/tidss/tidss_kms.c @@ -208,7 +208,9 @@ int tidss_modeset_init(struct tidss_device *tidss) dev_dbg(tidss->dev, "%s\n", __func__); - drm_mode_config_init(ddev); + ret = drm_mode_config_init(ddev); + if (ret) + return ret; ddev->mode_config.min_width = 8; ddev->mode_config.min_height = 8; @@ -220,11 +222,11 @@ int tidss_modeset_init(struct tidss_device *tidss) ret = tidss_dispc_modeset_init(tidss); if (ret) - goto err_mode_config_cleanup; + return ret; ret = drm_vblank_init(ddev, tidss->num_crtcs); if (ret) - goto err_mode_config_cleanup; + return ret; /* Start with vertical blanking interrupt reporting disabled. */ for (i = 0; i < tidss->num_crtcs; ++i) @@ -235,15 +237,4 @@ int tidss_modeset_init(struct tidss_device *tidss) dev_dbg(tidss->dev, "%s done\n", __func__); return 0; - -err_mode_config_cleanup: - drm_mode_config_cleanup(ddev); - return ret; -} - -void tidss_modeset_cleanup(struct tidss_device *tidss) -{ - struct drm_device *ddev = &tidss->ddev; - - drm_mode_config_cleanup(ddev); } diff --git a/drivers/gpu/drm/tidss/tidss_kms.h b/drivers/gpu/drm/tidss/tidss_kms.h index dda5625d0128..99aaff099f22 100644 --- a/drivers/gpu/drm/tidss/tidss_kms.h +++ b/drivers/gpu/drm/tidss/tidss_kms.h @@ -10,6 +10,5 @@ struct tidss_device; int tidss_modeset_init(struct tidss_device *tidss); -void tidss_modeset_cleanup(struct tidss_device *tidss); #endif
It's right above the drm_dev_put(). This is made possible by a preceeding patch which added a drmm_ cleanup action to drm_mode_config_init(), hence all we need to do to ensure that drm_mode_config_cleanup() is run on final drm_device cleanup is check the new error code for _init(). Aside: Another driver with a bit much devm_kzalloc, which should probably use drmm_kzalloc instead ... I'm pretty sure this one blows up already under KASAN because it's using devm_drm_dev_init, and later on devm_kzalloc. Hence the memory will get freed before the final drm_dev_put (all from the devres code), but the cleanup in that final drm_dev_put will access the just freed memory. Unfortunately fixing this properly needs slightly more work, namely drmm_ versions for all the drm objects (planes, crtc, ...), so that the cleanup actually happens before even drmm_kzalloc would release the underlying memory. Not quite there yet. v2: Explain why this cleanup is possible (Laurent). Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Jyri Sarha <jsarha@ti.com> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/tidss/tidss_drv.c | 4 ---- drivers/gpu/drm/tidss/tidss_kms.c | 19 +++++-------------- drivers/gpu/drm/tidss/tidss_kms.h | 1 - 3 files changed, 5 insertions(+), 19 deletions(-)