[Intel-gfx] [PATCHv2] drm/i915: free crtc on driver remove

[Murthy, Arun R]

> >> >> > intel_crtc is being allocated as part of
> >> >> > intel_modeset_init_nogem and not freed as part of driver remove.
> This will lead to memory leak.
> >> >> > Hence free up the allocated crtc on driver remove as part of
> >> >> > intel_modeset_driver_remove_nogem.
> >> >>
> >> >> No, there's no leak and this is not needed.
> >> >>
> >> >> See drm_mode_config_cleanup() calling crtc->funcs->destroy() on
> >> >> each
> >> crtc.
> >> >
> >> > Sorry, I didn't notice this function.
> >> >
> >> > intel_crtc_alloc() is called as part of
> >> > probe->intel_modeset_init_nogem->intel_crtc_init
> >> > on similar basis cleanup/free should be done in  driver
> >> > remove->intel_modeset_driver_remove_nogem->intel_crtc_free
> >>
> >> It's just an error prone extra burden for the drivers to take care of
> >> this manually, so we have drm_mode_config_cleanup(). Which also
> >> cleans up encoders and encoders etc.
> >>
> >> > Does this look cleaner?
> >> >
> >> > Kfree(crtc) which is called in crtc->funcs->destroy is intended for
> >> > cleanup and hence
> >> > drm_crtc_cleanup() is being called from intel_crtc_destroy(). The
> >> > comments added in drm_crtc_funcs say that cleanup resources on
> destroy.
> >> >
> >> > Again looking at the driver design, intel_crtc_alloc is not done as
> >> > part of any drm_crtc_funcs, rather on probe->modeset_init_nogem, so
> >> > calling intel_crtc_free from remove->modeset_driver_remove_nogem
> >> would make more sence.
> >>
> >> That would add another ordering dependency between calling
> >> drm_mode_config_cleanup() and freeing the individual crtcs separately
> >> afterwards. And looking at the cleanup code, I'm sure that's not what
> >> we want.
> >>
> > This is for sure ordering and not dependency. This ordering is being
> > followed all over the drivers across the kernel. Just like simple probe and
> remove.
> > So the usual tendency would be to see as whatever
> > initialization/allocation being done in probe in the same order
> > deinitialization/deallocation being done in remove.
> > Usually in probe multiple initializations are done and so on each
> > failures a goto will be used to cleanup the respective initialized
> > stuff. Eventually these functions mentioned in the cleanup under the goto
> will be the one called in remove.
> 
> Well, ordering is mostly driven by dependency, and as I said it's not purely
> reverse ordering for cleanup, because of non-trivial interdependencies...
> 

I am also trying to bring in the same reverse ordering, considering the dependencies
so as to make the code more readable.
While getting to this patch, I didn't notice the .destroy part, after considering that
I agree this patch makes no sense, but it just opens up opportunity to cleanup
remove so as to follow the reverse ordering to that being done in probe.

If maintainers feel that its not required and would survive with what exists, I will
just drop this patch.

Thanks and Regards,
Arun R Murthy
--------------------