Skip to contentSkip to conversation

(unknown)

PATCH In review Thomas Hellstrom 3 participants 14 messages Mar 13, 2014

Subsystems (2)
DRM drivers DRM misc
lore
Share & tools
lore

Thomas Hellstrom

author
#0 lore raw

After a previous patch series and a discussion with Daniel Vetter and David Herrmann, I've reworked the patches a bit. Please review.

Patch 5 is already reviewed.

/Thomas

Quoted 1 line

From Thomas Hellstrom <[email protected]> # This line is ignored.

From: Thomas Hellstrom <[email protected]>
Subject:
In-Reply-To:

author
#1 lore raw

PATCH 1/5 drm: Have the crtc code only reference master from legacy nodes

control- and render nodes are intended to be master-less.

Signed-off-by Thomas Hellstrom <[email protected]>
---
 drivers/gpu/drm/drm_crtc.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
drivers/gpu/drm/drm_crtc.c +8 -7
index 3b7d32d..c9d895a 100644
@@ -1398,7 +1398,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	uint32_t __user *crtc_id;
 	uint32_t __user *connector_id;
 	uint32_t __user *encoder_id;
-	struct drm_mode_group *mode_group;
+	struct drm_mode_group *mode_group = NULL;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;

@@ -1429,8 +1429,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	mutex_unlock(&file_priv->fbs_lock);
 
 	drm_modeset_lock_all(dev);
