All of lore.kernel.org
 help / color / mirror / Atom feed
From: Chris Wilson <chris@chris-wilson.co.uk>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Julia Lawall <julia.lawall@lip6.fr>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 12/14] drm: Move master pointer from drm_minor to drm_device
Date: Wed, 15 Jun 2016 17:33:14 +0100	[thread overview]
Message-ID: <20160615163314.GB6754@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <20160615160141.GP1338@phenom.ffwll.local>

On Wed, Jun 15, 2016 at 06:01:41PM +0200, Daniel Vetter wrote:
> On Wed, Jun 15, 2016 at 01:10:35PM +0100, Chris Wilson wrote:
> > On Tue, Jun 14, 2016 at 08:51:07PM +0200, Daniel Vetter wrote:
> > > There can only be one current master, and it's for the overall device.
> > > Render/control minors don't support master-based auth at all.
> > > 
> > > This simplifies the master logic a lot, at least in my eyes: All these
> > > additional pointer chases are just confusing.
> > 
> > One master for the device, on the struct drm_device, as opposed to hidden
> > behind the first of three minors, makes sense.
> > 
> > > @@ -128,13 +128,13 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
> > >  	lockdep_assert_held_once(&dev->master_mutex);
> > >  
> > >  	/* create a new master */
> > > -	fpriv->minor->master = drm_master_create(fpriv->minor->dev);
> > > -	if (!fpriv->minor->master)
> > > +	dev->master = drm_master_create(dev);
> > > +	if (!dev->master)
> > >  		return -ENOMEM;
> > >  
> > >  	/* take another reference for the copy in the local file priv */
> > >  	old_master = fpriv->master;
> > > -	fpriv->master = drm_master_get(fpriv->minor->master);
> > > +	fpriv->master = drm_master_get(dev->master);
> > >  
> > >  	if (dev->driver->master_create) {
> > >  		ret = dev->driver->master_create(dev, fpriv->master);
> > 
> > > @@ -234,10 +234,10 @@ int drm_master_open(struct drm_file *file_priv)
> > >  	/* if there is no current master make this fd it, but do not create
> > >  	 * any master object for render clients */
> > >  	mutex_lock(&dev->master_mutex);
> > > -	if (!file_priv->minor->master)
> > > +	if (!dev->master)
> > >  		ret = drm_new_set_master(dev, file_priv);
> > >  	else
> > > -		file_priv->master = drm_master_get(file_priv->minor->master);
> > > +		file_priv->master = drm_master_get(dev->master);
> > >  	mutex_unlock(&dev->master_mutex);
> > 
> > You could take the opportunity to make this a bit simpler:
> > 
> > 	if (!READ_ONCE(dev->master)) {
> > 		int ret;
> > 
> > 		ret = 0;
> > 		mutex_lock(&dev->master_mutex);
> > 		if (!dev->master)
> > 			ret = drm_new_master(dev);
> > 		mutex_unlock(&dev->master_mutex);
> > 		if (ret)
> > 			return ret;
> > 	}
> > 
> > 	file_priv->master = drm_master_get(dev->master);
> 
> drm_master_get(dev->master) must be under the master_mutex, without it we
> could race with a drm_master_put(&dev->master) and end up doing a kref_get
> when the refcount already reached 0.

Something is very fishy then. The behaviour of drm_new_master() would
appear to be to create a drm_master owned by the device, but really it is
owned by file_priv?

* checks back on drm_master

So drm_master is the authentication structure that needs to be unique to
a hierachy. So drm_new_set_master() and here really do appear backwards.

The old drm_new_set_master() makes more sense, it assigns to the
file_priv, and then performs a setmaster operation. In that ordering,
you could even do the file_priv local operation of creating the new
master structure before taking the lock to perform setmaster.


> > Just to straighten out the kref dance.
> > 
> > >  
> > >  	return ret;
> > > @@ -271,11 +271,11 @@ void drm_master_release(struct drm_file *file_priv)
> > >  		mutex_unlock(&dev->struct_mutex);
> > >  	}
> > >  
> > > -	if (file_priv->minor->master == file_priv->master) {
> > > +	if (dev->master == file_priv->master) {
> > >  		/* drop the reference held my the minor */
> > >  		if (dev->driver->master_drop)
> > >  			dev->driver->master_drop(dev, file_priv, true);
> > > -		drm_master_put(&file_priv->minor->master);
> > > +		drm_master_put(&dev->master);
> > 
> > This still makes me uneasy. This is not equivalent to dropmaster_ioctl
> > and subsequent setmaster_ioctl will fail as dev->master is still
> > assigned (but the owner has gone).
> 
> drm_master_put clears the pointer passed to it, so dev->master will be set
> to NULL. And it does the same as drop_master (wrt dev->master at least,
> master_release also needs to clean up file_priv->master on top). Not sure
> it's worth it to extract those 5 lines into a __drm_drop_master() helper
> function? I can respin with that if you want. On the master_open/setmaster
> side the shared code is already extracted in drm_new_set_master().

drm_master_put() nullifies, didn't expect that.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2016-06-15 16:33 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-14 18:50 [PATCH 00/14] Cruft removal around drm_master Daniel Vetter
2016-06-14 18:50 ` [PATCH 01/14] drm: Nuke legacy maps debugfs files Daniel Vetter
2016-06-15 11:23   ` Chris Wilson
2016-06-15 12:00   ` Emil Velikov
2016-06-14 18:50 ` [PATCH 02/14] drm: Hide hw.lock cleanup in filp->release better Daniel Vetter
2016-06-15 11:26   ` [Intel-gfx] " Chris Wilson
2016-06-15 12:10   ` Emil Velikov
2016-06-14 18:50 ` [PATCH 03/14] drm: Link directly from drm_master to drm_device Daniel Vetter
2016-06-15 11:29   ` Chris Wilson
2016-06-15 12:50   ` [Intel-gfx] " Emil Velikov
2016-06-15 15:45     ` Daniel Vetter
2016-06-16 20:04       ` Emil Velikov
2016-06-14 18:50 ` [PATCH 04/14] drm: Move master functions into drm_auth.c Daniel Vetter
2016-06-15 11:31   ` Chris Wilson
2016-06-15 15:47     ` Daniel Vetter
2016-06-14 18:51 ` [PATCH 05/14] drm: Extract drm_master_open Daniel Vetter
2016-06-15 11:41   ` [Intel-gfx] " Chris Wilson
2016-06-14 18:51 ` [PATCH 06/14] drm: Extract drm_master_relase Daniel Vetter
2016-06-15 11:43   ` [Intel-gfx] " Chris Wilson
2016-06-16  8:18     ` Daniel Vetter
2016-06-14 18:51 ` [PATCH 07/14] drm: Only do the hw.lock cleanup in master_relase for !MODESET Daniel Vetter
2016-06-15 11:48   ` Chris Wilson
2016-06-15 15:49     ` [Intel-gfx] " Daniel Vetter
2016-06-14 18:51 ` [PATCH 08/14] drm: Move authmagic cleanup into drm_master_release Daniel Vetter
2016-06-15 11:51   ` Chris Wilson
2016-06-14 18:51 ` [PATCH 09/14] drm: Protect authmagic with master_mutex Daniel Vetter
2016-06-15 11:54   ` [Intel-gfx] " Chris Wilson
2016-06-15 15:52     ` Daniel Vetter
2016-06-14 18:51 ` [PATCH 10/14] drm: Mark authmagic ioctls as unlocked Daniel Vetter
2016-06-15 11:55   ` [Intel-gfx] " Chris Wilson
2016-06-14 18:51 ` [PATCH 11/14] drm: Mark set/drop master ioctl " Daniel Vetter
2016-06-15 11:56   ` [Intel-gfx] " Chris Wilson
2016-06-14 18:51 ` [PATCH 12/14] drm: Move master pointer from drm_minor to drm_device Daniel Vetter
2016-06-15 12:10   ` [Intel-gfx] " Chris Wilson
2016-06-15 16:01     ` Daniel Vetter
2016-06-15 16:33       ` Chris Wilson [this message]
2016-06-15 19:54         ` [Intel-gfx] " Daniel Vetter
2016-06-14 18:51 ` [PATCH 13/14] drm: Clean up drm_crtc.h Daniel Vetter
2016-06-14 18:51 ` [PATCH 14/14] drm: Use dev->name as fallback for dev->unique Daniel Vetter
2016-06-15  5:42 ` ✓ Ro.CI.BAT: success for Cruft removal around drm_master Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160615163314.GB6754@nuc-i3427.alporthouse.com \
    --to=chris@chris-wilson.co.uk \
    --cc=daniel.vetter@ffwll.ch \
    --cc=daniel.vetter@intel.com \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=julia.lawall@lip6.fr \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.