Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Satendra Singh Thakur <satendra.t@samsung.com>
To:     Gustavo Padovan <gustavo@padovan.org>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Sean Paul <seanpaul@chromium.org>,
        David Airlie <airlied@linux.ie>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Cc:     vineet.j@samsung.com, hemanshu.s@samsung.com, sst2005@gmail.com,
        Satendra Singh Thakur <satendra.t@samsung.com>
Subject: [PATCH v1] drm/kms/crtc: Improving the func drm_mode_setcrtc
Date:   Fri,  3 Aug 2018 17:13:34 +0530
In-Reply-To: <66ead941-3c64-45e4-7835-949360e2b01f@linux.intel.com>
Message-Id: <20180803114404epcas5p1c5aaec7ca27e520b5cba52b0d7f09124~HXSyqLTJ81698916989epcas5p1G@epcas5p1.samsung.com>
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 105P
DLP-Filter: Pass
References: <66ead941-3c64-45e4-7835-949360e2b01f@linux.intel.com>
        <CGME20180803114404epcas5p1c5aaec7ca27e520b5cba52b0d7f09124@epcas5p1.samsung.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Following changes are done to this func:
1. The declaration of plane and it's assignment plane = crtc->primary
are only used when mode_valid is set. Therefore, moved it inside
the if(mode_valid) statement.

2. The declaration of connector and set_connectors_ptr and out_id
are moved inside the for loop, as their scope is limited within
that block.

3. Currently, there are 3 checks on count_connectors
and 4 checks on mode related params (mode_valid, mode, fb).
if (crtc_req->mode_valid) {
if (crtc_req->count_connectors == 0 && mode) {
if (crtc_req->count_connectors > 0 && (!mode || !fb)) {
if (crtc_req->count_connectors > 0) {

In the modified code, there are just 1 check on mode_valid and
2 checks  count_connectors.
Checks on mode and fb are not needed as these variables will
be non-NULL by the end of if(mode_valid) statement if mode_valid is set.
If mode_valid is clear, mode and fb will be NULL.
Therefore, we just check mode_valid and NOT mode or fb.

4. Moved kfree inside if statement

Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com>

---
 
 v1: Hi Mr Maarten, Thanks for the comments.
     I have fixed some of them and done more modifications to the patch.
     Please review.

 drivers/gpu/drm/drm_crtc.c | 57 ++++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 98a36e6..9842985 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -559,12 +559,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_mode_crtc *crtc_req = data;
 	struct drm_crtc *crtc;
-	struct drm_plane *plane;
-	struct drm_connector **connector_set = NULL, *connector;
+	struct drm_connector **connector_set = NULL;
 	struct drm_framebuffer *fb = NULL;
 	struct drm_display_mode *mode = NULL;
 	struct drm_mode_set set;
-	uint32_t __user *set_connectors_ptr;
 	struct drm_modeset_acquire_ctx ctx;
 	int ret;
 	int i;
@@ -586,7 +584,6 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 	}
 	DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
 
-	plane = crtc->primary;
 
 	mutex_lock(&crtc->dev->mode_config.mutex);
 	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
@@ -596,6 +593,8 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 		goto out;
 
 	if (crtc_req->mode_valid) {
+		struct drm_plane *plane = crtc->primary;
+		/* Handle framebuffer and mode here*/
 		/* If we have a mode we need a framebuffer. */
 		/* If we pass -1, set the mode with the currently bound fb */
 		if (crtc_req->fb_id == -1) {
@@ -636,8 +635,6 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 			ret = -EINVAL;
 			goto out;
 		}
-
-
 		ret = drm_mode_convert_umode(dev, mode, &crtc_req->mode);
 		if (ret) {
 			DRM_DEBUG_KMS("Invalid mode\n");
@@ -669,31 +666,22 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 					      mode, fb);
 		if (ret)
 			goto out;
-
-	}
-
-	if (crtc_req->count_connectors == 0 && mode) {
-		DRM_DEBUG_KMS("Count connectors is 0 but mode set\n");
-		ret = -EINVAL;
-		goto out;
-	}
-
-	if (crtc_req->count_connectors > 0 && (!mode || !fb)) {
-		DRM_DEBUG_KMS("Count connectors is %d but no mode or fb set\n",
-			  crtc_req->count_connectors);
-		ret = -EINVAL;
-		goto out;
-	}
-
-	if (crtc_req->count_connectors > 0) {
-		u32 out_id;
-
+		/* Handle connector here
+		 * crtc_req->mode_valid is set at this point
+		 * and we have mode and fb non-NULL.
+		 * We have already checked mode_valid
+		 * hence, we don't check mode and fb here.
+		 */
+		if (!crtc_req->count_connectors) {
+			DRM_DEBUG_KMS("Mode_valid flag is set but connectors' count is 0\n");
+			ret = -EINVAL;
+			goto out;
+		}
 		/* Avoid unbounded kernel memory allocation */
 		if (crtc_req->count_connectors > config->num_connector) {
 			ret = -EINVAL;
 			goto out;
 		}
-
 		connector_set = kmalloc_array(crtc_req->count_connectors,
 					      sizeof(struct drm_connector *),
 					      GFP_KERNEL);
@@ -703,6 +691,9 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 		}
 
 		for (i = 0; i < crtc_req->count_connectors; i++) {
+			struct drm_connector *connector;
+			uint32_t __user *set_connectors_ptr;
+			u32 out_id;
 			connector_set[i] = NULL;
 			set_connectors_ptr = (uint32_t __user *)(unsigned long)crtc_req->set_connectors_ptr;
 			if (get_user(out_id, &set_connectors_ptr[i])) {
@@ -723,6 +714,18 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 
 			connector_set[i] = connector;
 		}
+
+	} else {
+		/* crtc_req->mode_valid is clear at this point
+		 * if mode_valid is clear, mode and fb will be NULL
+		 * hence, we don't check mode and fb here.
+		 */
+		if (crtc_req->count_connectors) {
+			DRM_DEBUG_KMS("Connectors's count is %u but mode_valid flag is clear\n",
+			crtc_req->count_connectors);
+			ret = -EINVAL;
+			goto out;
+		}
 	}
 
 	set.crtc = crtc;
@@ -743,8 +746,8 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 			if (connector_set[i])
 				drm_connector_put(connector_set[i]);
 		}
+		kfree(connector_set);
 	}
-	kfree(connector_set);
 	drm_mode_destroy(dev, mode);
 	if (ret == -EDEADLK) {
 		ret = drm_modeset_backoff(&ctx);
-- 
2.7.4