On Tue, Apr 02, 2019 at 05:49:23PM +0200, Philipp Zabel wrote:
Hi Michael,
On Tue, 2019-04-02 at 15:49 +0200, Michael Grzeschik wrote:
The destroy function in drm_mode_config_cleanup will remove the objects in ipu-drm-core by calling its destroy functions if the bind function fails. The drm_crtc is also part of the devres allocated ipu_crtc object. The ipu_crtc object will already be cleaned up if the bind for the crtc fails. This leads drm_crtc_cleanup try to clean already freed memory.
We fix this issue by adding the devres action ipu_crtc_remove_head which will remove its head from the objects in ipu-drm-core which then never calls its destroy function anymore.
Signed-off-by: Michael Grzeschik m.grzeschik@pengutronix.de
drivers/gpu/drm/imx/ipuv3-crtc.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index ec3602ebbc1cd..fa1ee33a43d77 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -429,6 +429,14 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc, return ret; }
+static void ipu_crtc_remove_head(void *data) +{
- struct ipu_crtc *ipu_crtc = data;
- struct drm_crtc *crtc = &ipu_crtc->base;
- list_del(&crtc->head);
I don't think reaching into drm_crtc internals like this is going to be robust. Currently, this is either missing the rest of drm_crtc_cleanup, or it will crash if drm_crtc_init_with_planes hasn't been called yet.
I think you could call devm_add_action with a function that calls drm_crtc_cleanup after drm_crtc_init_with_planes in ipu_crtc_init.
Alternatively, the ipu_crtc allocation could be changed to kzalloc. It would then have to freed manually in the drm_crtc_funcs->destroy callback.
Dirty secret of devm: You can't use it for any drm_ structure, because the lifetimes of those do not match the lifetimes of the underlying device. We'd need to tie the lifetimes of drm objects to drm_device. Noralf has some patches to move that forward. We'd need something like drm_dev_kzalloc which releases memory when drm_device is freed.
Agreed that digging even deeper into the devm deadend is not a good idea. -Daniel