| Message ID | 1417046903-19949-1-git-send-email-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 26, 2014 at 07:08:23PM -0500, Rob Clark wrote: > _object_find() is not supposed to check for refcnt'd objects, that is > done in drm_mode_object_find(). > > Signed-off-by: Rob Clark <robdclark@gmail.com> > --- > drivers/gpu/drm/drm_crtc.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 9972cc8..3ba84df 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -356,9 +356,6 @@ static struct drm_mode_object *_object_find(struct drm_device *dev, > obj = NULL; > if (obj && obj->id != id) > obj = NULL; > - /* don't leak out unref'd fb's */ > - if (obj && (obj->type == DRM_MODE_OBJECT_FB)) > - obj = NULL; > mutex_unlock(&dev->mode_config.idr_mutex); Can't we just create a new _object_exists which returns bool? I know that this is safe, but it's also really tricky. And we have to get rid of the fairly useful comment explaining why this is dangerous. It's a bit of copypasted code, but imo that's much better here. Especially if there's a comment about refcounted objects. So still nack for this one. -Daniel
On Thu, Nov 27, 2014 at 4:49 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Nov 26, 2014 at 07:08:23PM -0500, Rob Clark wrote: >> _object_find() is not supposed to check for refcnt'd objects, that is >> done in drm_mode_object_find(). >> >> Signed-off-by: Rob Clark <robdclark@gmail.com> >> --- >> drivers/gpu/drm/drm_crtc.c | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >> index 9972cc8..3ba84df 100644 >> --- a/drivers/gpu/drm/drm_crtc.c >> +++ b/drivers/gpu/drm/drm_crtc.c >> @@ -356,9 +356,6 @@ static struct drm_mode_object *_object_find(struct drm_device *dev, >> obj = NULL; >> if (obj && obj->id != id) >> obj = NULL; >> - /* don't leak out unref'd fb's */ >> - if (obj && (obj->type == DRM_MODE_OBJECT_FB)) >> - obj = NULL; >> mutex_unlock(&dev->mode_config.idr_mutex); > > Can't we just create a new _object_exists which returns bool? I know that > this is safe, but it's also really tricky. And we have to get rid of the > fairly useful comment explaining why this is dangerous. that is basically the point of _object_find() :-) I'll add a comment that this doesn't take a ref so for refcnt'd mode objects returning non-null only means the object did exist BR, -R > It's a bit of copypasted code, but imo that's much better here. Especially > if there's a comment about refcounted objects. So still nack for this one. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 9972cc8..3ba84df 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -356,9 +356,6 @@ static struct drm_mode_object *_object_find(struct drm_device *dev, obj = NULL; if (obj && obj->id != id) obj = NULL; - /* don't leak out unref'd fb's */ - if (obj && (obj->type == DRM_MODE_OBJECT_FB)) - obj = NULL; mutex_unlock(&dev->mode_config.idr_mutex); return obj;
_object_find() is not supposed to check for refcnt'd objects, that is done in drm_mode_object_find(). Signed-off-by: Rob Clark <robdclark@gmail.com> --- drivers/gpu/drm/drm_crtc.c | 3 --- 1 file changed, 3 deletions(-)