| Message ID | 1353403816-8091-1-git-send-email-eich@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 20, 2012 at 4:30 AM, Egbert Eich <eich@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@suse.de> > --- > 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. 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. Sean > } else { > return -ENODEV; > } > -- > 1.7.7 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Sean Paul writes: > On Tue, Nov 20, 2012 at 4:30 AM, Egbert Eich <eich@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@suse.de> > > --- > > 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. 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. > > 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. Cheers, Egbert.
On Tue, Nov 20, 2012 at 3:29 PM, Egbert Eich <eich@suse.com> wrote: > Sean Paul writes: > > On Tue, Nov 20, 2012 at 4:30 AM, Egbert Eich <eich@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@suse.de> Reviewed-by: Sean Paul <seanpaul@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.
Ok, I modifed the subject of your patch like below and applied. drm/exynos: fix memory lean to EDID block Thanks, Inki Dae 2012/11/20 Egbert Eich <eich@suse.de> > 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@suse.de> > --- > 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); > } else { > return -ENODEV; > } > -- > 1.7.7 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Tue, 20 Nov 2012, Sean Paul <seanpaul@chromium.org> wrote: > On Tue, Nov 20, 2012 at 4:30 AM, Egbert Eich <eich@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@suse.de> >> --- >> 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. > > 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/ The patch is good. connector->display_info.raw_edid is gone since commit 451023dc32d4542c21b52ad1692e6e01cb75b099 Author: Jani Nikula <jani.nikula@intel.com> Date: Wed Aug 15 09:32:39 2012 +0000 drm: remove the raw_edid field from struct drm_display_info BR, Jani. > > For now, please drop this patch. > > Sean > >> } else { >> return -ENODEV; >> } >> -- >> 1.7.7 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
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); } else { return -ENODEV; }
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@suse.de> --- drivers/gpu/drm/exynos/exynos_hdmi.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)