diff mbox

[4/4] drm: Simplify GETRESOURCES ioctl

Message ID 20161210215255.7765-4-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Vetter Dec. 10, 2016, 9:52 p.m. UTC
Looping twice when we can do it once is silly. Also use a consistent
style. Note that there's a good race with the connector list walking,
since that is no longer protected by mode_config.mutex. But that's for
a later patch to fix.

v2: Actually try to not blow up, somehow I lost the hunk that checks
we don't copy too much. Noticed by Chris.

v3:
- squash all drm_mode_getresources cleanups into one
- use consistent style for walking objects (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_mode_config.c | 119 +++++++++++++++-----------------------
 1 file changed, 47 insertions(+), 72 deletions(-)

Comments

Chris Wilson Dec. 10, 2016, 10:04 p.m. UTC | #1
On Sat, Dec 10, 2016 at 10:52:55PM +0100, Daniel Vetter wrote:
> Looping twice when we can do it once is silly. Also use a consistent
> style. Note that there's a good race with the connector list walking,
> since that is no longer protected by mode_config.mutex. But that's for
> a later patch to fix.
> 
> v2: Actually try to not blow up, somehow I lost the hunk that checks
> we don't copy too much. Noticed by Chris.
> 
> v3:
> - squash all drm_mode_getresources cleanups into one
> - use consistent style for walking objects (Chris)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_mode_config.c | 119 +++++++++++++++-----------------------
>  1 file changed, 47 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 2735a5847ffa..54e371dac694 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -84,17 +84,11 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
>  			  struct drm_file *file_priv)
>  {
>  	struct drm_mode_card_res *card_res = data;
> -	struct list_head *lh;
>  	struct drm_framebuffer *fb;
>  	struct drm_connector *connector;
>  	struct drm_crtc *crtc;
>  	struct drm_encoder *encoder;
> -	int ret = 0;
> -	int connector_count = 0;
> -	int crtc_count = 0;
> -	int fb_count = 0;
> -	int encoder_count = 0;
> -	int copied = 0;
> +	int count, ret = 0;
>  	uint32_t __user *fb_id;
>  	uint32_t __user *crtc_id;
>  	uint32_t __user *connector_id;
> @@ -105,89 +99,70 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
>  
>  
>  	mutex_lock(&file_priv->fbs_lock);
> -	/*
> -	 * For the non-control nodes we need to limit the list of resources
> -	 * by IDs in the group list for this node
> -	 */
> -	list_for_each(lh, &file_priv->fbs)
> -		fb_count++;
> -
> -	/* handle this in 4 parts */
> -	/* FBs */
> -	if (card_res->count_fbs >= fb_count) {
> -		copied = 0;
> -		fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;
> -		list_for_each_entry(fb, &file_priv->fbs, filp_head) {
> -			if (put_user(fb->base.id, fb_id + copied)) {
> -				mutex_unlock(&file_priv->fbs_lock);
> -				return -EFAULT;
> -			}
> -			copied++;
> +	count = 0;
> +	fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;

u64_to_user_ptr() please :)

> +	list_for_each_entry(fb, &file_priv->fbs, filp_head) {
> +		count++;
> +		if (count > card_res->count_fbs)
> +			continue;
> +
> +		if (put_user(fb->base.id, fb_id + count)) {

In this style increment of count has to happen after the copy.

i.e.
	if (count < card_res->count_fbs &&
	    put_user(fb->base.id, fb_id + count) {
	    mutex_unlock()
	    return -EFAULT;
	}
	count++;
 

> +			mutex_unlock(&file_priv->fbs_lock);
> +			return -EFAULT;
>  		}
>  	}
> -	card_res->count_fbs = fb_count;
> +	card_res->count_fbs = count;
>  	mutex_unlock(&file_priv->fbs_lock);
Daniel Vetter Dec. 11, 2016, 10:55 a.m. UTC | #2
On Sat, Dec 10, 2016 at 11:04 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> +     list_for_each_entry(fb, &file_priv->fbs, filp_head) {
>> +             count++;
>> +             if (count > card_res->count_fbs)
>> +                     continue;
>> +
>> +             if (put_user(fb->base.id, fb_id + count)) {
>
> In this style increment of count has to happen after the copy.
>
> i.e.
>         if (count < card_res->count_fbs &&
>             put_user(fb->base.id, fb_id + count) {
>             mutex_unlock()
>             return -EFAULT;
>         }
>         count++;

Note I also have > instead of  <, so I think it should be equivalent.
Oops except for the connector lop, silly me that lost one.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 2735a5847ffa..54e371dac694 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -84,17 +84,11 @@  int drm_mode_getresources(struct drm_device *dev, void *data,
 			  struct drm_file *file_priv)
 {
 	struct drm_mode_card_res *card_res = data;
-	struct list_head *lh;
 	struct drm_framebuffer *fb;
 	struct drm_connector *connector;
 	struct drm_crtc *crtc;
 	struct drm_encoder *encoder;
-	int ret = 0;
-	int connector_count = 0;
-	int crtc_count = 0;
-	int fb_count = 0;
-	int encoder_count = 0;
-	int copied = 0;
+	int count, ret = 0;
 	uint32_t __user *fb_id;
 	uint32_t __user *crtc_id;
 	uint32_t __user *connector_id;
@@ -105,89 +99,70 @@  int drm_mode_getresources(struct drm_device *dev, void *data,
 
 
 	mutex_lock(&file_priv->fbs_lock);
-	/*
-	 * For the non-control nodes we need to limit the list of resources
-	 * by IDs in the group list for this node
-	 */
-	list_for_each(lh, &file_priv->fbs)
-		fb_count++;
-
-	/* handle this in 4 parts */
-	/* FBs */
-	if (card_res->count_fbs >= fb_count) {
-		copied = 0;
-		fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;
-		list_for_each_entry(fb, &file_priv->fbs, filp_head) {
-			if (put_user(fb->base.id, fb_id + copied)) {
-				mutex_unlock(&file_priv->fbs_lock);
-				return -EFAULT;
-			}
-			copied++;
+	count = 0;
+	fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;
+	list_for_each_entry(fb, &file_priv->fbs, filp_head) {
+		count++;
+		if (count > card_res->count_fbs)
+			continue;
+
+		if (put_user(fb->base.id, fb_id + count)) {
+			mutex_unlock(&file_priv->fbs_lock);
+			return -EFAULT;
 		}
 	}
-	card_res->count_fbs = fb_count;
+	card_res->count_fbs = count;
 	mutex_unlock(&file_priv->fbs_lock);
 
 	/* mode_config.mutex protects the connector list against e.g. DP MST
 	 * connector hot-adding. CRTC/Plane lists are invariant. */
 	mutex_lock(&dev->mode_config.mutex);
-	drm_for_each_crtc(crtc, dev)
-		crtc_count++;
-
-	drm_for_each_connector(connector, dev)
-		connector_count++;
-
-	drm_for_each_encoder(encoder, dev)
-		encoder_count++;
-
 	card_res->max_height = dev->mode_config.max_height;
 	card_res->min_height = dev->mode_config.min_height;
 	card_res->max_width = dev->mode_config.max_width;
 	card_res->min_width = dev->mode_config.min_width;
 
-	/* CRTCs */
-	if (card_res->count_crtcs >= crtc_count) {
-		copied = 0;
-		crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr;
-		drm_for_each_crtc(crtc, dev) {
-			if (put_user(crtc->base.id, crtc_id + copied)) {
-				ret = -EFAULT;
-				goto out;
-			}
-			copied++;
+	count = 0;
+	crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr;
+	drm_for_each_crtc(crtc, dev) {
+		count++;
+		if (count > card_res->count_crtcs)
+			continue;
+
+		if (put_user(crtc->base.id, crtc_id + count)) {
+			ret = -EFAULT;
+			goto out;
 		}
 	}
-	card_res->count_crtcs = crtc_count;
-
-	/* Encoders */
-	if (card_res->count_encoders >= encoder_count) {
-		copied = 0;
-		encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr;
-		drm_for_each_encoder(encoder, dev) {
-			if (put_user(encoder->base.id, encoder_id +
-				     copied)) {
-				ret = -EFAULT;
-				goto out;
-			}
-			copied++;
+	card_res->count_crtcs = count;
+
+	count = 0;
+	encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr;
+	drm_for_each_encoder(encoder, dev) {
+		count++;
+		if (count > card_res->count_encoders)
+			continue;
+
+		if (put_user(encoder->base.id, encoder_id + count)) {
+			ret = -EFAULT;
+			goto out;
 		}
 	}
-	card_res->count_encoders = encoder_count;
-
-	/* Connectors */
-	if (card_res->count_connectors >= connector_count) {
-		copied = 0;
-		connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr;
-		drm_for_each_connector(connector, dev) {
-			if (put_user(connector->base.id,
-				     connector_id + copied)) {
-				ret = -EFAULT;
-				goto out;
-			}
-			copied++;
+	card_res->count_encoders = count;
+
+	count = 0;
+	connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr;
+	drm_for_each_connector(connector, dev) {
+		count++;
+		if (count >= card_res->count_connectors)
+			continue;
+
+		if (put_user(connector->base.id, connector_id + count)) {
+			ret = -EFAULT;
+			goto out;
 		}
 	}
-	card_res->count_connectors = connector_count;
+	card_res->count_connectors = count;
 
 out:
 	mutex_unlock(&dev->mode_config.mutex);