[PATCH 0/5] drm/i915: CRC fixes

[PATCH 0/5] drm/i915: CRC fixes

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

A few CHV pipe C/port D specific things that slipped through the craps,
and also few generic fixes/cleanups to the CRC code.

Ville Syrjälä (5):
  drm/i915: Engage the DP scramble reset for pipe C on CHV
  drm/i915: Fix CRC support for DP port D on CHV
  drm/i915: Protect pipe_crc->entries update
  drm/i915: Allocate the pipe_crc->entires with kcalloc()
  drm/i915: Make i915_pipe_crc_read() oops proof

 drivers/gpu/drm/i915/i915_debugfs.c | 81 +++++++++++++++++++++++++------------
 drivers/gpu/drm/i915/i915_reg.h     |  3 +-
 2 files changed, 57 insertions(+), 27 deletions(-)

-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/5] drm/i915: Engage the DP scramble reset for pipe C on CHV

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

To get stable CRCs from the DP CRC source we need to reset the
scrambler for each frame. Enable the reset feature when grabbing
CRCs for pipe C on CHV. Pipes A and B were already covered due
sharing the code with VLV.

We can safely extend PIPE_SCRAMBLE_RESET_MASK to deal with CHV since
the extra bit was MBZ on the older platforms.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 27 ++++++++++++++++++++++-----
 drivers/gpu/drm/i915/i915_reg.h     |  3 ++-
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d0e445e..d74b62d 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3120,11 +3120,19 @@ static int vlv_pipe_crc_ctl_reg(struct drm_device *dev,
 		uint32_t tmp = I915_READ(PORT_DFT2_G4X);
 
 		tmp |= DC_BALANCE_RESET_VLV;
-		if (pipe == PIPE_A)
+		switch (pipe) {
+		case PIPE_A:
 			tmp |= PIPE_A_SCRAMBLE_RESET;
-		else
+			break;
+		case PIPE_B:
 			tmp |= PIPE_B_SCRAMBLE_RESET;
-
+			break;
+		case PIPE_C:
+			tmp |= PIPE_C_SCRAMBLE_RESET;
+			break;
+		default:
+			return -EINVAL;
+		}
 		I915_WRITE(PORT_DFT2_G4X, tmp);
 	}
 
@@ -3213,10 +3221,19 @@ static void vlv_undo_pipe_scramble_reset(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t tmp = I915_READ(PORT_DFT2_G4X);
 
-	if (pipe == PIPE_A)
+	switch (pipe) {
+	case PIPE_A:
 		tmp &= ~PIPE_A_SCRAMBLE_RESET;
-	else
+		break;
+	case PIPE_B:
 		tmp &= ~PIPE_B_SCRAMBLE_RESET;
+		break;
+	case PIPE_C:
+		tmp &= ~PIPE_C_SCRAMBLE_RESET;
+		break;
+	default:
+		return;
+	}
 	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK))
 		tmp &= ~DC_BALANCE_RESET_VLV;
 	I915_WRITE(PORT_DFT2_G4X, tmp);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 78d89a7..df7a708 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2787,7 +2787,8 @@ enum punit_power_well {
 #define   DC_BALANCE_RESET			(1 << 25)
 #define PORT_DFT2_G4X		(dev_priv->info.display_mmio_offset + 0x61154)
 #define   DC_BALANCE_RESET_VLV			(1 << 31)
-#define   PIPE_SCRAMBLE_RESET_MASK		(0x3 << 0)
+#define   PIPE_SCRAMBLE_RESET_MASK		((1 << 14) | (0x3 << 0))
+#define   PIPE_C_SCRAMBLE_RESET			(1 << 14) /* chv */
 #define   PIPE_B_SCRAMBLE_RESET			(1 << 1)
 #define   PIPE_A_SCRAMBLE_RESET			(1 << 0)
 
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm/i915: Fix CRC support for DP port D on CHV

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add the missing CRC control register value for DP port D on CHV.
Untested as I don't have a CHV machine with DP on port D.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d74b62d..5620b8f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3100,6 +3100,10 @@ static int vlv_pipe_crc_ctl_reg(struct drm_device *dev,
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_C_VLV;
 		need_stable_symbols = true;
 		break;
+	case INTEL_PIPE_CRC_SOURCE_DP_D:
+		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_D_VLV;
+		need_stable_symbols = true;
+		break;
 	case INTEL_PIPE_CRC_SOURCE_NONE:
 		*val = 0;
 		break;
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] drm/i915: Protect pipe_crc->entries update

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Set the pipe_crc->entries pointer while holding the relevant spinlock.
Doesn't matter too much since a spurious pipe crc interrupt would then
just update one entry but later that entry would get cleared when head
and tail are both set to 0. But being a bit more paranoid doesn't hurt.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5620b8f..3887e4f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3408,13 +3408,15 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 
 	/* none -> real source transition */
 	if (source) {
+		struct intel_pipe_crc_entry *entries;
+
 		DRM_DEBUG_DRIVER("collecting CRCs for pipe %c, %s\n",
 				 pipe_name(pipe), pipe_crc_source_name(source));
 
-		pipe_crc->entries = kzalloc(sizeof(*pipe_crc->entries) *
-					    INTEL_PIPE_CRC_ENTRIES_NR,
-					    GFP_KERNEL);
-		if (!pipe_crc->entries)
+		entries = kzalloc(sizeof(*pipe_crc->entries) *
+				  INTEL_PIPE_CRC_ENTRIES_NR,
+				  GFP_KERNEL);
+		if (!entries)
 			return -ENOMEM;
 
 		/*
@@ -3426,6 +3428,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		hsw_disable_ips(crtc);
 
 		spin_lock_irq(&pipe_crc->lock);
+		pipe_crc->entries = entries;
 		pipe_crc->head = 0;
 		pipe_crc->tail = 0;
 		spin_unlock_irq(&pipe_crc->lock);
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm/i915: Allocate the pipe_crc->entires with kcalloc()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

pipe_crc->entries[] is an array so allocate with kcalloc() instead of
kzalloc().

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 3887e4f..d9d5f1f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3413,8 +3413,8 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		DRM_DEBUG_DRIVER("collecting CRCs for pipe %c, %s\n",
 				 pipe_name(pipe), pipe_crc_source_name(source));
 
-		entries = kzalloc(sizeof(*pipe_crc->entries) *
-				  INTEL_PIPE_CRC_ENTRIES_NR,
+		entries = kcalloc(INTEL_PIPE_CRC_ENTRIES_NR,
+				  sizeof(pipe_crc->entries[0]),
 				  GFP_KERNEL);
 		if (!entries)
 			return -ENOMEM;
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/5] drm/i915: Make i915_pipe_crc_read() oops proof

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently i915_pipe_crc_read() will drop pipe_crc->lock for the entire
duration of the copy_to_user() loop, which means it'll access
pipe_crc->entries without any protection. If another thread sneaks in
and frees pipe_crc->entries the code will oops.

Reorganize the code to hold the lock around everything except
copy_to_user(). After the copy the lock is reacquired and the the number
of available entries is rechecked.

Since this is a debug feature simplify the error handling a bit by
consuming the crc entry even if copy_to_user() would fail.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 39 +++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d9d5f1f..1c61564 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2858,7 +2858,7 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
 	char buf[PIPE_CRC_BUFFER_LEN];
-	int head, tail, n_entries, n;
+	int n_entries;
 	ssize_t bytes_read;
 
 	/*
@@ -2890,36 +2890,39 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
 	}
 
 	/* We now have one or more entries to read */
-	head = pipe_crc->head;
-	tail = pipe_crc->tail;
-	n_entries = min((size_t)CIRC_CNT(head, tail, INTEL_PIPE_CRC_ENTRIES_NR),
-			count / PIPE_CRC_LINE_LEN);
-	spin_unlock_irq(&pipe_crc->lock);
+	n_entries = count / PIPE_CRC_LINE_LEN;
 
 	bytes_read = 0;
-	n = 0;
-	do {
-		struct intel_pipe_crc_entry *entry = &pipe_crc->entries[tail];
+	while (n_entries > 0) {
+		struct intel_pipe_crc_entry *entry =
+			&pipe_crc->entries[pipe_crc->tail];
 		int ret;
 
+		if (CIRC_CNT(pipe_crc->head, pipe_crc->tail,
+			     INTEL_PIPE_CRC_ENTRIES_NR) < 1)
+			break;
+
+		BUILD_BUG_ON_NOT_POWER_OF_2(INTEL_PIPE_CRC_ENTRIES_NR);
+		pipe_crc->tail = (pipe_crc->tail + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
+
 		bytes_read += snprintf(buf, PIPE_CRC_BUFFER_LEN,
 				       "%8u %8x %8x %8x %8x %8x\n",
 				       entry->frame, entry->crc[0],
 				       entry->crc[1], entry->crc[2],
 				       entry->crc[3], entry->crc[4]);
 
-		ret = copy_to_user(user_buf + n * PIPE_CRC_LINE_LEN,
-				   buf, PIPE_CRC_LINE_LEN);
+		spin_unlock_irq(&pipe_crc->lock);
+
+		ret = copy_to_user(user_buf, buf, PIPE_CRC_LINE_LEN);
 		if (ret == PIPE_CRC_LINE_LEN)
 			return -EFAULT;
 
-		BUILD_BUG_ON_NOT_POWER_OF_2(INTEL_PIPE_CRC_ENTRIES_NR);
-		tail = (tail + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
-		n++;
-	} while (--n_entries);
+		user_buf += PIPE_CRC_LINE_LEN;
+		n_entries--;
+
+		spin_lock_irq(&pipe_crc->lock);
+	}
 
-	spin_lock_irq(&pipe_crc->lock);
-	pipe_crc->tail = tail;
 	spin_unlock_irq(&pipe_crc->lock);
 
 	return bytes_read;
@@ -3456,6 +3459,8 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		spin_lock_irq(&pipe_crc->lock);
 		entries = pipe_crc->entries;
 		pipe_crc->entries = NULL;
+		pipe_crc->head = 0;
+		pipe_crc->tail = 0;
 		spin_unlock_irq(&pipe_crc->lock);
 
 		kfree(entries);
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: Fix CRC support for DP port D on CHV

[Daniel Vetter]

On Tue, Dec 09, 2014 at 09:28:29PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Add the missing CRC control register value for DP port D on CHV.
> Untested as I don't have a CHV machine with DP on port D.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index d74b62d..5620b8f 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3100,6 +3100,10 @@ static int vlv_pipe_crc_ctl_reg(struct drm_device *dev,
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_C_VLV;
>  		need_stable_symbols = true;
>  		break;
> +	case INTEL_PIPE_CRC_SOURCE_DP_D:

We need to keep rejecting DP D on vlv. I've added a check for that while
merging this patch.
-Daniel

> +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_D_VLV;
> +		need_stable_symbols = true;
> +		break;
>  	case INTEL_PIPE_CRC_SOURCE_NONE:
>  		*val = 0;
>  		break;
> -- 
> 2.0.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Make i915_pipe_crc_read() oops proof

[Daniel Vetter]

On Tue, Dec 09, 2014 at 09:28:32PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently i915_pipe_crc_read() will drop pipe_crc->lock for the entire
> duration of the copy_to_user() loop, which means it'll access
> pipe_crc->entries without any protection. If another thread sneaks in
> and frees pipe_crc->entries the code will oops.
> 
> Reorganize the code to hold the lock around everything except
> copy_to_user(). After the copy the lock is reacquired and the the number
> of available entries is rechecked.
> 
> Since this is a debug feature simplify the error handling a bit by
> consuming the crc entry even if copy_to_user() would fail.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

While you go to all this trouble with making the debugfs interface safe
against stupid userspace: set_source has a potential leak when ->entries
is already allocated when we set it. An unconditional
kfree(pipe_crc->entries); before should be all we need since kfree can
handle this. I'll smash a patch on top.

Entire series merged, thanks.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 39 +++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index d9d5f1f..1c61564 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2858,7 +2858,7 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
>  	char buf[PIPE_CRC_BUFFER_LEN];
> -	int head, tail, n_entries, n;
> +	int n_entries;
>  	ssize_t bytes_read;
>  
>  	/*
> @@ -2890,36 +2890,39 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
>  	}
>  
>  	/* We now have one or more entries to read */
> -	head = pipe_crc->head;
> -	tail = pipe_crc->tail;
> -	n_entries = min((size_t)CIRC_CNT(head, tail, INTEL_PIPE_CRC_ENTRIES_NR),
> -			count / PIPE_CRC_LINE_LEN);
> -	spin_unlock_irq(&pipe_crc->lock);
> +	n_entries = count / PIPE_CRC_LINE_LEN;
>  
>  	bytes_read = 0;
> -	n = 0;
> -	do {
> -		struct intel_pipe_crc_entry *entry = &pipe_crc->entries[tail];
> +	while (n_entries > 0) {
> +		struct intel_pipe_crc_entry *entry =
> +			&pipe_crc->entries[pipe_crc->tail];
>  		int ret;
>  
> +		if (CIRC_CNT(pipe_crc->head, pipe_crc->tail,
> +			     INTEL_PIPE_CRC_ENTRIES_NR) < 1)
> +			break;
> +
> +		BUILD_BUG_ON_NOT_POWER_OF_2(INTEL_PIPE_CRC_ENTRIES_NR);
> +		pipe_crc->tail = (pipe_crc->tail + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
> +
>  		bytes_read += snprintf(buf, PIPE_CRC_BUFFER_LEN,
>  				       "%8u %8x %8x %8x %8x %8x\n",
>  				       entry->frame, entry->crc[0],
>  				       entry->crc[1], entry->crc[2],
>  				       entry->crc[3], entry->crc[4]);
>  
> -		ret = copy_to_user(user_buf + n * PIPE_CRC_LINE_LEN,
> -				   buf, PIPE_CRC_LINE_LEN);
> +		spin_unlock_irq(&pipe_crc->lock);
> +
> +		ret = copy_to_user(user_buf, buf, PIPE_CRC_LINE_LEN);
>  		if (ret == PIPE_CRC_LINE_LEN)
>  			return -EFAULT;
>  
> -		BUILD_BUG_ON_NOT_POWER_OF_2(INTEL_PIPE_CRC_ENTRIES_NR);
> -		tail = (tail + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
> -		n++;
> -	} while (--n_entries);
> +		user_buf += PIPE_CRC_LINE_LEN;
> +		n_entries--;
> +
> +		spin_lock_irq(&pipe_crc->lock);
> +	}
>  
> -	spin_lock_irq(&pipe_crc->lock);
> -	pipe_crc->tail = tail;
>  	spin_unlock_irq(&pipe_crc->lock);
>  
>  	return bytes_read;
> @@ -3456,6 +3459,8 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>  		spin_lock_irq(&pipe_crc->lock);
>  		entries = pipe_crc->entries;
>  		pipe_crc->entries = NULL;
> +		pipe_crc->head = 0;
> +		pipe_crc->tail = 0;
>  		spin_unlock_irq(&pipe_crc->lock);
>  
>  		kfree(entries);
> -- 
> 2.0.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Protect against leaks in pipe_crc_set_source

[Daniel Vetter]

Stupid userspace (there is no evil userspace in debugfs by assumption)
might provoke a leak since we allocate the new array without holding
any locks. Drop in an unconditional kfree to deal with this - kfree
can handle NULL.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 72bb5aef9590..923e7575bb53 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3433,6 +3433,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		hsw_disable_ips(crtc);
 
 		spin_lock_irq(&pipe_crc->lock);
+		kfree(pipe_crc->entries);
 		pipe_crc->entries = entries;
 		pipe_crc->head = 0;
 		pipe_crc->tail = 0;
-- 
2.1.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Make i915_pipe_crc_read() oops proof

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  364/364              364/364
ILK              +1-6              364/366              359/366
SNB                                  448/450              448/450
IVB                                  497/498              497/498
BYT                                  289/289              289/289
HSW                                  563/564              563/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt_kms_flip_rcs-wf_vblank-vs-dpms-interruptible      PASS(6, M26)      NSPT(1, M26)
 ILK  igt_kms_flip_blocking-absolute-wf_vblank-interruptible      DMESG_WARN(1, M26)PASS(5, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_flip-vs-panning      PASS(5, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_rcs-flip-vs-dpms      PASS(4, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_rcs-flip-vs-modeset      PASS(4, M26)      NSPT(1, M26)
 ILK  igt_kms_flip_wf_vblank-ts-check      DMESG_WARN(2, M26)PASS(13, M26M37)      PASS(1, M26)
*ILK  igt_kms_flip_wf_vblank-vs-modeset-interruptible      PASS(4, M26)      DMESG_WARN(1, M26)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Protect against leaks in pipe_crc_set_source

[Ville Syrjälä]

On Wed, Dec 10, 2014 at 11:02:20AM +0100, Daniel Vetter wrote:
> Stupid userspace (there is no evil userspace in debugfs by assumption)
> might provoke a leak since we allocate the new array without holding
> any locks. Drop in an unconditional kfree to deal with this - kfree
> can handle NULL.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

I thought we had some higher level protection in pipe_crc_set_source()
but indeed we don't. So yeah it can still race with itself, but no
longer leak with your fix.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 72bb5aef9590..923e7575bb53 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3433,6 +3433,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>  		hsw_disable_ips(crtc);
>  
>  		spin_lock_irq(&pipe_crc->lock);
> +		kfree(pipe_crc->entries);
>  		pipe_crc->entries = entries;
>  		pipe_crc->head = 0;
>  		pipe_crc->tail = 0;
> -- 
> 2.1.1

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx