[PATCH 1/5] drm/i915: use pointer = k[cmz...]alloc(sizeof(*pointer), ...) pattern

[PATCH 1/5] drm/i915: use pointer = k[cmz...]alloc(sizeof(*pointer), ...) pattern

[Daniel Vetter]

Done while reviewing all our allocations for fubar. Also a few errant
cases of lacking () for the sizeof operator - just a bit of OCD.

I've left out all the conversions that also should use kcalloc from
this patch  (it's only 2).

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
 drivers/gpu/drm/i915/i915_dma.c      |  6 +++---
 drivers/gpu/drm/i915/i915_gem.c      |  2 +-
 drivers/gpu/drm/i915/intel_crt.c     |  2 +-
 drivers/gpu/drm/i915/intel_ddi.c     |  6 +++---
 drivers/gpu/drm/i915/intel_display.c |  2 +-
 drivers/gpu/drm/i915/intel_dp.c      |  4 ++--
 drivers/gpu/drm/i915/intel_dvo.c     |  4 ++--
 drivers/gpu/drm/i915/intel_fb.c      |  2 +-
 drivers/gpu/drm/i915/intel_hdmi.c    |  4 ++--
 drivers/gpu/drm/i915/intel_lvds.c    |  4 ++--
 drivers/gpu/drm/i915/intel_overlay.c |  4 ++--
 drivers/gpu/drm/i915/intel_pm.c      |  2 +-
 drivers/gpu/drm/i915/intel_sdvo.c    | 10 +++++-----
 drivers/gpu/drm/i915/intel_sprite.c  |  2 +-
 drivers/gpu/drm/i915/intel_tv.c      |  4 ++--
 16 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 1d77624..821406c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2145,7 +2145,7 @@ drm_add_fake_info_node(struct drm_minor *minor,
 {
 	struct drm_info_node *node;
 
-	node = kmalloc(sizeof(struct drm_info_node), GFP_KERNEL);
+	node = kmalloc(sizeof(*node), GFP_KERNEL);
 	if (node == NULL) {
 		debugfs_remove(ent);
 		return -ENOMEM;
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index f4f9895..f7b3d0f 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -641,7 +641,7 @@ static int i915_batchbuffer(struct drm_device *dev, void *data,
 
 	if (batch->num_cliprects) {
 		cliprects = kcalloc(batch->num_cliprects,
-				    sizeof(struct drm_clip_rect),
+				    sizeof(*cliprects),
 				    GFP_KERNEL);
 		if (cliprects == NULL)
 			return -ENOMEM;
@@ -703,7 +703,7 @@ static int i915_cmdbuffer(struct drm_device *dev, void *data,
 
 	if (cmdbuf->num_cliprects) {
 		cliprects = kcalloc(cmdbuf->num_cliprects,
-				    sizeof(struct drm_clip_rect), GFP_KERNEL);
+				    sizeof(*cliprects), GFP_KERNEL);
 		if (cliprects == NULL) {
 			ret = -ENOMEM;
 			goto fail_batch_free;
@@ -1472,7 +1472,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	dev->types[8] = _DRM_STAT_SECONDARY;
 	dev->types[9] = _DRM_STAT_DMA;
 
-	dev_priv = kzalloc(sizeof(drm_i915_private_t), GFP_KERNEL);
+	dev_priv = kzalloc(sizeof(*dev_priv), GFP_KERNEL);
 	if (dev_priv == NULL)
 		return -ENOMEM;
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d00d24f..c6d0353 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4599,7 +4599,7 @@ static int i915_gem_init_phys_object(struct drm_device *dev,
 	if (dev_priv->mm.phys_objs[id - 1] || !size)
 		return 0;
 
-	phys_obj = kzalloc(sizeof(struct drm_i915_gem_phys_object), GFP_KERNEL);
+	phys_obj = kzalloc(sizeof(*phys_obj), GFP_KERNEL);
 	if (!phys_obj)
 		return -ENOMEM;
 
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 6f101d5..f9a5f3d 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -764,7 +764,7 @@ void intel_crt_init(struct drm_device *dev)
 	if (!crt)
 		return;
 
-	intel_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL);
+	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
 	if (!intel_connector) {
 		kfree(crt);
 		return;
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 4918995..46bc43c 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1337,11 +1337,11 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
 	struct intel_connector *hdmi_connector = NULL;
 	struct intel_connector *dp_connector = NULL;
 
-	intel_dig_port = kzalloc(sizeof(struct intel_digital_port), GFP_KERNEL);
+	intel_dig_port = kzalloc(sizeof(*intel_dig_port), GFP_KERNEL);
 	if (!intel_dig_port)
 		return;
 
-	dp_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL);
+	dp_connector = kzalloc(sizeof(*dp_connector), GFP_KERNEL);
 	if (!dp_connector) {
 		kfree(intel_dig_port);
 		return;
@@ -1381,7 +1381,7 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
 	}
 
 	if (intel_encoder->type != INTEL_OUTPUT_EDP) {
-		hdmi_connector = kzalloc(sizeof(struct intel_connector),
+		hdmi_connector = kzalloc(sizeof(*hdmi_connector),
 					 GFP_KERNEL);
 		if (!hdmi_connector) {
 			return;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4dd6561..fe8db37 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8092,7 +8092,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	     fb->pitches[0] != crtc->fb->pitches[0]))
 		return -EINVAL;
 
-	work = kzalloc(sizeof *work, GFP_KERNEL);
+	work = kzalloc(sizeof(*work), GFP_KERNEL);
 	if (work == NULL)
 		return -ENOMEM;
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9770160..d840bc8 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3614,11 +3614,11 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
 	struct drm_encoder *encoder;
 	struct intel_connector *intel_connector;
 
-	intel_dig_port = kzalloc(sizeof(struct intel_digital_port), GFP_KERNEL);
+	intel_dig_port = kzalloc(sizeof(*intel_dig_port), GFP_KERNEL);
 	if (!intel_dig_port)
 		return;
 
-	intel_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL);
+	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
 	if (!intel_connector) {
 		kfree(intel_dig_port);
 		return;
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index fe65c72..3ff9e2c 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -446,11 +446,11 @@ void intel_dvo_init(struct drm_device *dev)
 	int i;
 	int encoder_type = DRM_MODE_ENCODER_NONE;
 
-	intel_dvo = kzalloc(sizeof(struct intel_dvo), GFP_KERNEL);
+	intel_dvo = kzalloc(sizeof(*intel_dvo), GFP_KERNEL);
 	if (!intel_dvo)
 		return;
 
-	intel_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL);
+	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
 	if (!intel_connector) {
 		kfree(intel_dvo);
 		return;
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index bc21000..6aa66aa 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -216,7 +216,7 @@ int intel_fbdev_init(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
+	ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
 	if (!ifbdev)
 		return -ENOMEM;
 
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 79582f9..a6310ca 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1292,11 +1292,11 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
 	struct intel_encoder *intel_encoder;
 	struct intel_connector *intel_connector;
 
-	intel_dig_port = kzalloc(sizeof(struct intel_digital_port), GFP_KERNEL);
+	intel_dig_port = kzalloc(sizeof(*intel_dig_port), GFP_KERNEL);
 	if (!intel_dig_port)
 		return;
 
-	intel_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL);
+	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
 	if (!intel_connector) {
 		kfree(intel_dig_port);
 		return;
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 05e5485..639650c 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -948,11 +948,11 @@ void intel_lvds_init(struct drm_device *dev)
 		}
 	}
 
-	lvds_encoder = kzalloc(sizeof(struct intel_lvds_encoder), GFP_KERNEL);
+	lvds_encoder = kzalloc(sizeof(*lvds_encoder), GFP_KERNEL);
 	if (!lvds_encoder)
 		return;
 
-	lvds_connector = kzalloc(sizeof(struct intel_lvds_connector), GFP_KERNEL);
+	lvds_connector = kzalloc(sizeof(*lvds_connector), GFP_KERNEL);
 	if (!lvds_connector) {
 		kfree(lvds_encoder);
 		return;
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 8d6d0a1..a98a990 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -1053,7 +1053,7 @@ int intel_overlay_put_image(struct drm_device *dev, void *data,
 		return ret;
 	}
 
-	params = kmalloc(sizeof(struct put_image_params), GFP_KERNEL);
+	params = kmalloc(sizeof(*params), GFP_KERNEL);
 	if (!params)
 		return -ENOMEM;
 
@@ -1320,7 +1320,7 @@ void intel_setup_overlay(struct drm_device *dev)
 	if (!HAS_OVERLAY(dev))
 		return;
 
-	overlay = kzalloc(sizeof(struct intel_overlay), GFP_KERNEL);
+	overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
 	if (!overlay)
 		return;
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fe19ba3..6fd2e05 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -370,7 +370,7 @@ static void intel_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 
 	intel_cancel_fbc_work(dev_priv);
 
-	work = kzalloc(sizeof *work, GFP_KERNEL);
+	work = kzalloc(sizeof(*work), GFP_KERNEL);
 	if (work == NULL) {
 		DRM_ERROR("Failed to allocate FBC work structure\n");
 		dev_priv->display.enable_fbc(crtc, interval);
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 8aa7be5..989cf74 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2388,7 +2388,7 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
 	struct intel_connector *intel_connector;
 	struct intel_sdvo_connector *intel_sdvo_connector;
 
-	intel_sdvo_connector = kzalloc(sizeof(struct intel_sdvo_connector), GFP_KERNEL);
+	intel_sdvo_connector = kzalloc(sizeof(*intel_sdvo_connector), GFP_KERNEL);
 	if (!intel_sdvo_connector)
 		return false;
 
@@ -2436,7 +2436,7 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
 	struct intel_connector *intel_connector;
 	struct intel_sdvo_connector *intel_sdvo_connector;
 
-	intel_sdvo_connector = kzalloc(sizeof(struct intel_sdvo_connector), GFP_KERNEL);
+	intel_sdvo_connector = kzalloc(sizeof(*intel_sdvo_connector), GFP_KERNEL);
 	if (!intel_sdvo_connector)
 		return false;
 
@@ -2473,7 +2473,7 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
 	struct intel_connector *intel_connector;
 	struct intel_sdvo_connector *intel_sdvo_connector;
 
-	intel_sdvo_connector = kzalloc(sizeof(struct intel_sdvo_connector), GFP_KERNEL);
+	intel_sdvo_connector = kzalloc(sizeof(*intel_sdvo_connector), GFP_KERNEL);
 	if (!intel_sdvo_connector)
 		return false;
 
@@ -2504,7 +2504,7 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
 	struct intel_connector *intel_connector;
 	struct intel_sdvo_connector *intel_sdvo_connector;
 
-	intel_sdvo_connector = kzalloc(sizeof(struct intel_sdvo_connector), GFP_KERNEL);
+	intel_sdvo_connector = kzalloc(sizeof(*intel_sdvo_connector), GFP_KERNEL);
 	if (!intel_sdvo_connector)
 		return false;
 
@@ -2870,7 +2870,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 	struct intel_encoder *intel_encoder;
 	struct intel_sdvo *intel_sdvo;
 	int i;
-	intel_sdvo = kzalloc(sizeof(struct intel_sdvo), GFP_KERNEL);
+	intel_sdvo = kzalloc(sizeof(*intel_sdvo), GFP_KERNEL);
 	if (!intel_sdvo)
 		return false;
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 231b289..cae10bc 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1034,7 +1034,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 	if (INTEL_INFO(dev)->gen < 5)
 		return -ENODEV;
 
-	intel_plane = kzalloc(sizeof(struct intel_plane), GFP_KERNEL);
+	intel_plane = kzalloc(sizeof(*intel_plane), GFP_KERNEL);
 	if (!intel_plane)
 		return -ENOMEM;
 
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index f2c6d79..adc7801 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1582,12 +1582,12 @@ intel_tv_init(struct drm_device *dev)
 	    (tv_dac_off & TVDAC_STATE_CHG_EN) != 0)
 		return;
 
-	intel_tv = kzalloc(sizeof(struct intel_tv), GFP_KERNEL);
+	intel_tv = kzalloc(sizeof(*intel_tv), GFP_KERNEL);
 	if (!intel_tv) {
 		return;
 	}
 
-	intel_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL);
+	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
 	if (!intel_connector) {
 		kfree(intel_tv);
 		return;
-- 
1.8.4.rc3

[PATCH 2/5] drm/i915: Use kcalloc more

[Daniel Vetter]

No buffer overflows here, but better safe than sorry.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 5 +++--
 drivers/gpu/drm/i915/i915_gem_gtt.c        | 8 ++++----
 drivers/gpu/drm/i915/i915_gem_tiling.c     | 6 +++---
 drivers/gpu/drm/i915/i915_gpu_error.c      | 4 ++--
 drivers/gpu/drm/i915/intel_display.c       | 2 +-
 5 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index ee93357..a733118 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1047,7 +1047,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			return -EINVAL;
 		}
 
-		cliprects = kmalloc(args->num_cliprects * sizeof(*cliprects),
+		cliprects = kcalloc(args->num_cliprects,
+				    sizeof(*cliprects),
 				    GFP_KERNEL);
 		if (cliprects == NULL) {
 			ret = -ENOMEM;
@@ -1302,7 +1303,7 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	exec2_list = kmalloc(sizeof(*exec2_list)*args->buffer_count,
+	exec2_list = kcalloc(args->buffer_count, sizeof(*exec2_list),
 			     GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
 	if (exec2_list == NULL)
 		exec2_list = drm_malloc_ab(sizeof(*exec2_list),
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 212f6d8..dafbdb7 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -336,8 +336,8 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 	ppgtt->base.insert_entries = gen6_ppgtt_insert_entries;
 	ppgtt->base.cleanup = gen6_ppgtt_cleanup;
 	ppgtt->base.scratch = dev_priv->gtt.base.scratch;
-	ppgtt->pt_pages = kzalloc(sizeof(struct page *)*ppgtt->num_pd_entries,
-				  GFP_KERNEL);
+	ppgtt->pt_pages = kcalloc(ppgtt->num_pd_entries, sizeof(struct page *),
+				  GFP_KERNEL | __GFP_ZERO);
 	if (!ppgtt->pt_pages)
 		return -ENOMEM;
 
@@ -347,8 +347,8 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 			goto err_pt_alloc;
 	}
 
-	ppgtt->pt_dma_addr = kzalloc(sizeof(dma_addr_t) *ppgtt->num_pd_entries,
-				     GFP_KERNEL);
+	ppgtt->pt_dma_addr = kcalloc(ppgtt->num_pd_entries, sizeof(dma_addr_t),
+				     GFP_KERNEL | __GFP_ZERO);
 	if (!ppgtt->pt_dma_addr)
 		goto err_pt_alloc;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 032e9ef..ac9ebe9 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -393,7 +393,7 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
 	/* Try to preallocate memory required to save swizzling on put-pages */
 	if (i915_gem_object_needs_bit17_swizzle(obj)) {
 		if (obj->bit_17 == NULL) {
-			obj->bit_17 = kmalloc(BITS_TO_LONGS(obj->base.size >> PAGE_SHIFT) *
+			obj->bit_17 = kcalloc(BITS_TO_LONGS(obj->base.size >> PAGE_SHIFT),
 					      sizeof(long), GFP_KERNEL);
 		}
 	} else {
@@ -504,8 +504,8 @@ i915_gem_object_save_bit_17_swizzle(struct drm_i915_gem_object *obj)
 	int i;
 
 	if (obj->bit_17 == NULL) {
-		obj->bit_17 = kmalloc(BITS_TO_LONGS(page_count) *
-					   sizeof(long), GFP_KERNEL);
+		obj->bit_17 = kcalloc(BITS_TO_LONGS(page_count),
+				      sizeof(long), GFP_KERNEL);
 		if (obj->bit_17 == NULL) {
 			DRM_ERROR("Failed to allocate memory for bit 17 "
 				  "record\n");
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index c38d575..763283e 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -791,7 +791,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
 
 		error->ring[i].num_requests = count;
 		error->ring[i].requests =
-			kmalloc(count*sizeof(struct drm_i915_error_request),
+			kcalloc(count, sizeof(error->ring[i].requests),
 				GFP_ATOMIC);
 		if (error->ring[i].requests == NULL) {
 			error->ring[i].num_requests = 0;
@@ -833,7 +833,7 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
 	error->pinned_bo_count[ndx] = i - error->active_bo_count[ndx];
 
 	if (i) {
-		active_bo = kmalloc(sizeof(*active_bo)*i, GFP_ATOMIC);
+		active_bo = kcalloc(i, sizeof(*active_bo), GFP_ATOMIC);
 		if (active_bo)
 			pinned_bo = active_bo + error->active_bo_count[ndx];
 	}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fe8db37..6b8a107 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9031,7 +9031,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	unsigned disable_pipes, prepare_pipes, modeset_pipes;
 	int ret = 0;
 
-	saved_mode = kmalloc(2 * sizeof(*saved_mode), GFP_KERNEL);
+	saved_mode = kcalloc(2, sizeof(*saved_mode), GFP_KERNEL);
 	if (!saved_mode)
 		return -ENOMEM;
 	saved_hwmode = saved_mode + 1;
-- 
1.8.4.rc3

Re: [PATCH 2/5] drm/i915: Use kcalloc more

[Jani Nikula]

On Thu, 19 Sep 2013, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> No buffer overflows here, but better safe than sorry.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 5 +++--
>  drivers/gpu/drm/i915/i915_gem_gtt.c        | 8 ++++----
>  drivers/gpu/drm/i915/i915_gem_tiling.c     | 6 +++---
>  drivers/gpu/drm/i915/i915_gpu_error.c      | 4 ++--
>  drivers/gpu/drm/i915/intel_display.c       | 2 +-
>  5 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index ee93357..a733118 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1047,7 +1047,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  			return -EINVAL;
>  		}
>  
> -		cliprects = kmalloc(args->num_cliprects * sizeof(*cliprects),
> +		cliprects = kcalloc(args->num_cliprects,
> +				    sizeof(*cliprects),
>  				    GFP_KERNEL);
>  		if (cliprects == NULL) {
>  			ret = -ENOMEM;
> @@ -1302,7 +1303,7 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
>  		return -EINVAL;
>  	}
>  
> -	exec2_list = kmalloc(sizeof(*exec2_list)*args->buffer_count,
> +	exec2_list = kcalloc(args->buffer_count, sizeof(*exec2_list),
>  			     GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
>  	if (exec2_list == NULL)
>  		exec2_list = drm_malloc_ab(sizeof(*exec2_list),
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 212f6d8..dafbdb7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -336,8 +336,8 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  	ppgtt->base.insert_entries = gen6_ppgtt_insert_entries;
>  	ppgtt->base.cleanup = gen6_ppgtt_cleanup;
>  	ppgtt->base.scratch = dev_priv->gtt.base.scratch;
> -	ppgtt->pt_pages = kzalloc(sizeof(struct page *)*ppgtt->num_pd_entries,
> -				  GFP_KERNEL);
> +	ppgtt->pt_pages = kcalloc(ppgtt->num_pd_entries, sizeof(struct page *),
> +				  GFP_KERNEL | __GFP_ZERO);

kcalloc implies __GFP_ZERO, specifying it is redundant. Ditto
below. This also means this patch does a bunch of zeroing that's
strictly not necessary.

>  	if (!ppgtt->pt_pages)
>  		return -ENOMEM;
>  
> @@ -347,8 +347,8 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  			goto err_pt_alloc;
>  	}
>  
> -	ppgtt->pt_dma_addr = kzalloc(sizeof(dma_addr_t) *ppgtt->num_pd_entries,
> -				     GFP_KERNEL);
> +	ppgtt->pt_dma_addr = kcalloc(ppgtt->num_pd_entries, sizeof(dma_addr_t),
> +				     GFP_KERNEL | __GFP_ZERO);
>  	if (!ppgtt->pt_dma_addr)
>  		goto err_pt_alloc;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> index 032e9ef..ac9ebe9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -393,7 +393,7 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
>  	/* Try to preallocate memory required to save swizzling on put-pages */
>  	if (i915_gem_object_needs_bit17_swizzle(obj)) {
>  		if (obj->bit_17 == NULL) {
> -			obj->bit_17 = kmalloc(BITS_TO_LONGS(obj->base.size >> PAGE_SHIFT) *
> +			obj->bit_17 = kcalloc(BITS_TO_LONGS(obj->base.size >> PAGE_SHIFT),
>  					      sizeof(long), GFP_KERNEL);
>  		}
>  	} else {
> @@ -504,8 +504,8 @@ i915_gem_object_save_bit_17_swizzle(struct drm_i915_gem_object *obj)
>  	int i;
>  
>  	if (obj->bit_17 == NULL) {
> -		obj->bit_17 = kmalloc(BITS_TO_LONGS(page_count) *
> -					   sizeof(long), GFP_KERNEL);
> +		obj->bit_17 = kcalloc(BITS_TO_LONGS(page_count),
> +				      sizeof(long), GFP_KERNEL);
>  		if (obj->bit_17 == NULL) {
>  			DRM_ERROR("Failed to allocate memory for bit 17 "
>  				  "record\n");
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index c38d575..763283e 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -791,7 +791,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
>  
>  		error->ring[i].num_requests = count;
>  		error->ring[i].requests =
> -			kmalloc(count*sizeof(struct drm_i915_error_request),
> +			kcalloc(count, sizeof(error->ring[i].requests),

Crash boom bang.


BR,
Jani.

>  				GFP_ATOMIC);
>  		if (error->ring[i].requests == NULL) {
>  			error->ring[i].num_requests = 0;
> @@ -833,7 +833,7 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
>  	error->pinned_bo_count[ndx] = i - error->active_bo_count[ndx];
>  
>  	if (i) {
> -		active_bo = kmalloc(sizeof(*active_bo)*i, GFP_ATOMIC);
> +		active_bo = kcalloc(i, sizeof(*active_bo), GFP_ATOMIC);
>  		if (active_bo)
>  			pinned_bo = active_bo + error->active_bo_count[ndx];
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fe8db37..6b8a107 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9031,7 +9031,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  	unsigned disable_pipes, prepare_pipes, modeset_pipes;
>  	int ret = 0;
>  
> -	saved_mode = kmalloc(2 * sizeof(*saved_mode), GFP_KERNEL);
> +	saved_mode = kcalloc(2, sizeof(*saved_mode), GFP_KERNEL);
>  	if (!saved_mode)
>  		return -ENOMEM;
>  	saved_hwmode = saved_mode + 1;
> -- 
> 1.8.4.rc3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/5] drm/i915: Use kcalloc more

[Chris Wilson]

On Thu, Sep 19, 2013 at 01:38:18PM +0300, Jani Nikula wrote:
> On Thu, 19 Sep 2013, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index c38d575..763283e 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -791,7 +791,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
> >  
> >  		error->ring[i].num_requests = count;
> >  		error->ring[i].requests =
> > -			kmalloc(count*sizeof(struct drm_i915_error_request),
> > +			kcalloc(count, sizeof(error->ring[i].requests),
> 
> Crash boom bang.

Not quite. This is evaluated at compile time by parsing the type rather
than by pointer dereference.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/5] drm/i915: Use kcalloc more

[Jani Nikula]

On Thu, 19 Sep 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Sep 19, 2013 at 01:38:18PM +0300, Jani Nikula wrote:
>> On Thu, 19 Sep 2013, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
>> > index c38d575..763283e 100644
>> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>> > @@ -791,7 +791,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
>> >  
>> >  		error->ring[i].num_requests = count;
>> >  		error->ring[i].requests =
>> > -			kmalloc(count*sizeof(struct drm_i915_error_request),
>> > +			kcalloc(count, sizeof(error->ring[i].requests),
>> 
>> Crash boom bang.
>
> Not quite. This is evaluated at compile time by parsing the type rather
> than by pointer dereference.

Sizeof changes from sizeof(struct drm_i915_error_request) to
sizeof(struct drm_i915_error_request *). It'll break something. Maybe
not as spectacularly as I was implying.

Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/5] drm/i915: Use kcalloc more

[Chris Wilson]

On Thu, Sep 19, 2013 at 02:00:30PM +0300, Jani Nikula wrote:
> On Thu, 19 Sep 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Thu, Sep 19, 2013 at 01:38:18PM +0300, Jani Nikula wrote:
> >> On Thu, 19 Sep 2013, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> >> > index c38d575..763283e 100644
> >> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> >> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> >> > @@ -791,7 +791,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
> >> >  
> >> >  		error->ring[i].num_requests = count;
> >> >  		error->ring[i].requests =
> >> > -			kmalloc(count*sizeof(struct drm_i915_error_request),
> >> > +			kcalloc(count, sizeof(error->ring[i].requests),
> >> 
> >> Crash boom bang.
> >
> > Not quite. This is evaluated at compile time by parsing the type rather
> > than by pointer dereference.
> 
> Sizeof changes from sizeof(struct drm_i915_error_request) to
> sizeof(struct drm_i915_error_request *). It'll break something. Maybe
> not as spectacularly as I was implying.

Apologies, I automatically go into CLANG is a not a valid C compiler
mode...

sizeof(*error->ring[i].requests)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/5] drm/i915: Use kcalloc more

[Chris Wilson]

On Thu, Sep 19, 2013 at 12:18:33PM +0200, Daniel Vetter wrote:
> No buffer overflows here, but better safe than sorry.

You are also introducing needless memsets.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/5] drm/i915: Use kcalloc more

[Daniel Vetter]

On Thu, Sep 19, 2013 at 11:46:55AM +0100, Chris Wilson wrote:
> On Thu, Sep 19, 2013 at 12:18:33PM +0200, Daniel Vetter wrote:
> > No buffer overflows here, but better safe than sorry.
> 
> You are also introducing needless memsets.

Meh, somehow I've thought kcalloc won't zero. I'll drop the redundant
GFP_ZEROs that Jani spotted and switch kcalloc to kmalloc_array in the
place we care about speed and want to avoid the memset.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: Use kcalloc more

[Daniel Vetter]

No buffer overflows here, but better safe than sorry.

v2:
- Fixup the sizeof conversion, I've missed the pointer deref (Jani).
- Drop the redundant GFP_ZERO, kcalloc alreads memsets (Jani).
- Use kmalloc_array for the execbuf fastpath to avoid the memset
  (Chris). I've opted to leave all other conversions as-is since they
  aren't in a fastpath and dealing with cleared memory instead of
  random garbage is just generally nicer.

Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 8 +++++---
 drivers/gpu/drm/i915/i915_gem_gtt.c        | 4 ++--
 drivers/gpu/drm/i915/i915_gem_tiling.c     | 6 +++---
 drivers/gpu/drm/i915/i915_gpu_error.c      | 4 ++--
 drivers/gpu/drm/i915/intel_display.c       | 2 +-
 5 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index ee93357..ccfb8e6 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1047,7 +1047,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			return -EINVAL;
 		}
 
-		cliprects = kmalloc(args->num_cliprects * sizeof(*cliprects),
+		cliprects = kcalloc(args->num_cliprects,
+				    sizeof(*cliprects),
 				    GFP_KERNEL);
 		if (cliprects == NULL) {
 			ret = -ENOMEM;
@@ -1302,8 +1303,9 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	exec2_list = kmalloc(sizeof(*exec2_list)*args->buffer_count,
-			     GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
+	exec2_list = kmalloc_array(args->buffer_count, sizeof(*exec2_list),
+				   GFP_TEMPORARY |
+				   __GFP_NOWARN | __GFP_NORETRY);
 	if (exec2_list == NULL)
 		exec2_list = drm_malloc_ab(sizeof(*exec2_list),
 					   args->buffer_count);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 212f6d8..e999496 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -336,7 +336,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 	ppgtt->base.insert_entries = gen6_ppgtt_insert_entries;
 	ppgtt->base.cleanup = gen6_ppgtt_cleanup;
 	ppgtt->base.scratch = dev_priv->gtt.base.scratch;
-	ppgtt->pt_pages = kzalloc(sizeof(struct page *)*ppgtt->num_pd_entries,
+	ppgtt->pt_pages = kcalloc(ppgtt->num_pd_entries, sizeof(struct page *),
 				  GFP_KERNEL);
 	if (!ppgtt->pt_pages)
 		return -ENOMEM;
@@ -347,7 +347,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 			goto err_pt_alloc;
 	}
 
-	ppgtt->pt_dma_addr = kzalloc(sizeof(dma_addr_t) *ppgtt->num_pd_entries,
+	ppgtt->pt_dma_addr = kcalloc(ppgtt->num_pd_entries, sizeof(dma_addr_t),
 				     GFP_KERNEL);
 	if (!ppgtt->pt_dma_addr)
 		goto err_pt_alloc;
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 032e9ef..ac9ebe9 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -393,7 +393,7 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
 	/* Try to preallocate memory required to save swizzling on put-pages */
 	if (i915_gem_object_needs_bit17_swizzle(obj)) {
 		if (obj->bit_17 == NULL) {
-			obj->bit_17 = kmalloc(BITS_TO_LONGS(obj->base.size >> PAGE_SHIFT) *
+			obj->bit_17 = kcalloc(BITS_TO_LONGS(obj->base.size >> PAGE_SHIFT),
 					      sizeof(long), GFP_KERNEL);
 		}
 	} else {
@@ -504,8 +504,8 @@ i915_gem_object_save_bit_17_swizzle(struct drm_i915_gem_object *obj)
 	int i;
 
 	if (obj->bit_17 == NULL) {
-		obj->bit_17 = kmalloc(BITS_TO_LONGS(page_count) *
-					   sizeof(long), GFP_KERNEL);
+		obj->bit_17 = kcalloc(BITS_TO_LONGS(page_count),
+				      sizeof(long), GFP_KERNEL);
 		if (obj->bit_17 == NULL) {
 			DRM_ERROR("Failed to allocate memory for bit 17 "
 				  "record\n");
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index c38d575..fde7c4d 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -791,7 +791,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
 
 		error->ring[i].num_requests = count;
 		error->ring[i].requests =
-			kmalloc(count*sizeof(struct drm_i915_error_request),
+			kcalloc(count, sizeof(*error->ring[i].requests),
 				GFP_ATOMIC);
 		if (error->ring[i].requests == NULL) {
 			error->ring[i].num_requests = 0;
@@ -833,7 +833,7 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
 	error->pinned_bo_count[ndx] = i - error->active_bo_count[ndx];
 
 	if (i) {
-		active_bo = kmalloc(sizeof(*active_bo)*i, GFP_ATOMIC);
+		active_bo = kcalloc(i, sizeof(*active_bo), GFP_ATOMIC);
 		if (active_bo)
 			pinned_bo = active_bo + error->active_bo_count[ndx];
 	}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fe8db37..6b8a107 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9031,7 +9031,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	unsigned disable_pipes, prepare_pipes, modeset_pipes;
 	int ret = 0;
 
-	saved_mode = kmalloc(2 * sizeof(*saved_mode), GFP_KERNEL);
+	saved_mode = kcalloc(2, sizeof(*saved_mode), GFP_KERNEL);
 	if (!saved_mode)
 		return -ENOMEM;
 	saved_hwmode = saved_mode + 1;
-- 
1.8.4.rc3

Re: [PATCH] drm/i915: Use kcalloc more

[Chris Wilson]

On Thu, Sep 19, 2013 at 02:06:39PM +0200, Daniel Vetter wrote:
> No buffer overflows here, but better safe than sorry.
> 
> v2:
> - Fixup the sizeof conversion, I've missed the pointer deref (Jani).
> - Drop the redundant GFP_ZERO, kcalloc alreads memsets (Jani).
> - Use kmalloc_array for the execbuf fastpath to avoid the memset
>   (Chris). I've opted to leave all other conversions as-is since they
>   aren't in a fastpath and dealing with cleared memory instead of
>   random garbage is just generally nicer.

I still don't agree with this change to kmalloc_array. The code is
written explicitly such that an invalid buffer_count is reported as
EINVAL and not ENOMEM.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Use kcalloc more

[Daniel Vetter]

On Thu, Sep 19, 2013 at 2:30 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Sep 19, 2013 at 02:06:39PM +0200, Daniel Vetter wrote:
>> No buffer overflows here, but better safe than sorry.
>>
>> v2:
>> - Fixup the sizeof conversion, I've missed the pointer deref (Jani).
>> - Drop the redundant GFP_ZERO, kcalloc alreads memsets (Jani).
>> - Use kmalloc_array for the execbuf fastpath to avoid the memset
>>   (Chris). I've opted to leave all other conversions as-is since they
>>   aren't in a fastpath and dealing with cleared memory instead of
>>   random garbage is just generally nicer.
>
> I still don't agree with this change to kmalloc_array. The code is
> written explicitly such that an invalid buffer_count is reported as
> EINVAL and not ENOMEM.

It's just paranoia - imo consistently using kcalloc/kmalloc array
where possible is just safer. Note also that the subtest I've added
explicitly checks for EINVAL, so if we ever botch this it should get
caught.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Use kcalloc more

[Chris Wilson]

On Thu, Sep 19, 2013 at 02:35:42PM +0200, Daniel Vetter wrote:
> On Thu, Sep 19, 2013 at 2:30 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Thu, Sep 19, 2013 at 02:06:39PM +0200, Daniel Vetter wrote:
> >> No buffer overflows here, but better safe than sorry.
> >>
> >> v2:
> >> - Fixup the sizeof conversion, I've missed the pointer deref (Jani).
> >> - Drop the redundant GFP_ZERO, kcalloc alreads memsets (Jani).
> >> - Use kmalloc_array for the execbuf fastpath to avoid the memset
> >>   (Chris). I've opted to leave all other conversions as-is since they
> >>   aren't in a fastpath and dealing with cleared memory instead of
> >>   random garbage is just generally nicer.
> >
> > I still don't agree with this change to kmalloc_array. The code is
> > written explicitly such that an invalid buffer_count is reported as
> > EINVAL and not ENOMEM.
> 
> It's just paranoia - imo consistently using kcalloc/kmalloc array
> where possible is just safer. Note also that the subtest I've added
> explicitly checks for EINVAL, so if we ever botch this it should get
> caught.

Paranoia for what? Checking the same thing twice in case the compiler
changes it mind?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Use kcalloc more

[Daniel Vetter]

On Thu, Sep 19, 2013 at 01:41:53PM +0100, Chris Wilson wrote:
> On Thu, Sep 19, 2013 at 02:35:42PM +0200, Daniel Vetter wrote:
> > On Thu, Sep 19, 2013 at 2:30 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > On Thu, Sep 19, 2013 at 02:06:39PM +0200, Daniel Vetter wrote:
> > >> No buffer overflows here, but better safe than sorry.
> > >>
> > >> v2:
> > >> - Fixup the sizeof conversion, I've missed the pointer deref (Jani).
> > >> - Drop the redundant GFP_ZERO, kcalloc alreads memsets (Jani).
> > >> - Use kmalloc_array for the execbuf fastpath to avoid the memset
> > >>   (Chris). I've opted to leave all other conversions as-is since they
> > >>   aren't in a fastpath and dealing with cleared memory instead of
> > >>   random garbage is just generally nicer.
> > >
> > > I still don't agree with this change to kmalloc_array. The code is
> > > written explicitly such that an invalid buffer_count is reported as
> > > EINVAL and not ENOMEM.
> > 
> > It's just paranoia - imo consistently using kcalloc/kmalloc array
> > where possible is just safer. Note also that the subtest I've added
> > explicitly checks for EINVAL, so if we ever botch this it should get
> > caught.
> 
> Paranoia for what? Checking the same thing twice in case the compiler
> changes it mind?

The compiler actually removes the 2nd check since it's the same ;-) I just
like the consisten pattern and cozy feeling that we'll have less to worry
for potential overflows. I can ditch it if you deem it too offensive.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Use kcalloc more

[Chris Wilson]

On Thu, Sep 19, 2013 at 02:51:10PM +0200, Daniel Vetter wrote:
> On Thu, Sep 19, 2013 at 01:41:53PM +0100, Chris Wilson wrote:
> > On Thu, Sep 19, 2013 at 02:35:42PM +0200, Daniel Vetter wrote:
> > > On Thu, Sep 19, 2013 at 2:30 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > On Thu, Sep 19, 2013 at 02:06:39PM +0200, Daniel Vetter wrote:
> > > >> No buffer overflows here, but better safe than sorry.
> > > >>
> > > >> v2:
> > > >> - Fixup the sizeof conversion, I've missed the pointer deref (Jani).
> > > >> - Drop the redundant GFP_ZERO, kcalloc alreads memsets (Jani).
> > > >> - Use kmalloc_array for the execbuf fastpath to avoid the memset
> > > >>   (Chris). I've opted to leave all other conversions as-is since they
> > > >>   aren't in a fastpath and dealing with cleared memory instead of
> > > >>   random garbage is just generally nicer.
> > > >
> > > > I still don't agree with this change to kmalloc_array. The code is
> > > > written explicitly such that an invalid buffer_count is reported as
> > > > EINVAL and not ENOMEM.
> > > 
> > > It's just paranoia - imo consistently using kcalloc/kmalloc array
> > > where possible is just safer. Note also that the subtest I've added
> > > explicitly checks for EINVAL, so if we ever botch this it should get
> > > caught.
> > 
> > Paranoia for what? Checking the same thing twice in case the compiler
> > changes it mind?
> 
> The compiler actually removes the 2nd check since it's the same ;-) I just
> like the consisten pattern and cozy feeling that we'll have less to worry
> for potential overflows. I can ditch it if you deem it too offensive.

Having been along this road before, I preferred the explicit checking
that also gets the right return value. The goal here is perform all
sanity checks as early as possible - but I'm not going to fight to move
the cliprects test as cliprects are broken by design.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Use kcalloc more

[Daniel Vetter]

On Thu, Sep 19, 2013 at 01:58:09PM +0100, Chris Wilson wrote:
> On Thu, Sep 19, 2013 at 02:51:10PM +0200, Daniel Vetter wrote:
> > On Thu, Sep 19, 2013 at 01:41:53PM +0100, Chris Wilson wrote:
> > > On Thu, Sep 19, 2013 at 02:35:42PM +0200, Daniel Vetter wrote:
> > > > On Thu, Sep 19, 2013 at 2:30 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > > On Thu, Sep 19, 2013 at 02:06:39PM +0200, Daniel Vetter wrote:
> > > > >> No buffer overflows here, but better safe than sorry.
> > > > >>
> > > > >> v2:
> > > > >> - Fixup the sizeof conversion, I've missed the pointer deref (Jani).
> > > > >> - Drop the redundant GFP_ZERO, kcalloc alreads memsets (Jani).
> > > > >> - Use kmalloc_array for the execbuf fastpath to avoid the memset
> > > > >>   (Chris). I've opted to leave all other conversions as-is since they
> > > > >>   aren't in a fastpath and dealing with cleared memory instead of
> > > > >>   random garbage is just generally nicer.
> > > > >
> > > > > I still don't agree with this change to kmalloc_array. The code is
> > > > > written explicitly such that an invalid buffer_count is reported as
> > > > > EINVAL and not ENOMEM.
> > > > 
> > > > It's just paranoia - imo consistently using kcalloc/kmalloc array
> > > > where possible is just safer. Note also that the subtest I've added
> > > > explicitly checks for EINVAL, so if we ever botch this it should get
> > > > caught.
> > > 
> > > Paranoia for what? Checking the same thing twice in case the compiler
> > > changes it mind?
> > 
> > The compiler actually removes the 2nd check since it's the same ;-) I just
> > like the consisten pattern and cozy feeling that we'll have less to worry
> > for potential overflows. I can ditch it if you deem it too offensive.
> 
> Having been along this road before, I preferred the explicit checking
> that also gets the right return value. The goal here is perform all
> sanity checks as early as possible - but I'm not going to fight to move
> the cliprects test as cliprects are broken by design.

I've dropped the contentious hunk and merged all the reviewed patches.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Use kcalloc more

[Jani Nikula]

On Thu, 19 Sep 2013, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> No buffer overflows here, but better safe than sorry.
>
> v2:
> - Fixup the sizeof conversion, I've missed the pointer deref (Jani).
> - Drop the redundant GFP_ZERO, kcalloc alreads memsets (Jani).
> - Use kmalloc_array for the execbuf fastpath to avoid the memset
>   (Chris). I've opted to leave all other conversions as-is since they
>   aren't in a fastpath and dealing with cleared memory instead of
>   random garbage is just generally nicer.

Whether the change to kmalloc_array makes sense or not I leave for you
and Chris to figure out; otherwise it looks like it does what it says on
the box.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 8 +++++---
>  drivers/gpu/drm/i915/i915_gem_gtt.c        | 4 ++--
>  drivers/gpu/drm/i915/i915_gem_tiling.c     | 6 +++---
>  drivers/gpu/drm/i915/i915_gpu_error.c      | 4 ++--
>  drivers/gpu/drm/i915/intel_display.c       | 2 +-
>  5 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index ee93357..ccfb8e6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1047,7 +1047,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  			return -EINVAL;
>  		}
>  
> -		cliprects = kmalloc(args->num_cliprects * sizeof(*cliprects),
> +		cliprects = kcalloc(args->num_cliprects,
> +				    sizeof(*cliprects),
>  				    GFP_KERNEL);
>  		if (cliprects == NULL) {
>  			ret = -ENOMEM;
> @@ -1302,8 +1303,9 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
>  		return -EINVAL;
>  	}
>  
> -	exec2_list = kmalloc(sizeof(*exec2_list)*args->buffer_count,
> -			     GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
> +	exec2_list = kmalloc_array(args->buffer_count, sizeof(*exec2_list),
> +				   GFP_TEMPORARY |
> +				   __GFP_NOWARN | __GFP_NORETRY);
>  	if (exec2_list == NULL)
>  		exec2_list = drm_malloc_ab(sizeof(*exec2_list),
>  					   args->buffer_count);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 212f6d8..e999496 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -336,7 +336,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  	ppgtt->base.insert_entries = gen6_ppgtt_insert_entries;
>  	ppgtt->base.cleanup = gen6_ppgtt_cleanup;
>  	ppgtt->base.scratch = dev_priv->gtt.base.scratch;
> -	ppgtt->pt_pages = kzalloc(sizeof(struct page *)*ppgtt->num_pd_entries,
> +	ppgtt->pt_pages = kcalloc(ppgtt->num_pd_entries, sizeof(struct page *),
>  				  GFP_KERNEL);
>  	if (!ppgtt->pt_pages)
>  		return -ENOMEM;
> @@ -347,7 +347,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  			goto err_pt_alloc;
>  	}
>  
> -	ppgtt->pt_dma_addr = kzalloc(sizeof(dma_addr_t) *ppgtt->num_pd_entries,
> +	ppgtt->pt_dma_addr = kcalloc(ppgtt->num_pd_entries, sizeof(dma_addr_t),
>  				     GFP_KERNEL);
>  	if (!ppgtt->pt_dma_addr)
>  		goto err_pt_alloc;
> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> index 032e9ef..ac9ebe9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -393,7 +393,7 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
>  	/* Try to preallocate memory required to save swizzling on put-pages */
>  	if (i915_gem_object_needs_bit17_swizzle(obj)) {
>  		if (obj->bit_17 == NULL) {
> -			obj->bit_17 = kmalloc(BITS_TO_LONGS(obj->base.size >> PAGE_SHIFT) *
> +			obj->bit_17 = kcalloc(BITS_TO_LONGS(obj->base.size >> PAGE_SHIFT),
>  					      sizeof(long), GFP_KERNEL);
>  		}
>  	} else {
> @@ -504,8 +504,8 @@ i915_gem_object_save_bit_17_swizzle(struct drm_i915_gem_object *obj)
>  	int i;
>  
>  	if (obj->bit_17 == NULL) {
> -		obj->bit_17 = kmalloc(BITS_TO_LONGS(page_count) *
> -					   sizeof(long), GFP_KERNEL);
> +		obj->bit_17 = kcalloc(BITS_TO_LONGS(page_count),
> +				      sizeof(long), GFP_KERNEL);
>  		if (obj->bit_17 == NULL) {
>  			DRM_ERROR("Failed to allocate memory for bit 17 "
>  				  "record\n");
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index c38d575..fde7c4d 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -791,7 +791,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
>  
>  		error->ring[i].num_requests = count;
>  		error->ring[i].requests =
> -			kmalloc(count*sizeof(struct drm_i915_error_request),
> +			kcalloc(count, sizeof(*error->ring[i].requests),
>  				GFP_ATOMIC);
>  		if (error->ring[i].requests == NULL) {
>  			error->ring[i].num_requests = 0;
> @@ -833,7 +833,7 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
>  	error->pinned_bo_count[ndx] = i - error->active_bo_count[ndx];
>  
>  	if (i) {
> -		active_bo = kmalloc(sizeof(*active_bo)*i, GFP_ATOMIC);
> +		active_bo = kcalloc(i, sizeof(*active_bo), GFP_ATOMIC);
>  		if (active_bo)
>  			pinned_bo = active_bo + error->active_bo_count[ndx];
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fe8db37..6b8a107 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9031,7 +9031,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  	unsigned disable_pipes, prepare_pipes, modeset_pipes;
>  	int ret = 0;
>  
> -	saved_mode = kmalloc(2 * sizeof(*saved_mode), GFP_KERNEL);
> +	saved_mode = kcalloc(2, sizeof(*saved_mode), GFP_KERNEL);
>  	if (!saved_mode)
>  		return -ENOMEM;
>  	saved_hwmode = saved_mode + 1;
> -- 
> 1.8.4.rc3
>

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH 3/5] drm/i915: Ditch INTELFB_CONN_LIMIT

[Daniel Vetter]

And the gratious overallocation of crtcs. Seems to go back to the ums
days of yonder ...

We also still need it to make the fbdev emulation happy, but I don't
think there's really a need. Especially since the current fbdev
emulation doesn't actually support cloning.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 drivers/gpu/drm/i915/intel_drv.h     | 1 -
 drivers/gpu/drm/i915/intel_fb.c      | 2 +-
 3 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6b8a107..52695cd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9570,7 +9570,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	struct intel_crtc *intel_crtc;
 	int i;
 
-	intel_crtc = kzalloc(sizeof(struct intel_crtc) + (INTELFB_CONN_LIMIT * sizeof(struct drm_connector *)), GFP_KERNEL);
+	intel_crtc = kzalloc(sizeof(struct intel_crtc), GFP_KERNEL);
 	if (intel_crtc == NULL)
 		return;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b85354f..c052bcf 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -77,7 +77,6 @@
 /* the i915, i945 have a single sDVO i2c bus - which is different */
 #define MAX_OUTPUTS 6
 /* maximum connectors per crtcs in the mode set */
-#define INTELFB_CONN_LIMIT 4
 
 #define INTEL_I2C_BUS_DVO 1
 #define INTEL_I2C_BUS_SDVO 2
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 6aa66aa..7ceb69b 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -225,7 +225,7 @@ int intel_fbdev_init(struct drm_device *dev)
 
 	ret = drm_fb_helper_init(dev, &ifbdev->helper,
 				 INTEL_INFO(dev)->num_pipes,
-				 INTELFB_CONN_LIMIT);
+				 4);
 	if (ret) {
 		kfree(ifbdev);
 		return ret;
-- 
1.8.4.rc3

Re: [PATCH 3/5] drm/i915: Ditch INTELFB_CONN_LIMIT

[Jani Nikula]

On Thu, 19 Sep 2013, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> And the gratious overallocation of crtcs. Seems to go back to the ums
> days of yonder ...
>
> We also still need it to make the fbdev emulation happy, but I don't
> think there's really a need. Especially since the current fbdev
> emulation doesn't actually support cloning.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  drivers/gpu/drm/i915/intel_drv.h     | 1 -
>  drivers/gpu/drm/i915/intel_fb.c      | 2 +-
>  3 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6b8a107..52695cd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9570,7 +9570,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	struct intel_crtc *intel_crtc;
>  	int i;
>  
> -	intel_crtc = kzalloc(sizeof(struct intel_crtc) + (INTELFB_CONN_LIMIT * sizeof(struct drm_connector *)), GFP_KERNEL);
> +	intel_crtc = kzalloc(sizeof(struct intel_crtc), GFP_KERNEL);

*cough* sizeof(*intel_crtc) *cough*

Cheers,
Jani.

>  	if (intel_crtc == NULL)
>  		return;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index b85354f..c052bcf 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -77,7 +77,6 @@
>  /* the i915, i945 have a single sDVO i2c bus - which is different */
>  #define MAX_OUTPUTS 6
>  /* maximum connectors per crtcs in the mode set */
> -#define INTELFB_CONN_LIMIT 4
>  
>  #define INTEL_I2C_BUS_DVO 1
>  #define INTEL_I2C_BUS_SDVO 2
> diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
> index 6aa66aa..7ceb69b 100644
> --- a/drivers/gpu/drm/i915/intel_fb.c
> +++ b/drivers/gpu/drm/i915/intel_fb.c
> @@ -225,7 +225,7 @@ int intel_fbdev_init(struct drm_device *dev)
>  
>  	ret = drm_fb_helper_init(dev, &ifbdev->helper,
>  				 INTEL_INFO(dev)->num_pipes,
> -				 INTELFB_CONN_LIMIT);
> +				 4);
>  	if (ret) {
>  		kfree(ifbdev);
>  		return ret;
> -- 
> 1.8.4.rc3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH] drm/i915: Ditch INTELFB_CONN_LIMIT

[Daniel Vetter]

And the gratious overallocation of crtcs. Seems to go back to the ums
days of yonder ...

We also still need it to make the fbdev emulation happy, but I don't
think there's really a need. Especially since the current fbdev
emulation doesn't actually support cloning.

v2: Use sizeof(*pointer) pattern (Jani).

Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 drivers/gpu/drm/i915/intel_drv.h     | 1 -
 drivers/gpu/drm/i915/intel_fb.c      | 2 +-
 3 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6b8a107..1c5c2c1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9570,7 +9570,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	struct intel_crtc *intel_crtc;
 	int i;
 
-	intel_crtc = kzalloc(sizeof(struct intel_crtc) + (INTELFB_CONN_LIMIT * sizeof(struct drm_connector *)), GFP_KERNEL);
+	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
 	if (intel_crtc == NULL)
 		return;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b85354f..c052bcf 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -77,7 +77,6 @@
 /* the i915, i945 have a single sDVO i2c bus - which is different */
 #define MAX_OUTPUTS 6
 /* maximum connectors per crtcs in the mode set */
-#define INTELFB_CONN_LIMIT 4
 
 #define INTEL_I2C_BUS_DVO 1
 #define INTEL_I2C_BUS_SDVO 2
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 6aa66aa..7ceb69b 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -225,7 +225,7 @@ int intel_fbdev_init(struct drm_device *dev)
 
 	ret = drm_fb_helper_init(dev, &ifbdev->helper,
 				 INTEL_INFO(dev)->num_pipes,
-				 INTELFB_CONN_LIMIT);
+				 4);
 	if (ret) {
 		kfree(ifbdev);
 		return ret;
-- 
1.8.4.rc3

Re: [PATCH] drm/i915: Ditch INTELFB_CONN_LIMIT

[Jani Nikula]

On Thu, 19 Sep 2013, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> And the gratious overallocation of crtcs. Seems to go back to the ums
> days of yonder ...
>
> We also still need it to make the fbdev emulation happy, but I don't
> think there's really a need. Especially since the current fbdev
> emulation doesn't actually support cloning.
>
> v2: Use sizeof(*pointer) pattern (Jani).
>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  drivers/gpu/drm/i915/intel_drv.h     | 1 -
>  drivers/gpu/drm/i915/intel_fb.c      | 2 +-
>  3 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6b8a107..1c5c2c1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9570,7 +9570,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	struct intel_crtc *intel_crtc;
>  	int i;
>  
> -	intel_crtc = kzalloc(sizeof(struct intel_crtc) + (INTELFB_CONN_LIMIT * sizeof(struct drm_connector *)), GFP_KERNEL);
> +	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
>  	if (intel_crtc == NULL)
>  		return;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index b85354f..c052bcf 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -77,7 +77,6 @@
>  /* the i915, i945 have a single sDVO i2c bus - which is different */
>  #define MAX_OUTPUTS 6
>  /* maximum connectors per crtcs in the mode set */
> -#define INTELFB_CONN_LIMIT 4
>  
>  #define INTEL_I2C_BUS_DVO 1
>  #define INTEL_I2C_BUS_SDVO 2
> diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
> index 6aa66aa..7ceb69b 100644
> --- a/drivers/gpu/drm/i915/intel_fb.c
> +++ b/drivers/gpu/drm/i915/intel_fb.c
> @@ -225,7 +225,7 @@ int intel_fbdev_init(struct drm_device *dev)
>  
>  	ret = drm_fb_helper_init(dev, &ifbdev->helper,
>  				 INTEL_INFO(dev)->num_pipes,
> -				 INTELFB_CONN_LIMIT);
> +				 4);
>  	if (ret) {
>  		kfree(ifbdev);
>  		return ret;
> -- 
> 1.8.4.rc3
>

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH 4/5] drm/i915: check for allocation overflow in error state capture

[Daniel Vetter]

Pretty harmless since actually binding such a giant thing would be
really hard to pull off - it doesn't fit into the gtt of any shipping
gpu right now.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 763283e..6c80636 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -478,7 +478,7 @@ static void i915_error_state_free(struct kref *error_ref)
 static struct drm_i915_error_object *
 i915_error_object_create_sized(struct drm_i915_private *dev_priv,
 			       struct drm_i915_gem_object *src,
-			       const int num_pages)
+			       const unsigned int num_pages)
 {
 	struct drm_i915_error_object *dst;
 	int i;
@@ -487,6 +487,12 @@ i915_error_object_create_sized(struct drm_i915_private *dev_priv,
 	if (src == NULL || src->pages == NULL)
 		return NULL;
 
+	if (num_pages > (UINT_MAX - sizeof(*dst)) / sizeof(u32 *)) {
+		DRM_DEBUG("error object with overflowing num_pages %u\n",
+			  num_pages);
+		return NULL;
+	}
+
 	dst = kmalloc(sizeof(*dst) + num_pages * sizeof(u32 *), GFP_ATOMIC);
 	if (dst == NULL)
 		return NULL;
-- 
1.8.4.rc3

Re: [PATCH 4/5] drm/i915: check for allocation overflow in error state capture

[Ben Widawsky]

On Thu, Sep 19, 2013 at 12:18:35PM +0200, Daniel Vetter wrote:
> Pretty harmless since actually binding such a giant thing would be
> really hard to pull off - it doesn't fit into the gtt of any shipping
> gpu right now.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gpu_error.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 763283e..6c80636 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -478,7 +478,7 @@ static void i915_error_state_free(struct kref *error_ref)
>  static struct drm_i915_error_object *
>  i915_error_object_create_sized(struct drm_i915_private *dev_priv,
>  			       struct drm_i915_gem_object *src,
> -			       const int num_pages)
> +			       const unsigned int num_pages)
>  {
>  	struct drm_i915_error_object *dst;
>  	int i;
> @@ -487,6 +487,12 @@ i915_error_object_create_sized(struct drm_i915_private *dev_priv,
>  	if (src == NULL || src->pages == NULL)
>  		return NULL;
>  
> +	if (num_pages > (UINT_MAX - sizeof(*dst)) / sizeof(u32 *)) {
> +		DRM_DEBUG("error object with overflowing num_pages %u\n",
> +			  num_pages);
> +		return NULL;
> +	}
> +

I think either of these two assertions would be much better:
if (num_pages > src->base.size >> PAGE_SHIFT)

or

if (num_pages > dev_priv->gtt.base.total >> 12)...

Later with PPGTT, the gtt will just be a VM.

>  	dst = kmalloc(sizeof(*dst) + num_pages * sizeof(u32 *), GFP_ATOMIC);
>  	if (dst == NULL)
>  		return NULL;

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH 5/5] drm/i915: Use unsigned for overflow checks in execbuf

[Daniel Vetter]

There's actually no real risk since we already check for stricter
constraints earlier (using UINT_MAX / sizeof (struct
drm_i915_gem_exec_object2) as the limit). But in eb_create we use
signed integers, which steals a factor of 2. Luckily struct
drm_i915_gem_exec_object2 for this to not matter.

Still, be consistent and use unsigned integers.

Similar use unsinged integers when checking for overflows in the
relocation entry processing.

I've also added a new subtests to igt/gem_reloc_overflow to also
test for overflowing args->buffer_count values.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index a733118..cd98aeb 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -45,18 +45,19 @@ struct eb_vmas {
 static struct eb_vmas *
 eb_create(struct drm_i915_gem_execbuffer2 *args, struct i915_address_space *vm)
 {
+	unsigned size, count;
 	struct eb_vmas *eb = NULL;
 
 	if (args->flags & I915_EXEC_HANDLE_LUT) {
-		int size = args->buffer_count;
+		size = args->buffer_count;
 		size *= sizeof(struct i915_vma *);
 		size += sizeof(struct eb_vmas);
 		eb = kmalloc(size, GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
 	}
 
 	if (eb == NULL) {
-		int size = args->buffer_count;
-		int count = PAGE_SIZE / sizeof(struct hlist_head) / 2;
+		size = args->buffer_count;
+		count = PAGE_SIZE / sizeof(struct hlist_head) / 2;
 		BUILD_BUG_ON_NOT_POWER_OF_2(PAGE_SIZE / sizeof(struct hlist_head));
 		while (count > 2*size)
 			count >>= 1;
@@ -667,7 +668,7 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
 	bool need_relocs;
 	int *reloc_offset;
 	int i, total, ret;
-	int count = args->buffer_count;
+	unsigned count = args->buffer_count;
 
 	if (WARN_ON(list_empty(&eb->vmas)))
 		return 0;
@@ -818,8 +819,8 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
 		   int count)
 {
 	int i;
-	int relocs_total = 0;
-	int relocs_max = INT_MAX / sizeof(struct drm_i915_gem_relocation_entry);
+	unsigned relocs_total = 0;
+	unsigned relocs_max = UINT_MAX / sizeof(struct drm_i915_gem_relocation_entry);
 
 	for (i = 0; i < count; i++) {
 		char __user *ptr = to_user_ptr(exec[i].relocs_ptr);
-- 
1.8.4.rc3

Re: [PATCH 5/5] drm/i915: Use unsigned for overflow checks in execbuf

[Chris Wilson]

On Thu, Sep 19, 2013 at 12:18:36PM +0200, Daniel Vetter wrote:
> There's actually no real risk since we already check for stricter
> constraints earlier (using UINT_MAX / sizeof (struct
> drm_i915_gem_exec_object2) as the limit). But in eb_create we use
> signed integers, which steals a factor of 2. Luckily struct
> drm_i915_gem_exec_object2 for this to not matter.
> 
> Still, be consistent and use unsigned integers.
> 
> Similar use unsinged integers when checking for overflows in the
> relocation entry processing.
> 
> I've also added a new subtests to igt/gem_reloc_overflow to also
> test for overflowing args->buffer_count values.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index a733118..cd98aeb 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -45,18 +45,19 @@ struct eb_vmas {
>  static struct eb_vmas *
>  eb_create(struct drm_i915_gem_execbuffer2 *args, struct i915_address_space *vm)
>  {
> +	unsigned size, count;
>  	struct eb_vmas *eb = NULL;
>  
>  	if (args->flags & I915_EXEC_HANDLE_LUT) {
> -		int size = args->buffer_count;

These were locally scoped so that the reader could be sure that we
didn't leak information from one branch to the other. The variables
exist only to break the computation up into manageable chunks, not
because they are useful outside of those branches.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Use unsigned for overflow checks in execbuf

[Daniel Vetter]

There's actually no real risk since we already check for stricter
constraints earlier (using UINT_MAX / sizeof (struct
drm_i915_gem_exec_object2) as the limit). But in eb_create we use
signed integers, which steals a factor of 2. Luckily struct
drm_i915_gem_exec_object2 for this to not matter.

Still, be consistent and use unsigned integers.

Similar use unsinged integers when checking for overflows in the
relocation entry processing.

I've also added a new subtests to igt/gem_reloc_overflow to also
test for overflowing args->buffer_count values.

v2: Give the variables again tighter scope to make it clear that the
computation is purely local and doesn't leak out to the 2nd block.
Requested by Chris Wilson.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index a733118..742b127 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -48,15 +48,15 @@ eb_create(struct drm_i915_gem_execbuffer2 *args, struct i915_address_space *vm)
 	struct eb_vmas *eb = NULL;
 
 	if (args->flags & I915_EXEC_HANDLE_LUT) {
-		int size = args->buffer_count;
+		unsigned size = args->buffer_count;
 		size *= sizeof(struct i915_vma *);
 		size += sizeof(struct eb_vmas);
 		eb = kmalloc(size, GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
 	}
 
 	if (eb == NULL) {
-		int size = args->buffer_count;
-		int count = PAGE_SIZE / sizeof(struct hlist_head) / 2;
+		unsigned size = args->buffer_count;
+		unsigned count = PAGE_SIZE / sizeof(struct hlist_head) / 2;
 		BUILD_BUG_ON_NOT_POWER_OF_2(PAGE_SIZE / sizeof(struct hlist_head));
 		while (count > 2*size)
 			count >>= 1;
@@ -667,7 +667,7 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
 	bool need_relocs;
 	int *reloc_offset;
 	int i, total, ret;
-	int count = args->buffer_count;
+	unsigned count = args->buffer_count;
 
 	if (WARN_ON(list_empty(&eb->vmas)))
 		return 0;
@@ -818,8 +818,8 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
 		   int count)
 {
 	int i;
-	int relocs_total = 0;
-	int relocs_max = INT_MAX / sizeof(struct drm_i915_gem_relocation_entry);
+	unsigned relocs_total = 0;
+	unsigned relocs_max = UINT_MAX / sizeof(struct drm_i915_gem_relocation_entry);
 
 	for (i = 0; i < count; i++) {
 		char __user *ptr = to_user_ptr(exec[i].relocs_ptr);
-- 
1.8.4.rc3

[PATCH] drm/i915: Use unsigned for overflow checks in execbuf

[Daniel Vetter]

There's actually no real risk since we already check for stricter
constraints earlier (using UINT_MAX / sizeof (struct
drm_i915_gem_exec_object2) as the limit). But in eb_create we use
signed integers, which steals a factor of 2. Luckily struct
drm_i915_gem_exec_object2 for this to not matter.

Still, be consistent and use unsigned integers.

Similar use unsinged integers when checking for overflows in the
relocation entry processing.

I've also added a new subtests to igt/gem_reloc_overflow to also
test for overflowing args->buffer_count values.

v2: Give the variables again tighter scope to make it clear that the
computation is purely local and doesn't leak out to the 2nd block.
Requested by Chris Wilson.

v3: Add a comment why we don't need to recheck for overflows.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index ccfb8e6..f71eb6c 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -47,16 +47,22 @@ eb_create(struct drm_i915_gem_execbuffer2 *args, struct i915_address_space *vm)
 {
 	struct eb_vmas *eb = NULL;
 
+	/*
+	 * We already check for potential overflows of args->buffer_count before
+	 * calling i915_gem_do_execbuffer. So here we just need to make sure
+	 * that we don't overflow the by using a different type than unsigned
+	 * integers.
+	 */
 	if (args->flags & I915_EXEC_HANDLE_LUT) {
-		int size = args->buffer_count;
+		unsigned size = args->buffer_count;
 		size *= sizeof(struct i915_vma *);
 		size += sizeof(struct eb_vmas);
 		eb = kmalloc(size, GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
 	}
 
 	if (eb == NULL) {
-		int size = args->buffer_count;
-		int count = PAGE_SIZE / sizeof(struct hlist_head) / 2;
+		unsigned size = args->buffer_count;
+		unsigned count = PAGE_SIZE / sizeof(struct hlist_head) / 2;
 		BUILD_BUG_ON_NOT_POWER_OF_2(PAGE_SIZE / sizeof(struct hlist_head));
 		while (count > 2*size)
 			count >>= 1;
@@ -667,7 +673,7 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
 	bool need_relocs;
 	int *reloc_offset;
 	int i, total, ret;
-	int count = args->buffer_count;
+	unsigned count = args->buffer_count;
 
 	if (WARN_ON(list_empty(&eb->vmas)))
 		return 0;
@@ -818,8 +824,8 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
 		   int count)
 {
 	int i;
-	int relocs_total = 0;
-	int relocs_max = INT_MAX / sizeof(struct drm_i915_gem_relocation_entry);
+	unsigned relocs_total = 0;
+	unsigned relocs_max = UINT_MAX / sizeof(struct drm_i915_gem_relocation_entry);
 
 	for (i = 0; i < count; i++) {
 		char __user *ptr = to_user_ptr(exec[i].relocs_ptr);
-- 
1.8.4.rc3

Re: [PATCH] drm/i915: Use unsigned for overflow checks in execbuf

[Chris Wilson]

On Thu, Sep 19, 2013 at 02:53:36PM +0200, Daniel Vetter wrote:
> There's actually no real risk since we already check for stricter
> constraints earlier (using UINT_MAX / sizeof (struct
> drm_i915_gem_exec_object2) as the limit). But in eb_create we use
> signed integers, which steals a factor of 2. Luckily struct
> drm_i915_gem_exec_object2 for this to not matter.
> 
> Still, be consistent and use unsigned integers.
> 
> Similar use unsinged integers when checking for overflows in the
> relocation entry processing.
> 
> I've also added a new subtests to igt/gem_reloc_overflow to also
> test for overflowing args->buffer_count values.
> 
> v2: Give the variables again tighter scope to make it clear that the
> computation is purely local and doesn't leak out to the 2nd block.
> Requested by Chris Wilson.
> 
> v3: Add a comment why we don't need to recheck for overflows.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/5] drm/i915: Use unsigned for overflow checks in execbuf

[Jani Nikula]

On Thu, 19 Sep 2013, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> There's actually no real risk since we already check for stricter
> constraints earlier (using UINT_MAX / sizeof (struct
> drm_i915_gem_exec_object2) as the limit). But in eb_create we use
> signed integers, which steals a factor of 2. Luckily struct
> drm_i915_gem_exec_object2 for this to not matter.

Parse error.

> Still, be consistent and use unsigned integers.
>
> Similar use unsinged integers when checking for overflows in the
> relocation entry processing.
>
> I've also added a new subtests to igt/gem_reloc_overflow to also
> test for overflowing args->buffer_count values.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index a733118..cd98aeb 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -45,18 +45,19 @@ struct eb_vmas {
>  static struct eb_vmas *
>  eb_create(struct drm_i915_gem_execbuffer2 *args, struct i915_address_space *vm)
>  {
> +	unsigned size, count;

My OCD says always use "unsigned int", not just "unsigned". To me this
is like having:

	const foo;
	static bar;

YMMV.

Otherwise,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>  	struct eb_vmas *eb = NULL;
>  
>  	if (args->flags & I915_EXEC_HANDLE_LUT) {
> -		int size = args->buffer_count;
> +		size = args->buffer_count;
>  		size *= sizeof(struct i915_vma *);
>  		size += sizeof(struct eb_vmas);
>  		eb = kmalloc(size, GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
>  	}
>  
>  	if (eb == NULL) {
> -		int size = args->buffer_count;
> -		int count = PAGE_SIZE / sizeof(struct hlist_head) / 2;
> +		size = args->buffer_count;
> +		count = PAGE_SIZE / sizeof(struct hlist_head) / 2;
>  		BUILD_BUG_ON_NOT_POWER_OF_2(PAGE_SIZE / sizeof(struct hlist_head));
>  		while (count > 2*size)
>  			count >>= 1;
> @@ -667,7 +668,7 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
>  	bool need_relocs;
>  	int *reloc_offset;
>  	int i, total, ret;
> -	int count = args->buffer_count;
> +	unsigned count = args->buffer_count;
>  
>  	if (WARN_ON(list_empty(&eb->vmas)))
>  		return 0;
> @@ -818,8 +819,8 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
>  		   int count)
>  {
>  	int i;
> -	int relocs_total = 0;
> -	int relocs_max = INT_MAX / sizeof(struct drm_i915_gem_relocation_entry);
> +	unsigned relocs_total = 0;
> +	unsigned relocs_max = UINT_MAX / sizeof(struct drm_i915_gem_relocation_entry);
>  
>  	for (i = 0; i < count; i++) {
>  		char __user *ptr = to_user_ptr(exec[i].relocs_ptr);
> -- 
> 1.8.4.rc3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/5] drm/i915: use pointer = k[cmz...]alloc(sizeof(*pointer), ...) pattern

[Jani Nikula]

On Thu, 19 Sep 2013, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Done while reviewing all our allocations for fubar. Also a few errant
> cases of lacking () for the sizeof operator - just a bit of OCD.
>
> I've left out all the conversions that also should use kcalloc from
> this patch  (it's only 2).

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
>  drivers/gpu/drm/i915/i915_dma.c      |  6 +++---
>  drivers/gpu/drm/i915/i915_gem.c      |  2 +-
>  drivers/gpu/drm/i915/intel_crt.c     |  2 +-
>  drivers/gpu/drm/i915/intel_ddi.c     |  6 +++---
>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>  drivers/gpu/drm/i915/intel_dp.c      |  4 ++--
>  drivers/gpu/drm/i915/intel_dvo.c     |  4 ++--
>  drivers/gpu/drm/i915/intel_fb.c      |  2 +-
>  drivers/gpu/drm/i915/intel_hdmi.c    |  4 ++--
>  drivers/gpu/drm/i915/intel_lvds.c    |  4 ++--
>  drivers/gpu/drm/i915/intel_overlay.c |  4 ++--
>  drivers/gpu/drm/i915/intel_pm.c      |  2 +-
>  drivers/gpu/drm/i915/intel_sdvo.c    | 10 +++++-----
>  drivers/gpu/drm/i915/intel_sprite.c  |  2 +-
>  drivers/gpu/drm/i915/intel_tv.c      |  4 ++--
>  16 files changed, 30 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 1d77624..821406c 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2145,7 +2145,7 @@ drm_add_fake_info_node(struct drm_minor *minor,
>  {
>  	struct drm_info_node *node;
>  
> -	node = kmalloc(sizeof(struct drm_info_node), GFP_KERNEL);
> +	node = kmalloc(sizeof(*node), GFP_KERNEL);
>  	if (node == NULL) {
>  		debugfs_remove(ent);
>  		return -ENOMEM;
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index f4f9895..f7b3d0f 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -641,7 +641,7 @@ static int i915_batchbuffer(struct drm_device *dev, void *data,
>  
>  	if (batch->num_cliprects) {
>  		cliprects = kcalloc(batch->num_cliprects,
> -				    sizeof(struct drm_clip_rect),
> +				    sizeof(*cliprects),
>  				    GFP_KERNEL);
>  		if (cliprects == NULL)
>  			return -ENOMEM;
> @@ -703,7 +703,7 @@ static int i915_cmdbuffer(struct drm_device *dev, void *data,
>  
>  	if (cmdbuf->num_cliprects) {
>  		cliprects = kcalloc(cmdbuf->num_cliprects,
> -				    sizeof(struct drm_clip_rect), GFP_KERNEL);
> +				    sizeof(*cliprects), GFP_KERNEL);
>  		if (cliprects == NULL) {
>  			ret = -ENOMEM;
>  			goto fail_batch_free;
> @@ -1472,7 +1472,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	dev->types[8] = _DRM_STAT_SECONDARY;
>  	dev->types[9] = _DRM_STAT_DMA;
>  
> -	dev_priv = kzalloc(sizeof(drm_i915_private_t), GFP_KERNEL);
> +	dev_priv = kzalloc(sizeof(*dev_priv), GFP_KERNEL);
>  	if (dev_priv == NULL)
>  		return -ENOMEM;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d00d24f..c6d0353 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4599,7 +4599,7 @@ static int i915_gem_init_phys_object(struct drm_device *dev,
>  	if (dev_priv->mm.phys_objs[id - 1] || !size)
>  		return 0;
>  
> -	phys_obj = kzalloc(sizeof(struct drm_i915_gem_phys_object), GFP_KERNEL);
> +	phys_obj = kzalloc(sizeof(*phys_obj), GFP_KERNEL);
>  	if (!phys_obj)
>  		return -ENOMEM;
>  
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 6f101d5..f9a5f3d 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -764,7 +764,7 @@ void intel_crt_init(struct drm_device *dev)
>  	if (!crt)
>  		return;
>  
> -	intel_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL);
> +	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
>  	if (!intel_connector) {
>  		kfree(crt);
>  		return;
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 4918995..46bc43c 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1337,11 +1337,11 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
>  	struct intel_connector *hdmi_connector = NULL;
>  	struct intel_connector *dp_connector = NULL;
>  
> -	intel_dig_port = kzalloc(sizeof(struct intel_digital_port), GFP_KERNEL);
> +	intel_dig_port = kzalloc(sizeof(*intel_dig_port), GFP_KERNEL);
>  	if (!intel_dig_port)
>  		return;
>  
> -	dp_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL);
> +	dp_connector = kzalloc(sizeof(*dp_connector), GFP_KERNEL);
>  	if (!dp_connector) {
>  		kfree(intel_dig_port);
>  		return;
> @@ -1381,7 +1381,7 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
>  	}
>  
>  	if (intel_encoder->type != INTEL_OUTPUT_EDP) {
> -		hdmi_connector = kzalloc(sizeof(struct intel_connector),
> +		hdmi_connector = kzalloc(sizeof(*hdmi_connector),
>  					 GFP_KERNEL);
>  		if (!hdmi_connector) {
>  			return;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4dd6561..fe8db37 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8092,7 +8092,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	     fb->pitches[0] != crtc->fb->pitches[0]))
>  		return -EINVAL;
>  
> -	work = kzalloc(sizeof *work, GFP_KERNEL);
> +	work = kzalloc(sizeof(*work), GFP_KERNEL);
>  	if (work == NULL)
>  		return -ENOMEM;
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 9770160..d840bc8 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3614,11 +3614,11 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
>  	struct drm_encoder *encoder;
>  	struct intel_connector *intel_connector;
>  
> -	intel_dig_port = kzalloc(sizeof(struct intel_digital_port), GFP_KERNEL);
> +	intel_dig_port = kzalloc(sizeof(*intel_dig_port), GFP_KERNEL);
>  	if (!intel_dig_port)
>  		return;
>  
> -	intel_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL);
> +	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
>  	if (!intel_connector) {
>  		kfree(intel_dig_port);
>  		return;
> diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> index fe65c72..3ff9e2c 100644
> --- a/drivers/gpu/drm/i915/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/intel_dvo.c
> @@ -446,11 +446,11 @@ void intel_dvo_init(struct drm_device *dev)
>  	int i;
>  	int encoder_type = DRM_MODE_ENCODER_NONE;
>  
> -	intel_dvo = kzalloc(sizeof(struct intel_dvo), GFP_KERNEL);
> +	intel_dvo = kzalloc(sizeof(*intel_dvo), GFP_KERNEL);
>  	if (!intel_dvo)
>  		return;
>  
> -	intel_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL);
> +	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
>  	if (!intel_connector) {
>  		kfree(intel_dvo);
>  		return;
> diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
> index bc21000..6aa66aa 100644
> --- a/drivers/gpu/drm/i915/intel_fb.c
> +++ b/drivers/gpu/drm/i915/intel_fb.c
> @@ -216,7 +216,7 @@ int intel_fbdev_init(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  
> -	ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
> +	ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
>  	if (!ifbdev)
>  		return -ENOMEM;
>  
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 79582f9..a6310ca 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1292,11 +1292,11 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
>  	struct intel_encoder *intel_encoder;
>  	struct intel_connector *intel_connector;
>  
> -	intel_dig_port = kzalloc(sizeof(struct intel_digital_port), GFP_KERNEL);
> +	intel_dig_port = kzalloc(sizeof(*intel_dig_port), GFP_KERNEL);
>  	if (!intel_dig_port)
>  		return;
>  
> -	intel_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL);
> +	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
>  	if (!intel_connector) {
>  		kfree(intel_dig_port);
>  		return;
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 05e5485..639650c 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -948,11 +948,11 @@ void intel_lvds_init(struct drm_device *dev)
>  		}
>  	}
>  
> -	lvds_encoder = kzalloc(sizeof(struct intel_lvds_encoder), GFP_KERNEL);
> +	lvds_encoder = kzalloc(sizeof(*lvds_encoder), GFP_KERNEL);
>  	if (!lvds_encoder)
>  		return;
>  
> -	lvds_connector = kzalloc(sizeof(struct intel_lvds_connector), GFP_KERNEL);
> +	lvds_connector = kzalloc(sizeof(*lvds_connector), GFP_KERNEL);
>  	if (!lvds_connector) {
>  		kfree(lvds_encoder);
>  		return;
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 8d6d0a1..a98a990 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -1053,7 +1053,7 @@ int intel_overlay_put_image(struct drm_device *dev, void *data,
>  		return ret;
>  	}
>  
> -	params = kmalloc(sizeof(struct put_image_params), GFP_KERNEL);
> +	params = kmalloc(sizeof(*params), GFP_KERNEL);
>  	if (!params)
>  		return -ENOMEM;
>  
> @@ -1320,7 +1320,7 @@ void intel_setup_overlay(struct drm_device *dev)
>  	if (!HAS_OVERLAY(dev))
>  		return;
>  
> -	overlay = kzalloc(sizeof(struct intel_overlay), GFP_KERNEL);
> +	overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
>  	if (!overlay)
>  		return;
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index fe19ba3..6fd2e05 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -370,7 +370,7 @@ static void intel_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
>  
>  	intel_cancel_fbc_work(dev_priv);
>  
> -	work = kzalloc(sizeof *work, GFP_KERNEL);
> +	work = kzalloc(sizeof(*work), GFP_KERNEL);
>  	if (work == NULL) {
>  		DRM_ERROR("Failed to allocate FBC work structure\n");
>  		dev_priv->display.enable_fbc(crtc, interval);
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 8aa7be5..989cf74 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -2388,7 +2388,7 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
>  	struct intel_connector *intel_connector;
>  	struct intel_sdvo_connector *intel_sdvo_connector;
>  
> -	intel_sdvo_connector = kzalloc(sizeof(struct intel_sdvo_connector), GFP_KERNEL);
> +	intel_sdvo_connector = kzalloc(sizeof(*intel_sdvo_connector), GFP_KERNEL);
>  	if (!intel_sdvo_connector)
>  		return false;
>  
> @@ -2436,7 +2436,7 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
>  	struct intel_connector *intel_connector;
>  	struct intel_sdvo_connector *intel_sdvo_connector;
>  
> -	intel_sdvo_connector = kzalloc(sizeof(struct intel_sdvo_connector), GFP_KERNEL);
> +	intel_sdvo_connector = kzalloc(sizeof(*intel_sdvo_connector), GFP_KERNEL);
>  	if (!intel_sdvo_connector)
>  		return false;
>  
> @@ -2473,7 +2473,7 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
>  	struct intel_connector *intel_connector;
>  	struct intel_sdvo_connector *intel_sdvo_connector;
>  
> -	intel_sdvo_connector = kzalloc(sizeof(struct intel_sdvo_connector), GFP_KERNEL);
> +	intel_sdvo_connector = kzalloc(sizeof(*intel_sdvo_connector), GFP_KERNEL);
>  	if (!intel_sdvo_connector)
>  		return false;
>  
> @@ -2504,7 +2504,7 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
>  	struct intel_connector *intel_connector;
>  	struct intel_sdvo_connector *intel_sdvo_connector;
>  
> -	intel_sdvo_connector = kzalloc(sizeof(struct intel_sdvo_connector), GFP_KERNEL);
> +	intel_sdvo_connector = kzalloc(sizeof(*intel_sdvo_connector), GFP_KERNEL);
>  	if (!intel_sdvo_connector)
>  		return false;
>  
> @@ -2870,7 +2870,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
>  	struct intel_encoder *intel_encoder;
>  	struct intel_sdvo *intel_sdvo;
>  	int i;
> -	intel_sdvo = kzalloc(sizeof(struct intel_sdvo), GFP_KERNEL);
> +	intel_sdvo = kzalloc(sizeof(*intel_sdvo), GFP_KERNEL);
>  	if (!intel_sdvo)
>  		return false;
>  
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 231b289..cae10bc 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1034,7 +1034,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  	if (INTEL_INFO(dev)->gen < 5)
>  		return -ENODEV;
>  
> -	intel_plane = kzalloc(sizeof(struct intel_plane), GFP_KERNEL);
> +	intel_plane = kzalloc(sizeof(*intel_plane), GFP_KERNEL);
>  	if (!intel_plane)
>  		return -ENOMEM;
>  
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index f2c6d79..adc7801 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1582,12 +1582,12 @@ intel_tv_init(struct drm_device *dev)
>  	    (tv_dac_off & TVDAC_STATE_CHG_EN) != 0)
>  		return;
>  
> -	intel_tv = kzalloc(sizeof(struct intel_tv), GFP_KERNEL);
> +	intel_tv = kzalloc(sizeof(*intel_tv), GFP_KERNEL);
>  	if (!intel_tv) {
>  		return;
>  	}
>  
> -	intel_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL);
> +	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
>  	if (!intel_connector) {
>  		kfree(intel_tv);
>  		return;
> -- 
> 1.8.4.rc3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center