[Intel-gfx] [PATCH] drm: Release reference from blob lookup after replacing property
On Tue, Oct 25, 2016 at 05:27:21PM -0400, Sean Paul wrote: > On Tue, Oct 25, 2016 at 3:46 PM, Chris Wilson > wrote: > > drm_property_lookup_blob() returns a reference to the returned blob, and > > drm_atomic_replace_property_blob() takes a references to the blob it > > stores, so afterwards we are left owning a reference to the new_blob that > > we never release, and thus leak memory every time we update a property > > such as during drm_atomic_helper_legacy_gamma_set(). > > > > Based on a patch by Felix Monninger > > > > Reported-by: Felix Monninger > > References: https://bugs.freedesktop.org/show_bug.cgi?id=98420 > > Signed-off-by: Chris Wilson > > --- > > drivers/gpu/drm/drm_atomic.c | 11 --- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 1b5a32df9a9a..3b35ab793100 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -416,19 +416,24 @@ drm_atomic_replace_property_blob_from_id(struct > > drm_crtc *crtc, > > ssize_t expected_size, > > bool *replaced) > > { > > - struct drm_device *dev = crtc->dev; > > struct drm_property_blob *new_blob = NULL; > > > > if (blob_id != 0) { > > - new_blob = drm_property_lookup_blob(dev, blob_id); > > + new_blob = drm_property_lookup_blob(crtc->dev, blob_id); > > I think this could be further simplified by making use of > drm_property_lookup_blob() returning NULL for blob_id == 0 > > Then you could do something like: > > static int > drm_atomic_replace_property_blob_from_id(struct drm_crtc *crtc, > struct drm_property_blob **old_blob, > uint64_t blob_id, > ssize_t expected_size, > bool *replaced) > { > struct drm_property_blob *blob = NULL; > int ret = 0; > > blob = drm_property_lookup_blob(crtc->dev, blob_id); Not sure. I think the orignal code would have been clearer as blob = NULL; if (id) { blob = drm_property_lookup_blob(dev, id); if (!blob) return -ENOENT; if (blob->length != expected_size) return -EINVAL; } i.e. the code currently reports if the blob_id doesn't match an existing blob, and only removes the current blob if passed in 0. Otherwise it becomes like: struct drm_property_blob *blob; int ret = -EINVAL; blob = drm_property_lookup_blob(crtc->dev, blob_id); if (!blob_id || (blob && (expected_size == 0 || expected_size == blob->length))) { drm_atomic_replace_property_blob(old_blob, blob, replaced); ret = 0; } drm_property_unreference_blob(blob); for which I'm actually favouring the existing code for the extra whitespace. If we insisted on a single return path: struct drm_property_blob *new_blob = NULL; int ret = -EINVAL; if (blob_id != 0) { new_blob = drm_property_lookup_blob(crtc->dev, blob_id); if (new_blob == NULL) goto out; if (expected_size > 0 && expected_size != new_blob->length) goto out; } drm_atomic_replace_property_blob(blob, new_blob, replaced); ret = 0; out: drm_property_unreference_blob(new_blob); return ret; -Chris -- Chris Wilson, Intel Open Source Technology Centre
[PATCH] drm: Release reference from blob lookup after replacing property
drm_property_lookup_blob() returns a reference to the returned blob, and drm_atomic_replace_property_blob() takes a references to the blob it stores, so afterwards we are left owning a reference to the new_blob that we never release, and thus leak memory every time we update a property such as during drm_atomic_helper_legacy_gamma_set(). Based on a patch by Felix Monninger Reported-by: Felix Monninger References: https://bugs.freedesktop.org/show_bug.cgi?id=98420 Signed-off-by: Chris Wilson --- drivers/gpu/drm/drm_atomic.c | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 1b5a32df9a9a..3b35ab793100 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -416,19 +416,24 @@ drm_atomic_replace_property_blob_from_id(struct drm_crtc *crtc, ssize_t expected_size, bool *replaced) { - struct drm_device *dev = crtc->dev; struct drm_property_blob *new_blob = NULL; if (blob_id != 0) { - new_blob = drm_property_lookup_blob(dev, blob_id); + new_blob = drm_property_lookup_blob(crtc->dev, blob_id); if (new_blob == NULL) return -EINVAL; - if (expected_size > 0 && expected_size != new_blob->length) + + if (expected_size > 0 && expected_size != new_blob->length) { + drm_property_unreference_blob(new_blob); return -EINVAL; + } } drm_atomic_replace_property_blob(blob, new_blob, replaced); + if (new_blob) + drm_property_unreference_blob(new_blob); + return 0; } -- 2.10.1
[Intel-gfx] [PATCH] drm: Release reference from blob lookup after replacing property
On Tue, Oct 25, 2016 at 3:46 PM, Chris Wilson wrote: > drm_property_lookup_blob() returns a reference to the returned blob, and > drm_atomic_replace_property_blob() takes a references to the blob it > stores, so afterwards we are left owning a reference to the new_blob that > we never release, and thus leak memory every time we update a property > such as during drm_atomic_helper_legacy_gamma_set(). > > Based on a patch by Felix Monninger > > Reported-by: Felix Monninger > References: https://bugs.freedesktop.org/show_bug.cgi?id=98420 > Signed-off-by: Chris Wilson > --- > drivers/gpu/drm/drm_atomic.c | 11 --- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 1b5a32df9a9a..3b35ab793100 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -416,19 +416,24 @@ drm_atomic_replace_property_blob_from_id(struct > drm_crtc *crtc, > ssize_t expected_size, > bool *replaced) > { > - struct drm_device *dev = crtc->dev; > struct drm_property_blob *new_blob = NULL; > > if (blob_id != 0) { > - new_blob = drm_property_lookup_blob(dev, blob_id); > + new_blob = drm_property_lookup_blob(crtc->dev, blob_id); I think this could be further simplified by making use of drm_property_lookup_blob() returning NULL for blob_id == 0 Then you could do something like: static int drm_atomic_replace_property_blob_from_id(struct drm_crtc *crtc, struct drm_property_blob **old_blob, uint64_t blob_id, ssize_t expected_size, bool *replaced) { struct drm_property_blob *blob = NULL; int ret = 0; blob = drm_property_lookup_blob(crtc->dev, blob_id); if (blob && expected_size > 0 && expected_size != blob->length) { ret = -EINVAL; goto out; } } drm_atomic_replace_property_blob(blob, blob, replaced); out: drm_property_unreference_blob(blob); return 0; } > if (new_blob == NULL) > return -EINVAL; > - if (expected_size > 0 && expected_size != new_blob->length) > + > + if (expected_size > 0 && expected_size != new_blob->length) { > + drm_property_unreference_blob(new_blob); > return -EINVAL; > + } > } > > drm_atomic_replace_property_blob(blob, new_blob, replaced); > > + if (new_blob) > + drm_property_unreference_blob(new_blob); > + > return 0; > } > > -- > 2.10.1 > > ___ > Intel-gfx mailing list > Intel-gfx at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx