Re: [PATCH] drm/i915: Don't leak VBT mode data

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Don't leak VBT mode data
Date: Wed, 23 Sep 2015 14:02:03 +0300	[thread overview]
Message-ID: <87zj0do0us.fsf@intel.com> (raw)
In-Reply-To: <20150923092324.GD3383@phenom.ffwll.local>

On Wed, 23 Sep 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Sep 15, 2015 at 10:50:45AM +0300, Jani Nikula wrote:
>> On Tue, 15 Sep 2015, Matt Roper <matthew.d.roper@intel.com> wrote:
>> > We allocate memory for LVDS modes while parsing the VBT at startup, but
>> > never free this memory when the driver is unloaded, causing a small
>> > leak.
>> >
>> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> 
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>> 
>> We could probably use devm_*alloc functions for this kind of stuff more.
>
> The problem with devm_* is that the lifetime of the underlying hw
> device doesn't stricltly match the lifetime of the drm device.

Is it not enough that the lifetime of the driver is bounded by the
lifetime of the drm device? What is the problem you're referring to,
apart from handing the hardware devm allocated memory and unreferencing
the drm device?

> Not that anything in drm gets that right (pretty far from it), but
> that's why I haven't gone ballastic yet with rolling out devm_* all
> over our init code.

I think you mean "ballistic", but, in this case, I think your typo
"ballastic" is actually a more accurate description of the change. :)

BR,
Jani.

> -Daniel
>
>> 
>> 
>> > ---
>> >  drivers/gpu/drm/i915/i915_dma.c | 4 ++++
>> >  1 file changed, 4 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>> > index 066a0ef..c069550 100644
>> > --- a/drivers/gpu/drm/i915/i915_dma.c
>> > +++ b/drivers/gpu/drm/i915/i915_dma.c
>> > @@ -1137,6 +1137,10 @@ int i915_driver_unload(struct drm_device *dev)
>> >  		dev_priv->vbt.child_dev = NULL;
>> >  		dev_priv->vbt.child_dev_num = 0;
>> >  	}
>> > +	kfree(dev_priv->vbt.sdvo_lvds_vbt_mode);
>> > +	dev_priv->vbt.sdvo_lvds_vbt_mode = NULL;
>> > +	kfree(dev_priv->vbt.lfp_lvds_vbt_mode);
>> > +	dev_priv->vbt.lfp_lvds_vbt_mode = NULL;
>> >  
>> >  	vga_switcheroo_unregister_client(dev->pdev);
>> >  	vga_client_register(dev->pdev, NULL, NULL, NULL);
>> > -- 
>> > 2.1.4
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx