[PATCH] drm/exynos: fix memory leak: free EDID block

[Sean Paul]

On Tue, Nov 20, 2012 at 3:29 PM, Egbert Eich <eich at suse.com> wrote:
> Sean Paul writes:
>  > On Tue, Nov 20, 2012 at 4:30 AM, Egbert Eich <eich at suse.de> wrote:
>  > > drm_get_edid() returns a pointer to an EDID block. The caller
>  > > is responsible to free this pointer itself.
>  > > Here the pointer gets assigned to the local variable raw_edid.
>  > > Therefore it should be freed before the variable goes out of
>  > > scope.
>  > >
>  > > Signed-off-by: Egbert Eich <eich at suse.de>

Reviewed-by: Sean Paul <seanpaul at chromium.org>

>  > > ---
>  > >  drivers/gpu/drm/exynos/exynos_hdmi.c |    1 +
>  > >  1 files changed, 1 insertions(+), 0 deletions(-)
>  > >
>  > > diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
>  > > index 2c115f8..bc87bca 100644
>  > > --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
>  > > +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
>  > > @@ -1293,6 +1293,7 @@ static int hdmi_get_edid(void *ctx, struct drm_connector *connector,
>  > >                 DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n",
>  > >                         (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"),
>  > >                         raw_edid->width_cm, raw_edid->height_cm);
>  > > +               kfree(raw_edid);
>  >
>  > This will actually cause the memory to be freed twice.
>  >
>  > The reason this happens is drm_get_edid attaches this to
>  > connector->display_info.raw_edid, which is then freed in the
>  > exynos_drm_connector function that gets the edid.
>
> No, the raw_edid member is gone from struct drm_display_info.
> All other drivers free the edid data returned by drm_get_edid()
> themselves.

A ha! I should have checked first, my apologies.

> I would actually prefer if the edid data would be freed by
> drm_connector_destroy rather than by the drivers as this would
> allow us to cache an edid without creating copies to pass to
> the drivers. However some drivers store the pointer to returned
> edid block somewhere in their own data structures, so if we
> do this it can easily get out of sync when we reread the
> edid and it has changed for whatever reason in which case
> the driver will have a stale and invalid pointer.
>

Agreed, that would be nice.

>  >
>  > The whole thing is ugly, and needs to be revised. I've uploaded a
>  > patch to refactor this against the chromium tree, but haven't yet
>  > rebased against upstream. See
>  > https://gerrit.chromium.org/gerrit/#/c/38406/
>  >
>  > For now, please drop this patch.
>  >
>
> I've looked at the code in the exynos driver in the drm_next
> branch - there's nothing that destroys this edid data any more.
>

Yep, right you are. Your patch does the right thing, thanks for
setting me straight :)

I hope we won't need to live with this double-allocation + copy for too long.

Sean



> Cheers,
>         Egbert.