On Thu, 30 Jun 2022, "Murthy, Arun R" <arun.r.murthy@xxxxxxxxx> wrote:
>> > 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.
Moreover, drm is moving towards managed initialization, which means not
having to call drm_mode_config_cleanup() explicitly at all. It'll get
called as part of drmm managed release action tied to the drm device. So
really, calling kfree as part of the callback is the natural thing to
do. Indeed it would be difficult to do it anywhere else, for no benefit.
> Also, looking into the func intel_modeset_init_nogem(), the func intel_modeset_driver_remove_nogem
> should be renamed as intel_modeset_deinit_nogem().
The cleanup naming comes from them being called as part of struct
pci_driver .remove callback chain, and it's a useful reminder.
Also, the intel_modeset_driver_remove{,noirq,nogem} functions should
*not* be considered 1:1 counterparts of intel_modeset_init{noirq,nogem,}
as the init/remove are asymmetric around irq and gem.
Sure, there's work to be done in cleaning up the probe and remove paths,
and further trying to separate the gem and display parts, but that's way
more involved than simple renames, really.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
