From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH] drm: fix returning -EINVAL on setmaster if another master is active Date: Thu, 11 Oct 2012 12:35:03 +0200 Message-ID: <1477778.PIc27Jio4Y@avalon> References: <1349632406-24068-1-git-send-email-dh.herrmann@googlemail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [95.142.166.194]) by gabe.freedesktop.org (Postfix) with ESMTP id 10C309E782 for ; Thu, 11 Oct 2012 03:34:19 -0700 (PDT) In-Reply-To: <1349632406-24068-1-git-send-email-dh.herrmann@googlemail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: dri-devel@lists.freedesktop.org Cc: Kristian Hoegsberg List-Id: dri-devel@lists.freedesktop.org Hi David, Would you have time to document the master_set operation in Documentation/DocBook/drm.tmpl ? :-) On Sunday 07 October 2012 19:53:26 David Herrmann wrote: > We link every DRM "file_priv" to a "drm_master" structure. Currently, the > drmSetMaster() call returns 0 when there is _any_ active master associated > with the "drm_master" structure of the calling "file_priv". This means, > that after drmSetMaster() we are not guaranteed to be DRM-Master and might > not be able to perform mode-setting. > > A way to reproduce this is by starting weston with the DRM backend from > within an X-console (eg., xterm). Because the xserver's "drm_master" is > currently active, weston is assigned to the same master but is inactive > because its VT is inactive and the xserver is still active. But when > "fake-activating" weston, it calls drmSetMaster(). With current behavior > this returns "0/success" and weston thinks that it is DRM-Master, even > though it is not (as the xserver is still DRM-Master). > Expected behavior would be drmSetMaster() to return -EINVAL, because the > xserver is still DRM-Master. This patch changes exactly that. > > The only way this bogus behavior would be useful is for clients to check > whether their associated "drm_master" is currently the active DRM-Master. > But this logic fails if no DRM-Master is currently active at all. Because > then the client itself would become DRM-Master (if it is root) and this > makes this whole thing useles. > > Also note that the second "if-condition": > file_priv->minor->master != file_priv->master > is always true and can be skipped. > > Signed-off-by: David Herrmann > --- > Note: > Note that this only removes the "if-clause". The code that performs the > setmaster() is actually left unchanged but makes the patch look scarier than > it really is. > > drivers/gpu/drm/drm_stub.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c > index c236fd2..581e61d 100644 > --- a/drivers/gpu/drm/drm_stub.c > +++ b/drivers/gpu/drm/drm_stub.c > @@ -221,20 +221,20 @@ int drm_setmaster_ioctl(struct drm_device *dev, void > *data, if (!file_priv->master) > return -EINVAL; > > - if (!file_priv->minor->master && > - file_priv->minor->master != file_priv->master) { > - mutex_lock(&dev->struct_mutex); > - file_priv->minor->master = drm_master_get(file_priv->master); > - file_priv->is_master = 1; > - if (dev->driver->master_set) { > - ret = dev->driver->master_set(dev, file_priv, false); > - if (unlikely(ret != 0)) { > - file_priv->is_master = 0; > - drm_master_put(&file_priv->minor->master); > - } > + if (file_priv->minor->master) > + return -EINVAL; > + > + mutex_lock(&dev->struct_mutex); > + file_priv->minor->master = drm_master_get(file_priv->master); > + file_priv->is_master = 1; > + if (dev->driver->master_set) { > + ret = dev->driver->master_set(dev, file_priv, false); > + if (unlikely(ret != 0)) { > + file_priv->is_master = 0; > + drm_master_put(&file_priv->minor->master); > } > - mutex_unlock(&dev->struct_mutex); > } > + mutex_unlock(&dev->struct_mutex); > > return 0; > } -- Regards, Laurent Pinchart