-	mode_group = &file_priv->master->minor->mode_group;
-	if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
+	if (file_priv->minor->type != DRM_MINOR_LEGACY) {
 
 		list_for_each(lh, &dev->mode_config.crtc_list)
 			crtc_count++;

@@ -1442,6 +1441,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 			encoder_count++;
 	} else {
 
+		mode_group = &file_priv->master->minor->mode_group;
 		crtc_count = mode_group->num_crtcs;
 		connector_count = mode_group->num_connectors;
 		encoder_count = mode_group->num_encoders;

@@ -1456,7 +1456,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	if (card_res->count_crtcs >= crtc_count) {
 		copied = 0;
 		crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr;
-		if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
+		if (file_priv->minor->type != DRM_MINOR_LEGACY) {
 			list_for_each_entry(crtc, &dev->mode_config.crtc_list,
 					    head) {
 				DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);

@@ -1483,7 +1483,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	if (card_res->count_encoders >= encoder_count) {
 		copied = 0;
 		encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr;
-		if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
+		if (file_priv->minor->type != DRM_MINOR_LEGACY) {
 			list_for_each_entry(encoder,
 					    &dev->mode_config.encoder_list,
 					    head) {

@@ -1514,7 +1514,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	if (card_res->count_connectors >= connector_count) {
 		copied = 0;
 		connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr;
-		if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
+		if (file_priv->minor->type != DRM_MINOR_LEGACY) {
 			list_for_each_entry(connector,
 					    &dev->mode_config.connector_list,
 					    head) {

@@ -2715,7 +2715,8 @@ int drm_mode_getfb(struct drm_device *dev,
 	r->bpp = fb->bits_per_pixel;
 	r->pitch = fb->pitches[0];
 	if (fb->funcs->create_handle) {
-		if (file_priv->is_master || capable(CAP_SYS_ADMIN)) {
+		if (file_priv->is_master || capable(CAP_SYS_ADMIN) ||
+		    file_priv->minor->type == DRM_MINOR_CONTROL) {
 			ret = fb->funcs->create_handle(fb, file_priv,
 						       &r->handle);
 		} else {
Signature · 1 line
1.7.10.4
Reviewed-by on v1 1/5
#2 lore raw

Re: [PATCH 1/5] drm: Have the crtc code only reference master from legacy nodes

Hi

Quoted 12 lines · Thomas Hellstrom

control- and render nodes are intended to be master-less.

Signed-off-by Thomas Hellstrom <[email protected]>
---
 drivers/gpu/drm/drm_crtc.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
drivers/gpu/drm/drm_crtc.c +0 -0
index 3b7d32d..c9d895a 100644

drivers/gpu/drm/drm_crtc.c int drm_mode_getresources(struct drm_device *dev, void *data, · line 1401

Quoted 5 lines
drivers/gpu/drm/drm_crtc.c +1 -1
@@ -1398,7 +1398,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
        uint32_t __user *crtc_id;
        uint32_t __user *connector_id;
        uint32_t __user *encoder_id;
-       struct drm_mode_group *mode_group;
+       struct drm_mode_group *mode_group = NULL;

Why not move this mode_group=NULL; into the if() condition below? Avoids global initialization and makes the "if() - else" parts easier to read as you directly see that mode_group is set to NULL for non-legacy nodes.

Quoted 22 lines
drivers/gpu/drm/drm_crtc.c +2 -2
        if (!drm_core_check_feature(dev, DRIVER_MODESET))
                return -EINVAL;

@@ -1429,8 +1429,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
        mutex_unlock(&file_priv->fbs_lock);
 
        drm_modeset_lock_all(dev);
-       mode_group = &file_priv->master->minor->mode_group;
-       if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
+       if (file_priv->minor->type != DRM_MINOR_LEGACY) {
 
                list_for_each(lh, &dev->mode_config.crtc_list)
                        crtc_count++;

@@ -1442,6 +1441,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
                        encoder_count++;
        } else {
 
+               mode_group = &file_priv->master->minor->mode_group;
                crtc_count = mode_group->num_crtcs;
                connector_count = mode_group->num_connectors;
                encoder_count = mode_group->num_encoders;

drivers/gpu/drm/drm_crtc.c int drm_mode_getresources(struct drm_device *dev, void *data, · line 1459

Quoted 5 lines
drivers/gpu/drm/drm_crtc.c +1 -1
@@ -1456,7 +1456,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
        if (card_res->count_crtcs >= crtc_count) {
                copied = 0;
                crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr;
-               if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
+               if (file_priv->minor->type != DRM_MINOR_LEGACY) {

if (!mode_group) {

You set mode_group=NULL with your patch, so I think it's much easier to read if you replace all these minor-id-tests with "if (!mode_group)". This moves the group-selection to the head of the function and makes the remaining parts just work on the selected group (or global if NULL).

Same for the two conditions below..

Apart from that:

Reviewed-by David Herrmann <[email protected]>

Thanks
David

drivers/gpu/drm/drm_crtc.c int drm_mode_getresources(struct drm_device *dev, void *data, · line 1486

Quoted 9 lines
drivers/gpu/drm/drm_crtc.c +1 -1
                        list_for_each_entry(crtc, &dev->mode_config.crtc_list,
                                            head) {
                                DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);

@@ -1483,7 +1483,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
        if (card_res->count_encoders >= encoder_count) {
                copied = 0;
                encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr;
-               if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
+               if (file_priv->minor->type != DRM_MINOR_LEGACY) {

if (!mode_group) {

drivers/gpu/drm/drm_crtc.c int drm_mode_getresources(struct drm_device *dev, void *data, · line 1517

Quoted 9 lines
drivers/gpu/drm/drm_crtc.c +1 -1
                        list_for_each_entry(encoder,
                                            &dev->mode_config.encoder_list,
                                            head) {

@@ -1514,7 +1514,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
        if (card_res->count_connectors >= connector_count) {
                copied = 0;
                connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr;
-               if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
+               if (file_priv->minor->type != DRM_MINOR_LEGACY) {

if (!mode_group) {

Quoted 19 lines
drivers/gpu/drm/drm_crtc.c +2 -2
                        list_for_each_entry(connector,
                                            &dev->mode_config.connector_list,
                                            head) {

@@ -2715,7 +2715,8 @@ int drm_mode_getfb(struct drm_device *dev,
        r->bpp = fb->bits_per_pixel;
        r->pitch = fb->pitches[0];
        if (fb->funcs->create_handle) {
-               if (file_priv->is_master || capable(CAP_SYS_ADMIN)) {
+               if (file_priv->is_master || capable(CAP_SYS_ADMIN) ||
+                   file_priv->minor->type == DRM_MINOR_CONTROL) {
                        ret = fb->funcs->create_handle(fb, file_priv,
                                                       &r->handle);
                } else {
--

1.7.10.4
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

author
#3 lore raw

PATCH 2/5 drm: Break out ioctl permission check to a separate function

In reply to Thomas Hellstrom

Helps reviewing and understanding these checks.

Signed-off-by Thomas Hellstrom <[email protected]>
---
 drivers/gpu/drm/drm_drv.c |  116 ++++++++++++++++++++++++++++++---------------
 1 file changed, 78 insertions(+), 38 deletions(-)
drivers/gpu/drm/drm_drv.c +78 -38
index 345be03..0afc6e4 100644
@@ -285,6 +285,47 @@ static int drm_version(struct drm_device *dev, void *data,
 	return err;
 }
 
+
+/**
+ * drm_ioctl_permit - Check ioctl permissions against caller
+ *
+ * @flags: ioctl permission flags.
+ * @file_priv: Pointer to struct drm_file identifying the caller.
+ *
+ * Checks whether the caller is allowed to run an ioctl with the
+ * indicated permissions. If so, returns zero. Otherwise returns an
+ * error code suitable for ioctl return.
+ */
+static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
+{
+
+	/* ROOT_ONLY is only for CAP_SYS_ADMIN */
+	if (unlikely((flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)))
+		return -EACCES;
+
+	/* AUTH is only for authenticated or render client */
+	if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) &&
+		     !file_priv->authenticated))
+		return -EACCES;
+
+	/* MASTER is only for master */
+	if (unlikely((flags & DRM_MASTER) && !file_priv->is_master))
+		return -EACCES;
+
+	/* Control clients must be explicitly allowed */
+	if (unlikely(!(flags & DRM_CONTROL_ALLOW) &&
+		     file_priv->minor->type == DRM_MINOR_CONTROL))
+		return -EACCES;
+
+	/* Render clients must be explicitly allowed */
+	if (unlikely(!(flags & DRM_RENDER_ALLOW) &&
+		     drm_is_render_client(file_priv)))
+		return -EACCES;
+
+	return 0;
+}
+
+
 /**
  * Called whenever a process performs an ioctl on /dev/drm.
  *

@@ -350,52 +391,51 @@ long drm_ioctl(struct file *filp,
 	/* Do not trust userspace, use our own definition */
 	func = ioctl->func;
 
-	if (!func) {
+	if (unlikely(!func)) {
 		DRM_DEBUG("no function\n");
 		retcode = -EINVAL;
-	} else if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) ||
-		   ((ioctl->flags & DRM_AUTH) && !drm_is_render_client(file_priv) && !file_priv->authenticated) ||
-		   ((ioctl->flags & DRM_MASTER) && !file_priv->is_master) ||
-		   (!(ioctl->flags & DRM_CONTROL_ALLOW) && (file_priv->minor->type == DRM_MINOR_CONTROL)) ||
-		   (!(ioctl->flags & DRM_RENDER_ALLOW) && drm_is_render_client(file_priv))) {
-		retcode = -EACCES;
-	} else {
-		if (cmd & (IOC_IN | IOC_OUT)) {
-			if (asize <= sizeof(stack_kdata)) {
-				kdata = stack_kdata;
-			} else {
-				kdata = kmalloc(asize, GFP_KERNEL);
-				if (!kdata) {
-					retcode = -ENOMEM;
-					goto err_i1;
-				}
-			}
-			if (asize > usize)
-				memset(kdata + usize, 0, asize - usize);
-		}
+		goto err_i1;
+	}
 
-		if (cmd & IOC_IN) {
-			if (copy_from_user(kdata, (void __user *)arg,
-					   usize) != 0) {
-				retcode = -EFAULT;
+	retcode = drm_ioctl_permit(ioctl->flags, file_priv);
+	if (unlikely(retcode))
+		goto err_i1;
+
+	if (cmd & (IOC_IN | IOC_OUT)) {
+		if (asize <= sizeof(stack_kdata)) {
+			kdata = stack_kdata;
+		} else {
+			kdata = kmalloc(asize, GFP_KERNEL);
+			if (!kdata) {
+				retcode = -ENOMEM;
 				goto err_i1;
 			}
-		} else
-			memset(kdata, 0, usize);
-
-		if (ioctl->flags & DRM_UNLOCKED)
-			retcode = func(dev, kdata, file_priv);
-		else {
-			mutex_lock(&drm_global_mutex);
-			retcode = func(dev, kdata, file_priv);
-			mutex_unlock(&drm_global_mutex);
 		}
+		if (asize > usize)
+			memset(kdata + usize, 0, asize - usize);
+	}
 
-		if (cmd & IOC_OUT) {
-			if (copy_to_user((void __user *)arg, kdata,
-					 usize) != 0)
-				retcode = -EFAULT;
+	if (cmd & IOC_IN) {
+		if (copy_from_user(kdata, (void __user *)arg,
+				   usize) != 0) {
+			retcode = -EFAULT;
+			goto err_i1;
 		}
+	} else
+		memset(kdata, 0, usize);
+
+	if (ioctl->flags & DRM_UNLOCKED)
+		retcode = func(dev, kdata, file_priv);
+	else {
+		mutex_lock(&drm_global_mutex);
+		retcode = func(dev, kdata, file_priv);
+		mutex_unlock(&drm_global_mutex);
+	}
+
+	if (cmd & IOC_OUT) {
+		if (copy_to_user((void __user *)arg, kdata,
+				 usize) != 0)
+			retcode = -EFAULT;
 	}
 
       err_i1:
Signature · 1 line
1.7.10.4
Reviewed-by on v1 2/5
#4 lore raw

Re: [PATCH 2/5] drm: Break out ioctl permission check to a separate function

Hi

Quoted 12 lines · Thomas Hellstrom

Helps reviewing and understanding these checks.

Signed-off-by Thomas Hellstrom <[email protected]>
---
 drivers/gpu/drm/drm_drv.c |  116 ++++++++++++++++++++++++++++++---------------
 1 file changed, 78 insertions(+), 38 deletions(-)
drivers/gpu/drm/drm_drv.c +0 -0
index 345be03..0afc6e4 100644

drivers/gpu/drm/drm_drv.c static int drm_version(struct drm_device *dev, void *data, · line 288

Quoted 4 lines
drivers/gpu/drm/drm_drv.c +1 -0
@@ -285,6 +285,47 @@ static int drm_version(struct drm_device *dev, void *data,
        return err;
 }
 
+

Why the double blank line?

drivers/gpu/drm/drm_drv.c static int drm_version(struct drm_device *dev, void *data, · line 301

Quoted 13 lines
drivers/gpu/drm/drm_drv.c +13 -0
+/**
+ * drm_ioctl_permit - Check ioctl permissions against caller
+ *
+ * @flags: ioctl permission flags.
+ * @file_priv: Pointer to struct drm_file identifying the caller.
+ *
+ * Checks whether the caller is allowed to run an ioctl with the
+ * indicated permissions. If so, returns zero. Otherwise returns an
+ * error code suitable for ioctl return.
+ */
+static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
+{
+

We don't do blank lines after function-headers.

Quoted 17 lines
drivers/gpu/drm/drm_drv.c +17 -0
+       /* ROOT_ONLY is only for CAP_SYS_ADMIN */
+       if (unlikely((flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)))
+               return -EACCES;
+
+       /* AUTH is only for authenticated or render client */
+       if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) &&
+                    !file_priv->authenticated))
+               return -EACCES;
+
+       /* MASTER is only for master */
+       if (unlikely((flags & DRM_MASTER) && !file_priv->is_master))
+               return -EACCES;
+
+       /* Control clients must be explicitly allowed */
+       if (unlikely(!(flags & DRM_CONTROL_ALLOW) &&
+                    file_priv->minor->type == DRM_MINOR_CONTROL))
+               return -EACCES;

drivers/gpu/drm/drm_drv.c static int drm_version(struct drm_device *dev, void *data, · line 328

Quoted 10 lines
drivers/gpu/drm/drm_drv.c +10 -0
+
+       /* Render clients must be explicitly allowed */
+       if (unlikely(!(flags & DRM_RENDER_ALLOW) &&
+                    drm_is_render_client(file_priv)))
+               return -EACCES;
+
+       return 0;
+}
+
+

Again, double blank-line.

Quoted 30 lines
drivers/gpu/drm/drm_drv.c +1 -20
 /**
  * Called whenever a process performs an ioctl on /dev/drm.
  *

@@ -350,52 +391,51 @@ long drm_ioctl(struct file *filp,
        /* Do not trust userspace, use our own definition */
        func = ioctl->func;
 
-       if (!func) {
+       if (unlikely(!func)) {
                DRM_DEBUG("no function\n");
                retcode = -EINVAL;
-       } else if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) ||
-                  ((ioctl->flags & DRM_AUTH) && !drm_is_render_client(file_priv) && !file_priv->authenticated) ||
-                  ((ioctl->flags & DRM_MASTER) && !file_priv->is_master) ||
-                  (!(ioctl->flags & DRM_CONTROL_ALLOW) && (file_priv->minor->type == DRM_MINOR_CONTROL)) ||
-                  (!(ioctl->flags & DRM_RENDER_ALLOW) && drm_is_render_client(file_priv))) {
-               retcode = -EACCES;
-       } else {
-               if (cmd & (IOC_IN | IOC_OUT)) {
-                       if (asize <= sizeof(stack_kdata)) {
-                               kdata = stack_kdata;
-                       } else {
-                               kdata = kmalloc(asize, GFP_KERNEL);
-                               if (!kdata) {
-                                       retcode = -ENOMEM;
-                                       goto err_i1;
-                               }
-                       }
-                       if (asize > usize)
-                               memset(kdata + usize, 0, asize - usize);

drivers/gpu/drm/drm_drv.c long drm_ioctl(struct file *filp, · line 401

Quoted 10 lines
drivers/gpu/drm/drm_drv.c +4 -5
-               }
+               goto err_i1;
+       }
 
-               if (cmd & IOC_IN) {
-                       if (copy_from_user(kdata, (void __user *)arg,
-                                          usize) != 0) {
-                               retcode = -EFAULT;
+       retcode = drm_ioctl_permit(ioctl->flags, file_priv);
+       if (unlikely(retcode))

That "unlikely" seems redundant given that all error paths in drm_ioctl_permit() already are "unlikely".

Otherwise, patch looks good:

Reviewed-by David Herrmann <[email protected]>

Thanks
David

Quoted 59 lines
drivers/gpu/drm/drm_drv.c +32 -14
+               goto err_i1;
+
+       if (cmd & (IOC_IN | IOC_OUT)) {
+               if (asize <= sizeof(stack_kdata)) {
+                       kdata = stack_kdata;
+               } else {
+                       kdata = kmalloc(asize, GFP_KERNEL);
+                       if (!kdata) {
+                               retcode = -ENOMEM;
                                goto err_i1;
                        }
-               } else
-                       memset(kdata, 0, usize);
-
-               if (ioctl->flags & DRM_UNLOCKED)
-                       retcode = func(dev, kdata, file_priv);
-               else {
-                       mutex_lock(&drm_global_mutex);
-                       retcode = func(dev, kdata, file_priv);
-                       mutex_unlock(&drm_global_mutex);
                }
+               if (asize > usize)
+                       memset(kdata + usize, 0, asize - usize);
+       }
 
-               if (cmd & IOC_OUT) {
-                       if (copy_to_user((void __user *)arg, kdata,
-                                        usize) != 0)
-                               retcode = -EFAULT;
+       if (cmd & IOC_IN) {
+               if (copy_from_user(kdata, (void __user *)arg,
+                                  usize) != 0) {
+                       retcode = -EFAULT;
+                       goto err_i1;
                }
+       } else
+               memset(kdata, 0, usize);
+
+       if (ioctl->flags & DRM_UNLOCKED)
+               retcode = func(dev, kdata, file_priv);
+       else {
+               mutex_lock(&drm_global_mutex);
+               retcode = func(dev, kdata, file_priv);
+               mutex_unlock(&drm_global_mutex);
+       }
+
+       if (cmd & IOC_OUT) {
+               if (copy_to_user((void __user *)arg, kdata,
+                                usize) != 0)
+                       retcode = -EFAULT;
        }
 
       err_i1:
--

1.7.10.4
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

author
#5 lore raw

Re: [PATCH 2/5] drm: Break out ioctl permission check to a separate function

Hi.

Thanks for reviewing. I'll incorporate your suggestions, except this one, and resend.

Quoted 4 lines · David Herrmann

Hi

On Thu, Mar 13, 2014 at 11:57 AM, Thomas Hellstrom <[email protected]> wrote:

...

- if (cmd & IOC_IN) {
- if (copy_from_user(kdata, (void __user *)arg,
- usize) != 0) {
- retcode = -EFAULT;
+ retcode = drm_ioctl_permit(ioctl->flags, file_priv);
+ if (unlikely(retcode))

Quoted 2 lines

That "unlikely" seems redundant given that all error paths in drm_ioctl_permit() already are "unlikely".

Yes, we know that's true, but I don't think compilers in general can combine branch prediction hints in that way,
or even have the information necessary to do it.
I mean even if each individual test resulting in an error is unlikely, how could the compiler know that
all tests combined would result in an error being unlikely?

/Thomas

#6 lore raw

Re: [PATCH 2/5] drm: Break out ioctl permission check to a separate function

Hi

Quoted 23 lines · Thomas Hellstrom

Hi.

Thanks for reviewing. I'll incorporate your suggestions, except this one, and resend.

Quoted 4 lines · David Herrmann

Hi

On Thu, Mar 13, 2014 at 11:57 AM, Thomas Hellstrom <[email protected]> wrote:

...

drivers/gpu/drm/drm_drv.c +2 -4
-               if (cmd & IOC_IN) {
-                       if (copy_from_user(kdata, (void __user *)arg,
-                                          usize) != 0) {
-                               retcode = -EFAULT;
+       retcode = drm_ioctl_permit(ioctl->flags, file_priv);
+       if (unlikely(retcode))
Quoted 3 lines

That "unlikely" seems redundant given that all error paths in drm_ioctl_permit() already are "unlikely".

Quoted 6 lines

Yes, we know that's true, but I don't think compilers in general can combine branch prediction hints in that way,
or even have the information necessary to do it.
I mean even if each individual test resulting in an error is unlikely, how could the compiler know that
all tests combined would result in an error being unlikely?

The function is static, so the compiler can see that it returns "!=0" only if one of the "unlikely" branches was hit. So I think it's safe to assume the whole thing returns "!=0" only in unlikely conditions. But it's probably inlined, anyway..

I'm no big fan of excessive likely/unlikely annotations, but I'm fine if you want to keep it.

Thanks
David

author
#7 lore raw

Re: [PATCH 2/5] drm: Break out ioctl permission check to a separate function

Quoted 31 lines · David Herrmann

Hi

Quoted 20 lines · Thomas Hellstrom

Hi.

Thanks for reviewing. I'll incorporate your suggestions, except this one, and resend.

Quoted 4 lines · David Herrmann

Hi

On Thu, Mar 13, 2014 at 11:57 AM, Thomas Hellstrom <[email protected]> wrote:

...

drivers/gpu/drm/drm_drv.c +2 -4
-               if (cmd & IOC_IN) {
-                       if (copy_from_user(kdata, (void __user *)arg,
-                                          usize) != 0) {
-                               retcode = -EFAULT;
+       retcode = drm_ioctl_permit(ioctl->flags, file_priv);
+       if (unlikely(retcode))
Quoted 8 lines
Quoted 2 lines

That "unlikely" seems redundant given that all error paths in drm_ioctl_permit() already are "unlikely".

Yes, we know that's true, but I don't think compilers in general can combine branch prediction hints in that way,
or even have the information necessary to do it.
I mean even if each individual test resulting in an error is unlikely, how could the compiler know that
all tests combined would result in an error being unlikely?

Quoted 4 lines

The function is static, so the compiler can see that it returns "!=0" only if one of the "unlikely" branches was hit. So I think it's safe to assume the whole thing returns "!=0" only in unlikely conditions.

But a compiler can't (or shouldn't) make that assumption. Just as an (adapted) example, imagine that
each test had a 20% probability of returning an error. The probability of the function returning an error would
then be 68%..

Quoted 2 lines

I'm no big fan of excessive likely/unlikely annotations, but I'm fine if you want to keep it.

Fair enough.

Thanks,
Thomas

Quoted 7 lines

Thanks
David
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

#8 lore raw

Re: [PATCH 2/5] drm: Break out ioctl permission check to a separate function

Quoted 5 lines · Thomas Hellstrom

But a compiler can't (or shouldn't) make that assumption. Just as an (adapted) example, imagine that
each test had a 20% probability of returning an error. The probability of the function returning an error would
then be 68%..

Otoh if you'd put the unlikely just onto the if (ret) then the compiler could infer that by necessity all branches leading towards this one are also unlikely. Dunno whether compilers are this clever though, and I also don't really care if we throw a few too many likely/unlikely annotations over the place. Just figured I'll throw this in.
-Daniel

Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
author
#9 lore raw

PATCH 3/5 drm: Make control nodes master-less v2

In reply to Thomas Hellstrom

Like for render-nodes, there is no point in maintaining the master concept for control nodes, so set the struct drm_file::master pointer to NULL.

At the same time, make sure DRM_MASTER | DRM_CONTROL_ALLOW ioctls are always allowed when called through the control node. Previously the caller also needed to be master.

v2: Adapt to refactoring of ioctl permission check.

Signed-off-by Thomas Hellstrom <[email protected]>
---
 drivers/gpu/drm/drm_drv.c  |    9 +++++----
 drivers/gpu/drm/drm_fops.c |    5 +++--
 include/drm/drmP.h         |    5 +++++
 3 files changed, 13 insertions(+), 6 deletions(-)
drivers/gpu/drm/drm_drv.c +5 -4
index 0afc6e4..e41ee82 100644
@@ -307,14 +307,15 @@ static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
 	if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) &&
 		     !file_priv->authenticated))
 		return -EACCES;
-
-	/* MASTER is only for master */
-	if (unlikely((flags & DRM_MASTER) && !file_priv->is_master))
+	
+	/* MASTER is only for master or control clients */
+	if (unlikely((flags & DRM_MASTER) &&
+		     !(file_priv->is_master || drm_is_control(file_priv))))
 		return -EACCES;
 
 	/* Control clients must be explicitly allowed */
 	if (unlikely(!(flags & DRM_CONTROL_ALLOW) &&
-		     file_priv->minor->type == DRM_MINOR_CONTROL))
+		     drm_is_control(file_priv)))
 		return -EACCES;
 
 	/* Render clients must be explicitly allowed */
drivers/gpu/drm/drm_fops.c +3 -2
index 7f2af9a..08a3196 100644
@@ -259,7 +259,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
 	/* if there is no current master make this fd it, but do not create
 	 * any master object for render clients */
 	mutex_lock(&dev->struct_mutex);
-	if (!priv->minor->master && !drm_is_render_client(priv)) {
+	if (!priv->minor->master && !drm_is_render_client(priv) &&
+	    !drm_is_control(priv)) {
 		/* create a new master */
 		priv->minor->master = drm_master_create(priv->minor);
 		if (!priv->minor->master) {

@@ -297,7 +298,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
 				goto out_close;
 			}
 		}
-	} else if (!drm_is_render_client(priv)) {
+	} else if (!drm_is_render_client(priv) && !drm_is_control(priv)) {
 		/* get a reference to the master */
 		priv->master = drm_master_get(priv->minor->master);
 	}
include/drm/drmP.h +5 -0
index 04a7f31..ff68e26 100644
@@ -1246,6 +1246,11 @@ static inline bool drm_is_render_client(struct drm_file *file_priv)
 	return file_priv->minor->type == DRM_MINOR_RENDER;
 }
 
+static inline bool drm_is_control(struct drm_file *file_priv)
+{
+	return file_priv->minor->type == DRM_MINOR_CONTROL;
+}
+
 /******************************************************************/
 /** \name Internal function definitions */
 /*@{*/
Signature · 1 line
1.7.10.4
#10 lore raw

Re: [PATCH 3/5] drm: Make control nodes master-less v2

Hi

Quoted 21 lines · Thomas Hellstrom

Like for render-nodes, there is no point in maintaining the master concept for control nodes, so set the struct drm_file::master pointer to NULL.

At the same time, make sure DRM_MASTER | DRM_CONTROL_ALLOW ioctls are always allowed when called through the control node. Previously the caller also needed to be master.

v2: Adapt to refactoring of ioctl permission check.

Signed-off-by Thomas Hellstrom <[email protected]>
---
 drivers/gpu/drm/drm_drv.c  |    9 +++++----
 drivers/gpu/drm/drm_fops.c |    5 +++--
 include/drm/drmP.h         |    5 +++++
 3 files changed, 13 insertions(+), 6 deletions(-)
drivers/gpu/drm/drm_drv.c +0 -0
index 0afc6e4..e41ee82 100644

drivers/gpu/drm/drm_drv.c static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv) · line 313

Quoted 10 lines
drivers/gpu/drm/drm_drv.c +4 -3
@@ -307,14 +307,15 @@ static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
        if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) &&
                     !file_priv->authenticated))
                return -EACCES;
-
-       /* MASTER is only for master */
-       if (unlikely((flags & DRM_MASTER) && !file_priv->is_master))
+
+       /* MASTER is only for master or control clients */
+       if (unlikely((flags & DRM_MASTER) &&
+                    !(file_priv->is_master || drm_is_control(file_priv))))

imo this looks nicer:
(flags & XY) && !is_master && !drm_is_control()

but that's probably a matter of taste

Quoted 37 lines
drivers/gpu/drm/drm_drv.c +1 -1
                return -EACCES;
 
        /* Control clients must be explicitly allowed */
        if (unlikely(!(flags & DRM_CONTROL_ALLOW) &&
-                    file_priv->minor->type == DRM_MINOR_CONTROL))
+                    drm_is_control(file_priv)))
                return -EACCES;
 
        /* Render clients must be explicitly allowed */
drivers/gpu/drm/drm_fops.c +3 -2
index 7f2af9a..08a3196 100644
@@ -259,7 +259,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
        /* if there is no current master make this fd it, but do not create
         * any master object for render clients */
        mutex_lock(&dev->struct_mutex);
-       if (!priv->minor->master && !drm_is_render_client(priv)) {
+       if (!priv->minor->master && !drm_is_render_client(priv) &&
+           !drm_is_control(priv)) {
                /* create a new master */
                priv->minor->master = drm_master_create(priv->minor);
                if (!priv->minor->master) {

@@ -297,7 +298,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
                                goto out_close;
                        }
                }
-       } else if (!drm_is_render_client(priv)) {
+       } else if (!drm_is_render_client(priv) && !drm_is_control(priv)) {
                /* get a reference to the master */
                priv->master = drm_master_get(priv->minor->master);
        }
include/drm/drmP.h +0 -0
index 04a7f31..ff68e26 100644

include/drm/drmP.h static inline bool drm_is_render_client(struct drm_file *file_priv) · line 1249

Quoted 4 lines
include/drm/drmP.h +1 -0
@@ -1246,6 +1246,11 @@ static inline bool drm_is_render_client(struct drm_file *file_priv)
        return file_priv->minor->type == DRM_MINOR_RENDER;
 }
 
+static inline bool drm_is_control(struct drm_file *file_priv)

drm_is_control()? Pretty inexpressive.. Why not keep the _client suffix? drm_is_control_client()..

Thanks
David

Quoted 13 lines
include/drm/drmP.h +4 -1
+{
+       return file_priv->minor->type == DRM_MINOR_CONTROL;
+}
+
 /******************************************************************/
 /** \name Internal function definitions */
 /*@{*/
--

1.7.10.4
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

author
#11 lore raw

PATCH 4/5 drm: Improve on minor type helpers

In reply to Thomas Hellstrom

Add a drm_is_legacy() helper, constify argument to drm_is_render_client(), and use / change helpers where appropriate.

Signed-off-by Thomas Hellstrom <[email protected]>
---
 drivers/gpu/drm/drm_crtc.c |   10 +++++-----
 drivers/gpu/drm/drm_fops.c |    5 ++---
 include/drm/drmP.h         |    9 +++++++--
 3 files changed, 14 insertions(+), 10 deletions(-)
drivers/gpu/drm/drm_crtc.c +5 -5
index c9d895a..4190c7e 100644
@@ -1429,7 +1429,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	mutex_unlock(&file_priv->fbs_lock);
 
 	drm_modeset_lock_all(dev);
-	if (file_priv->minor->type != DRM_MINOR_LEGACY) {
+	if (!drm_is_legacy(file_priv)) {
 
 		list_for_each(lh, &dev->mode_config.crtc_list)
 			crtc_count++;

@@ -1456,7 +1456,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	if (card_res->count_crtcs >= crtc_count) {
 		copied = 0;
 		crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr;
-		if (file_priv->minor->type != DRM_MINOR_LEGACY) {
+		if (!drm_is_legacy(file_priv)) {
 			list_for_each_entry(crtc, &dev->mode_config.crtc_list,
 					    head) {
 				DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);

@@ -1483,7 +1483,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	if (card_res->count_encoders >= encoder_count) {
 		copied = 0;
 		encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr;
-		if (file_priv->minor->type != DRM_MINOR_LEGACY) {
+		if (!drm_is_legacy(file_priv)) {
 			list_for_each_entry(encoder,
 					    &dev->mode_config.encoder_list,
 					    head) {

@@ -1514,7 +1514,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	if (card_res->count_connectors >= connector_count) {
 		copied = 0;
 		connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr;
-		if (file_priv->minor->type != DRM_MINOR_LEGACY) {
+		if (!drm_is_legacy(file_priv)) {
 			list_for_each_entry(connector,
 					    &dev->mode_config.connector_list,
 					    head) {

@@ -2716,7 +2716,7 @@ int drm_mode_getfb(struct drm_device *dev,
 	r->pitch = fb->pitches[0];
 	if (fb->funcs->create_handle) {
 		if (file_priv->is_master || capable(CAP_SYS_ADMIN) ||
-		    file_priv->minor->type == DRM_MINOR_CONTROL) {
+		    drm_is_control(file_priv)) {
 			ret = fb->funcs->create_handle(fb, file_priv,
 						       &r->handle);
 		} else {
drivers/gpu/drm/drm_fops.c +2 -3
index 08a3196..31a4655 100644
@@ -259,8 +259,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
 	/* if there is no current master make this fd it, but do not create
 	 * any master object for render clients */
 	mutex_lock(&dev->struct_mutex);
-	if (!priv->minor->master && !drm_is_render_client(priv) &&
-	    !drm_is_control(priv)) {
+	if (drm_is_legacy(priv) && !priv->minor->master) {
 		/* create a new master */
 		priv->minor->master = drm_master_create(priv->minor);
 		if (!priv->minor->master) {

@@ -298,7 +297,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
 				goto out_close;
 			}
 		}
-	} else if (!drm_is_render_client(priv) && !drm_is_control(priv)) {
+	} else if (drm_is_legacy(priv)) {
 		/* get a reference to the master */
 		priv->master = drm_master_get(priv->minor->master);
 	}
include/drm/drmP.h +7 -2
index ff68e26..5db7f86 100644
@@ -1241,16 +1241,21 @@ static inline bool drm_modeset_is_locked(struct drm_device *dev)
 	return mutex_is_locked(&dev->mode_config.mutex);
 }
 
-static inline bool drm_is_render_client(struct drm_file *file_priv)
+static inline bool drm_is_render_client(const struct drm_file *file_priv)
 {
 	return file_priv->minor->type == DRM_MINOR_RENDER;
 }
 
-static inline bool drm_is_control(struct drm_file *file_priv)
+static inline bool drm_is_control(const struct drm_file *file_priv)
 {
 	return file_priv->minor->type == DRM_MINOR_CONTROL;
 }
 
+static inline bool drm_is_legacy(const struct drm_file *file_priv)
+{
+	return file_priv->minor->type == DRM_MINOR_LEGACY;
+}
+
 /******************************************************************/
 /** \name Internal function definitions */
 /*@{*/
Signature · 1 line
1.7.10.4
author
#12 lore raw

PATCH 5/5 drm: Remove the minor master list

In reply to Thomas Hellstrom

It doesn't appear to be used anywhere.

Signed-off-by Thomas Hellstrom <[email protected]>
Reviewed-by David Herrmann <[email protected]>
---
 drivers/gpu/drm/drm_stub.c |    5 -----
 include/drm/drmP.h         |    2 --
 2 files changed, 7 deletions(-)
drivers/gpu/drm/drm_stub.c +0 -5
index 98a33c580..4f17c79 100644
@@ -152,8 +152,6 @@ struct drm_master *drm_master_create(struct drm_minor *minor)
 	INIT_LIST_HEAD(&master->magicfree);
 	master->minor = minor;
 
-	list_add_tail(&master->head, &minor->master_list);
-
 	return master;
 }
 

@@ -171,8 +169,6 @@ static void drm_master_destroy(struct kref *kref)
 	struct drm_device *dev = master->minor->dev;
 	struct drm_map_list *r_list, *list_temp;
 
-	list_del(&master->head);
-
 	if (dev->driver->master_destroy)
 		dev->driver->master_destroy(dev, master);
 

@@ -296,7 +292,6 @@ static int drm_get_minor(struct drm_device *dev, struct drm_minor **minor,
 	new_minor->device = MKDEV(DRM_MAJOR, minor_id);
 	new_minor->dev = dev;
 	new_minor->index = minor_id;
-	INIT_LIST_HEAD(&new_minor->master_list);
 
 	idr_replace(&drm_minors_idr, new_minor, minor_id);
include/drm/drmP.h +0 -2
index 5db7f86..33e55c7 100644
@@ -718,7 +718,6 @@ struct drm_master {
 
 	struct kref refcount; /* refcount for this master */
 
-	struct list_head head; /**< each minor contains a list of masters */
 	struct drm_minor *minor; /**< link back to minor we are a master for */
 
 	char *unique;			/**< Unique identifier: e.g., busid */

@@ -1050,7 +1049,6 @@ struct drm_minor {
 	struct mutex debugfs_lock; /* Protects debugfs_list. */
 
 	struct drm_master *master; /* currently active master for this node */
-	struct list_head master_list;
 	struct drm_mode_group mode_group;
 };
Signature · 1 line
1.7.10.4
author
#13 lore raw

cover Control node master fixes WAS:

In reply to Thomas Hellstrom

Hmm. Screwed up git-send-email a bit :(
Resending with new subject.

/Thomas

Quoted 15 lines · Thomas Hellstrom

After a previous patch series and a discussion with Daniel Vetter and David Herrmann, I've reworked the patches a bit. Please review.

Patch 5 is already reviewed.

/Thomas

Quoted 1 line

From Thomas Hellstrom <[email protected]> # This line is ignored.

From: Thomas Hellstrom <[email protected]>
Subject:
In-Reply-To:
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel