linux-kernel - RE: [RFC][PATCH v3] DRM: add DRM Driver for Samsung SoC EXYNOS4210.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-id: <001401cc6967$e41f3460$ac5d9d20$%dae@samsung.com>
Date:	Fri, 02 Sep 2011 21:00:21 +0900
From:	Inki Dae <inki.dae@...sung.com>
To:	'Rob Clark' <robdclark@...il.com>
Cc:	airlied@...ux.ie, dri-devel@...ts.freedesktop.org,
	sw0312.kim@...sung.com, linux-kernel@...r.kernel.org,
	kyungmin.park@...sung.com, linux-arm-kernel@...ts.infradead.org
Subject: RE: [RFC][PATCH v3] DRM: add DRM Driver for Samsung SoC EXYNOS4210.

Hello Rob.
Below is my comments.

> -----Original Message-----
> From: Rob Clark [mailto:robdclark@...il.com]
> Sent: Friday, September 02, 2011 10:18 AM
> To: Inki Dae
> Cc: airlied@...ux.ie; dri-devel@...ts.freedesktop.org;
> sw0312.kim@...sung.com; linux-kernel@...r.kernel.org;
> kyungmin.park@...sung.com; linux-arm-kernel@...ts.infradead.org
> Subject: Re: [RFC][PATCH v3] DRM: add DRM Driver for Samsung SoC
> EXYNOS4210.
> 
> On Thu, Sep 1, 2011 at 8:06 AM, Inki Dae <inki.dae@...sung.com> wrote:
> >> >> > +struct samsung_drm_gem_obj *
> >> >> > +               find_samsung_drm_gem_object(struct drm_file
> > *file_priv,
> >> >> > +                       struct drm_device *dev, unsigned int
handle)
> >> >> > +{
> >> >> > +       struct drm_gem_object *gem_obj;
> >> >> > +
> >> >> > +       gem_obj = drm_gem_object_lookup(dev, file_priv, handle);
> >> >> > +       if (!gem_obj) {
> >> >> > +               DRM_LOG_KMS("a invalid gem object not registered
to
> >> >> lookup.\n");
> >> >> > +               return NULL;
> >> >> > +       }
> >> >> > +
> >> >> > +       /**
> >> >> > +        * unreference refcount of the gem object.
> >> >> > +        * at drm_gem_object_lookup(), the gem object was
> referenced.
> >> >> > +        */
> >> >> > +       drm_gem_object_unreference(gem_obj);
> >> >>
> >> >> this doesn't seem right, to drop the reference before you use the
> >> >> buffer elsewhere..
> >> >>
> >> > No, see drm_gem_object_lookup fxn. at this function, if there is a
> >> object
> >> > found then drm_gem_object_reference is called to increase refcount of
> >> this
> >> > object. if there is any missing point, give me any comment please.
> thank
> >> > you.
> >>
> >>
> >> Right, but I think there is a reason it takes a reference... so that
> >> the object doesn't get free'd from under your feet.  So pattern
> >> should, I think, be:
> >>
> >>   obj = lookup(...);
> >>   ... do stuff w/ obj ...
> >>   unreference(obj)
> >>
> >> so the caller who is using the looked up obj should unref it when done
> >>
> >> Instead, you have:
> >>
> >>   obj = lookup(...);
> >>   unreference(obj);
> >>   ... do stuff w/ obj ...
> >>
> >>
> >
> > Generally right, but in this case, it is just used to get specific gem
> > object through find_samsung_drm_gem_object() so doesn't reference this
> gem
> > object anywhere.
> > therefore reference and unreference should be done within
> > find_samsung_drm_gem_object(). if there is any point I missed then let
> me
> > know please. thank you.
> >
> 
> Still, it seems like find_samsung_drm_gem_object() is encouraging the
> wrong usage-pattern, even if it works fine today because you know
> somewhere else is holding a reference to the object.  Later if you
> expand your use of GEM objects, this fxn might come back to bite you.
> There is a good reason that drm_gem_object_lookup() takes a reference
> to the object, and it feels wrong to intentionally subvert that.
> 
> (I'm perfectly willing to be overridden on the subject.. there are
> plenty of folks on this list who have been doing the GEM thing longer
> than I have.  But it just seems better to use APIs like
> drm_gem_object_lookup() the way they were intended.)
> 

Ah, you are right. I misunderstanded it. as you pointed out, a gem object
should be unreferenced after doing something with the gem object. so I will
remove find_samsung_drm_gem_object() and use drm_gem_object_lookup()
directly to get a gem object instead. of course, the gem object will be
unreferenced after doing something with it. thank you for your explanation.
:)

> BR,
> -R

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/