Re: [PATCH 13/16] drm: Refactor drop/set master code a bit

Hi Daniel,

It doesn't look quite right I'm afraid. This causes a leak plus there's a small style issue. See below for details.

On 17 June 2016 at 08:33, Daniel Vetter daniel.vetter@ffwll.ch wrote:

@@ -134,24 +152,21 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)

    /* take another reference for the copy in the local file priv */
    old_master = fpriv->master;

•   fpriv->master = drm_master_get(dev->master);
  

•   fpriv->master = drm_master_create(dev);
  

•   if (!fpriv->master)
  

•           return -ENOMEM;
  
  

Now dev->master != fpriv->master

    if (dev->driver->master_create) {
            ret = dev->driver->master_create(dev, fpriv->master);
            if (ret)
                    goto out_err;
    }

•   if (dev->driver->master_set) {
  

•           ret = dev->driver->master_set(dev, fpriv, true);
  

•           if (ret)
  

•                   goto out_err;
  

•   }
  

•   fpriv->is_master = 1;
    fpriv->allowed_master = 1;
    fpriv->authenticated = 1;
  

•   if (old_master)
  

•           drm_master_put(&old_master);
  

•   ret = drm_set_master(dev, fpriv, true);
  

+static int drm_set_master(struct drm_device *dev, struct drm_file *fpriv,

•                     bool new_master)
  

+{

•   int ret = 0;
  

•   dev->master = drm_master_get(fpriv->master);
  

drm_master_get() is defined as follows kref_get(&master->refcount); return master;

Since dev->master != fpriv->master, we'll leak the former.

+void drm_drop_master(struct drm_device *dev,

•                struct drm_file *fpriv)
  

Function is used only locally thus it should be static.

Regards, Emil