| Message ID | 1392925627-6033-2-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2014-02-20 at 11:47 -0800, Ben Widawsky wrote: > GEN8 never freed the PPGTT struct. As GEN8 doesn't use full PPGTT, the > leak is small and only found on a module reload. ie. I don't think this > needs to go to stable. > > v2: The very naive, kfree in gen8 ppgtt cleanup, is subject to a double > free on PPGTT initialization failure. (Spotted by Imre). Instead this > patch pulls the ppgtt struct freeing out of the cleanup and leaves it to > the allocators/callers or the one doing the last kref_put as in standard > convention > > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> Reviewed-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 12 ++++++++++-- > drivers/gpu/drm/i915/i915_gem_gtt.c | 1 - > 2 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 171a2ef..9096e2a 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -99,9 +99,8 @@ > static int do_switch(struct intel_ring_buffer *ring, > struct i915_hw_context *to); > > -static void ppgtt_release(struct kref *kref) > +static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt) > { > - struct i915_hw_ppgtt *ppgtt = container_of(kref, struct i915_hw_ppgtt, ref); > struct drm_device *dev = ppgtt->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > struct i915_address_space *vm = &ppgtt->base; > @@ -135,6 +134,15 @@ static void ppgtt_release(struct kref *kref) > ppgtt->base.cleanup(&ppgtt->base); > } > > +static void ppgtt_release(struct kref *kref) > +{ > + struct i915_hw_ppgtt *ppgtt = > + container_of(kref, struct i915_hw_ppgtt, ref); > + > + do_ppgtt_cleanup(ppgtt); > + kfree(ppgtt); > +} > + > static size_t get_context_alignment(struct drm_device *dev) > { > if (IS_GEN6(dev)) > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 69a88d4..49e79fb 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -859,7 +859,6 @@ static void gen6_ppgtt_cleanup(struct i915_address_space *vm) > for (i = 0; i < ppgtt->num_pd_entries; i++) > __free_page(ppgtt->pt_pages[i]); > kfree(ppgtt->pt_pages); > - kfree(ppgtt); > } > > static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 171a2ef..9096e2a 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -99,9 +99,8 @@ static int do_switch(struct intel_ring_buffer *ring, struct i915_hw_context *to); -static void ppgtt_release(struct kref *kref) +static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt) { - struct i915_hw_ppgtt *ppgtt = container_of(kref, struct i915_hw_ppgtt, ref); struct drm_device *dev = ppgtt->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; struct i915_address_space *vm = &ppgtt->base; @@ -135,6 +134,15 @@ static void ppgtt_release(struct kref *kref) ppgtt->base.cleanup(&ppgtt->base); } +static void ppgtt_release(struct kref *kref) +{ + struct i915_hw_ppgtt *ppgtt = + container_of(kref, struct i915_hw_ppgtt, ref); + + do_ppgtt_cleanup(ppgtt); + kfree(ppgtt); +} + static size_t get_context_alignment(struct drm_device *dev) { if (IS_GEN6(dev)) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 69a88d4..49e79fb 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -859,7 +859,6 @@ static void gen6_ppgtt_cleanup(struct i915_address_space *vm) for (i = 0; i < ppgtt->num_pd_entries; i++) __free_page(ppgtt->pt_pages[i]); kfree(ppgtt->pt_pages); - kfree(ppgtt); } static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
GEN8 never freed the PPGTT struct. As GEN8 doesn't use full PPGTT, the leak is small and only found on a module reload. ie. I don't think this needs to go to stable. v2: The very naive, kfree in gen8 ppgtt cleanup, is subject to a double free on PPGTT initialization failure. (Spotted by Imre). Instead this patch pulls the ppgtt struct freeing out of the cleanup and leaves it to the allocators/callers or the one doing the last kref_put as in standard convention Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem_context.c | 12 ++++++++++-- drivers/gpu/drm/i915/i915_gem_gtt.c | 1 - 2 files changed, 10 insertions(+), 3 deletions(-)