All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Don't leak VBT mode data
@ 2015-09-15  2:24 Matt Roper
  2015-09-15  7:50 ` Jani Nikula
  2015-09-18 11:27 ` Jani Nikula
  0 siblings, 2 replies; 6+ messages in thread
From: Matt Roper @ 2015-09-15  2:24 UTC (permalink / raw)
  To: intel-gfx

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>
---
 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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Don't leak VBT mode data
  2015-09-15  2:24 [PATCH] drm/i915: Don't leak VBT mode data Matt Roper
@ 2015-09-15  7:50 ` Jani Nikula
  2015-09-23  9:23   ` Daniel Vetter
  2015-09-18 11:27 ` Jani Nikula
  1 sibling, 1 reply; 6+ messages in thread
From: Jani Nikula @ 2015-09-15  7:50 UTC (permalink / raw)
  To: Matt Roper, intel-gfx

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.


> ---
>  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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Don't leak VBT mode data
  2015-09-15  2:24 [PATCH] drm/i915: Don't leak VBT mode data Matt Roper
  2015-09-15  7:50 ` Jani Nikula
@ 2015-09-18 11:27 ` Jani Nikula
  1 sibling, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2015-09-18 11:27 UTC (permalink / raw)
  To: Matt Roper, intel-gfx

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>

Pushed to drm-intel-next-queued, thanks for the patch.

BR,
Jani.



> ---
>  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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Don't leak VBT mode data
  2015-09-15  7:50 ` Jani Nikula
@ 2015-09-23  9:23   ` Daniel Vetter
  2015-09-23 11:02     ` Jani Nikula
  0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2015-09-23  9:23 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

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. 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.
-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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Don't leak VBT mode data
  2015-09-23  9:23   ` Daniel Vetter
@ 2015-09-23 11:02     ` Jani Nikula
  2015-09-23 13:15       ` Daniel Vetter
  0 siblings, 1 reply; 6+ messages in thread
From: Jani Nikula @ 2015-09-23 11:02 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Don't leak VBT mode data
  2015-09-23 11:02     ` Jani Nikula
@ 2015-09-23 13:15       ` Daniel Vetter
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2015-09-23 13:15 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

On Wed, Sep 23, 2015 at 02:02:03PM +0300, Jani Nikula wrote:
> 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?

For udl if you unplug the physical device the struct device _will_ go
away.

The drm_device otoh might be referenced by piles of other internal things
(like open file descriptors, shared dma-bufs and what else) and hence very
much must stick around. Of course we /should/ try to not call down into
lower-level hw handling code in that state. Which all very much doesn't
happen (at least in a race-free manner).

So the drm_device is not bounded by the lifetime of the underlying
physical device in full generality. Which is why we can't really do any
devm_* stuff in drm core.

> > 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. :)

Either way I think we should be ok with fully abusing devm_* ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-09-23 13:12 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-15  2:24 [PATCH] drm/i915: Don't leak VBT mode data Matt Roper
2015-09-15  7:50 ` Jani Nikula
2015-09-23  9:23   ` Daniel Vetter
2015-09-23 11:02     ` Jani Nikula
2015-09-23 13:15       ` Daniel Vetter
2015-09-18 11:27 ` Jani Nikula

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.