diff mbox

drm: fix memory leak around mode_group

Message ID 1397779321-30350-1-git-send-email-airlied@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dave Airlie April 18, 2014, 12:02 a.m. UTC
From: Dave Airlie <airlied@redhat.com>

This mode group id_list was never being freed.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_crtc.c | 6 ++++++
 drivers/gpu/drm/drm_stub.c | 1 +
 include/drm/drm_crtc.h     | 1 +
 3 files changed, 8 insertions(+)

Comments

David Herrmann April 22, 2014, 7:39 a.m. UTC | #1
Hi

On Fri, Apr 18, 2014 at 2:02 AM, Dave Airlie <airlied@gmail.com> wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> This mode group id_list was never being freed.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 6 ++++++
>  drivers/gpu/drm/drm_stub.c | 1 +
>  include/drm/drm_crtc.h     | 1 +
>  3 files changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index d8b7099..a3fe324 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1378,6 +1378,12 @@ static int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *gr
>         return 0;
>  }
>
> +void drm_mode_group_destroy(struct drm_mode_group *group)
> +{
> +       kfree(group->id_list);
> +       group->id_list = NULL;
> +}
> +
>  /*
>   * NOTE: Driver's shouldn't ever call drm_mode_group_init_legacy_group - it is
>   * the drm core's responsibility to set up mode control groups.
> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> index 4c24c3a..80bc780 100644
> --- a/drivers/gpu/drm/drm_stub.c
> +++ b/drivers/gpu/drm/drm_stub.c
> @@ -371,6 +371,7 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
>         spin_unlock_irqrestore(&drm_minor_lock, flags);
>         minor->index = 0;
>
> +       drm_mode_group_destroy(&minor->mode_group);

Nice catch, but this is racy regarding drm_unplug_dev(). Can we do it
in drm_minor_free() instead? Imho, given that this is a
memory-cleanup, not a runtime helper, it belongs there, anyway.

Thanks
David
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d8b7099..a3fe324 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1378,6 +1378,12 @@  static int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *gr
 	return 0;
 }
 
+void drm_mode_group_destroy(struct drm_mode_group *group)
+{
+	kfree(group->id_list);
+	group->id_list = NULL;
+}
+
 /*
  * NOTE: Driver's shouldn't ever call drm_mode_group_init_legacy_group - it is
  * the drm core's responsibility to set up mode control groups.
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 4c24c3a..80bc780 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -371,6 +371,7 @@  static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
 	spin_unlock_irqrestore(&drm_minor_lock, flags);
 	minor->index = 0;
 
+	drm_mode_group_destroy(&minor->mode_group);
 	drm_debugfs_cleanup(minor);
 	drm_sysfs_device_remove(minor);
 }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index e55fccb..c6b9e8a 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -915,6 +915,7 @@  extern const char *drm_get_tv_subconnector_name(int val);
 extern const char *drm_get_tv_select_name(int val);
 extern void drm_fb_release(struct drm_file *file_priv);
 extern int drm_mode_group_init_legacy_group(struct drm_device *dev, struct drm_mode_group *group);
+extern void drm_mode_group_destroy(struct drm_mode_group *group);
 extern bool drm_probe_ddc(struct i2c_adapter *adapter);
 extern struct edid *drm_get_edid(struct drm_connector *connector,
 				 struct i2c_adapter *adapter);