| Message ID | 20161028121227.16904-5-cfergeau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> > qdev->gem.objects was initialized directly in qxl_device_init() rather > than going through qxl_gem_init(), and qxl_gem_fini() was never called. > Considering "qxl_gem_fini() was never called" did we have a leak? > Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> > --- > drivers/gpu/drm/qxl/qxl_kms.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c > index e642242..af685f1 100644 > --- a/drivers/gpu/drm/qxl/qxl_kms.c > +++ b/drivers/gpu/drm/qxl/qxl_kms.c > @@ -131,7 +131,7 @@ static int qxl_device_init(struct qxl_device *qdev, > mutex_init(&qdev->update_area_mutex); > mutex_init(&qdev->release_mutex); > mutex_init(&qdev->surf_evict_mutex); > - INIT_LIST_HEAD(&qdev->gem.objects); > + qxl_gem_init(qdev); > Here qxl_gem_init returns a value that is always ignored, perhaps would be better to return void from qxl_gem_init if it cannot fails. > qdev->rom_base = pci_resource_start(pdev, 2); > qdev->rom_size = pci_resource_len(pdev, 2); > @@ -273,6 +273,7 @@ static void qxl_device_fini(struct qxl_device *qdev) > qxl_ring_free(qdev->command_ring); > qxl_ring_free(qdev->cursor_ring); > qxl_ring_free(qdev->release_ring); > + qxl_gem_fini(qdev); > qxl_bo_fini(qdev); > io_mapping_free(qdev->surface_mapping); > io_mapping_free(qdev->vram_mapping); Frediano
On Mon, Oct 31, 2016 at 07:42:35AM -0400, Frediano Ziglio wrote: > > > > qdev->gem.objects was initialized directly in qxl_device_init() rather > > than going through qxl_gem_init(), and qxl_gem_fini() was never called. > > > > Considering "qxl_gem_fini() was never called" did we have a leak? Maybe, as this forces the freeing of some pending bo's if they are still in use. This would only happen when unloading/reloading the module repeatedly, which I don't expect to happen on production systems. > > > Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> > > --- > > drivers/gpu/drm/qxl/qxl_kms.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c > > index e642242..af685f1 100644 > > --- a/drivers/gpu/drm/qxl/qxl_kms.c > > +++ b/drivers/gpu/drm/qxl/qxl_kms.c > > @@ -131,7 +131,7 @@ static int qxl_device_init(struct qxl_device *qdev, > > mutex_init(&qdev->update_area_mutex); > > mutex_init(&qdev->release_mutex); > > mutex_init(&qdev->surf_evict_mutex); > > - INIT_LIST_HEAD(&qdev->gem.objects); > > + qxl_gem_init(qdev); > > > > Here qxl_gem_init returns a value that is always ignored, perhaps > would be better to return void from qxl_gem_init if it cannot > fails. Good suggestion, I'll add that to a separate patch. Christophe
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c index e642242..af685f1 100644 --- a/drivers/gpu/drm/qxl/qxl_kms.c +++ b/drivers/gpu/drm/qxl/qxl_kms.c @@ -131,7 +131,7 @@ static int qxl_device_init(struct qxl_device *qdev, mutex_init(&qdev->update_area_mutex); mutex_init(&qdev->release_mutex); mutex_init(&qdev->surf_evict_mutex); - INIT_LIST_HEAD(&qdev->gem.objects); + qxl_gem_init(qdev); qdev->rom_base = pci_resource_start(pdev, 2); qdev->rom_size = pci_resource_len(pdev, 2); @@ -273,6 +273,7 @@ static void qxl_device_fini(struct qxl_device *qdev) qxl_ring_free(qdev->command_ring); qxl_ring_free(qdev->cursor_ring); qxl_ring_free(qdev->release_ring); + qxl_gem_fini(qdev); qxl_bo_fini(qdev); io_mapping_free(qdev->surface_mapping); io_mapping_free(qdev->vram_mapping);
qdev->gem.objects was initialized directly in qxl_device_init() rather than going through qxl_gem_init(), and qxl_gem_fini() was never called. Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> --- drivers/gpu/drm/qxl/qxl_kms.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)