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

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

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