[PATCH] drm/i915: add check for valid init_clock_gating-pointer

[PATCH] drm/i915: add check for valid init_clock_gating-pointer

[Wolfram Sang]

Commit 6067aa (drm/i915: split clock gating init into per-chipset
functions) unconditionally calls the newly created
init_clock_gating-pointer. There is one case, however, where it does
not get set:

if (HAS_PCH_SPLIT(dev)) {
	...
	} else
		dev_priv->display.update_wm = NULL;
}

This can lead to a NULL-pointer exception as in
https://bugzilla.kernel.org/show_bug.cgi?id=37252

Fix it by checking if the pointer is valid before using it.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Keith Packard <keithp@keithp.com>
---

Compile tested only, due to no hardware. I was going through the list of
regressions and had my take on this one. Exploring new subsystems here,
so hopefully it's the right direction. The other solution would be
initializing the pointer to a default value, but that one I don't know.

 drivers/gpu/drm/i915/intel_display.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 81a9059..cf75856 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7511,7 +7511,8 @@ void intel_init_clock_gating(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	dev_priv->display.init_clock_gating(dev);
+	if (dev_priv->display.init_clock_gating)
+		dev_priv->display.init_clock_gating(dev);
 
 	if (dev_priv->display.init_pch_clock_gating)
 		dev_priv->display.init_pch_clock_gating(dev);
-- 
1.7.2.5

Re: [PATCH] drm/i915: add check for valid init_clock_gating-pointer

[Jesse Barnes]

On Thu, 16 Jun 2011 00:24:39 +0200
Wolfram Sang <w.sang@pengutronix.de> wrote:

> Commit 6067aa (drm/i915: split clock gating init into per-chipset
> functions) unconditionally calls the newly created
> init_clock_gating-pointer. There is one case, however, where it does
> not get set:
> 
> if (HAS_PCH_SPLIT(dev)) {
> 	...
> 	} else
> 		dev_priv->display.update_wm = NULL;
> }

We'll only hit this path on non-existent hardware.  Since a clock
gating routine is required I'd rather just see the panic and add a new
routine at that time (i.e. what we normally do during bringup).

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: add check for valid init_clock_gating-pointer

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 838 bytes --]

> > Commit 6067aa (drm/i915: split clock gating init into per-chipset
> > functions) unconditionally calls the newly created
> > init_clock_gating-pointer. There is one case, however, where it does
> > not get set:
> > 
> > if (HAS_PCH_SPLIT(dev)) {
> > 	...
> > 	} else
> > 		dev_priv->display.update_wm = NULL;
> > }
> 
> We'll only hit this path on non-existent hardware.  Since a clock
> gating routine is required I'd rather just see the panic and add a new
> routine at that time (i.e. what we normally do during bringup).

How about BUG_ON(!ptr) in the init-routine for a bit more grace? And/or
a warning in the else-block? It seems to happen to users...

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915: add check for valid init_clock_gating-pointer

[Jesse Barnes]

On Thu, 16 Jun 2011 15:28:46 +0200
Wolfram Sang <w.sang@pengutronix.de> wrote:

> > > Commit 6067aa (drm/i915: split clock gating init into per-chipset
> > > functions) unconditionally calls the newly created
> > > init_clock_gating-pointer. There is one case, however, where it does
> > > not get set:
> > > 
> > > if (HAS_PCH_SPLIT(dev)) {
> > > 	...
> > > 	} else
> > > 		dev_priv->display.update_wm = NULL;
> > > }
> > 
> > We'll only hit this path on non-existent hardware.  Since a clock
> > gating routine is required I'd rather just see the panic and add a new
> > routine at that time (i.e. what we normally do during bringup).
> 
> How about BUG_ON(!ptr) in the init-routine for a bit more grace? And/or
> a warning in the else-block? It seems to happen to users...

Yeah, a BUG_ON would be fine.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: add check for valid init_clock_gating-pointer

[Chris Wilson]

On Thu, 16 Jun 2011 08:15:57 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Thu, 16 Jun 2011 15:28:46 +0200
> Wolfram Sang <w.sang@pengutronix.de> wrote:
> > How about BUG_ON(!ptr) in the init-routine for a bit more grace? And/or
> > a warning in the else-block? It seems to happen to users...
> 
> Yeah, a BUG_ON would be fine.

if (WARN_ON(!ptr, "no display vtable"))
	return -ENODEV;
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: add check for valid init_clock_gating-pointer

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 850 bytes --]

On Thu, Jun 16, 2011 at 04:47:53PM +0100, Chris Wilson wrote:
> On Thu, 16 Jun 2011 08:15:57 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > On Thu, 16 Jun 2011 15:28:46 +0200
> > Wolfram Sang <w.sang@pengutronix.de> wrote:
> > > How about BUG_ON(!ptr) in the init-routine for a bit more grace? And/or
> > > a warning in the else-block? It seems to happen to users...
> > 
> > Yeah, a BUG_ON would be fine.
> 
> if (WARN_ON(!ptr, "no display vtable"))
> 	return -ENODEV;

That would mean converting the involved void-functions to int to propagate the
error (intel_init_clock_gating, intel_modeset_init). Not a big deal, but quite
intrusive. Do you really mean that?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915: add check for valid init_clock_gating-pointer

[Chris Wilson]

On Sun, 19 Jun 2011 21:22:11 +0200, Wolfram Sang <w.sang@pengutronix.de> wrote:
> On Thu, Jun 16, 2011 at 04:47:53PM +0100, Chris Wilson wrote:
> > On Thu, 16 Jun 2011 08:15:57 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > On Thu, 16 Jun 2011 15:28:46 +0200
> > > Wolfram Sang <w.sang@pengutronix.de> wrote:
> > > > How about BUG_ON(!ptr) in the init-routine for a bit more grace? And/or
> > > > a warning in the else-block? It seems to happen to users...
> > > 
> > > Yeah, a BUG_ON would be fine.
> > 
> > if (WARN_ON(!ptr, "no display vtable"))
> > 	return -ENODEV;
> 
> That would mean converting the involved void-functions to int to propagate the
> error (intel_init_clock_gating, intel_modeset_init). Not a big deal, but quite
> intrusive. Do you really mean that?

A choice between a BUG_ON and error propagation? Choose error propagation,
one day it will be for real. ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: gracefully bail out when init_clock_gating-pointer is not set

[Wolfram Sang]

Commit 6067aa (drm/i915: split clock gating init into per-chipset
functions) introduces an init_clock_gating-pointer. There is one case,
however, where it does not get set, so that caused an OOPS (Bug 37252).
Change the code to return -ENODEV in this case and propagate it to the
upper layers.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---

This is my take on the sugessted solution.

 drivers/gpu/drm/i915/i915_dma.c      |    4 +++-
 drivers/gpu/drm/i915/i915_drv.h      |    2 +-
 drivers/gpu/drm/i915/intel_display.c |   20 ++++++++++++++------
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 0239e99..ace1df2 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1258,7 +1258,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (IS_GEN3(dev) && (I915_READ(ECOSKPD) & ECO_FLIP_DONE))
 		dev_priv->flip_pending_is_done = true;
 
-	intel_modeset_init(dev);
+	ret = intel_modeset_init(dev);
+	if (ret)
+		goto cleanup_vga_switcheroo;
 
 	ret = i915_load_gem_init(dev);
 	if (ret)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f63ee16..90ce405 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1303,7 +1303,7 @@ static inline void intel_unregister_dsm_handler(void) { return; }
 #endif /* CONFIG_ACPI */
 
 /* modesetting */
-extern void intel_modeset_init(struct drm_device *dev);
+extern int intel_modeset_init(struct drm_device *dev);
 extern void intel_modeset_gem_init(struct drm_device *dev);
 extern void intel_modeset_cleanup(struct drm_device *dev);
 extern int intel_modeset_vga_set_state(struct drm_device *dev, bool state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 81a9059..6b897db 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7518,7 +7518,7 @@ void intel_init_clock_gating(struct drm_device *dev)
 }
 
 /* Set up chip specific display functions */
-static void intel_init_display(struct drm_device *dev)
+static int intel_init_display(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -7610,8 +7610,10 @@ static void intel_init_display(struct drm_device *dev)
 			}
 			dev_priv->display.init_clock_gating = ivybridge_init_clock_gating;
 
-		} else
-			dev_priv->display.update_wm = NULL;
+		} else {
+			dev_err(dev->dev, "Unknown type!\n");
+			return -ENODEV;
+		}
 	} else if (IS_PINEVIEW(dev)) {
 		if (!intel_get_cxsr_latency(IS_PINEVIEW_G(dev),
 					    dev_priv->is_ddr3,
@@ -7657,6 +7659,8 @@ static void intel_init_display(struct drm_device *dev)
 		else
 			dev_priv->display.get_fifo_size = i830_get_fifo_size;
 	}
+
+	return 0;
 }
 
 /*
@@ -7742,10 +7746,10 @@ static void i915_disable_vga(struct drm_device *dev)
 	POSTING_READ(vga_reg);
 }
 
-void intel_modeset_init(struct drm_device *dev)
+int intel_modeset_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int i;
+	int i, ret;
 
 	drm_mode_config_init(dev);
 
@@ -7756,7 +7760,9 @@ void intel_modeset_init(struct drm_device *dev)
 
 	intel_init_quirks(dev);
 
-	intel_init_display(dev);
+	ret = intel_init_display(dev);
+	if (ret)
+		return ret;
 
 	if (IS_GEN2(dev)) {
 		dev->mode_config.max_width = 2048;
@@ -7794,6 +7800,8 @@ void intel_modeset_init(struct drm_device *dev)
 	INIT_WORK(&dev_priv->idle_work, intel_idle_update);
 	setup_timer(&dev_priv->idle_timer, intel_gpu_idle_timer,
 		    (unsigned long)dev);
+
+	return 0;
 }
 
 void intel_modeset_gem_init(struct drm_device *dev)
-- 
1.7.2.5

Re: [PATCH] drm/i915: gracefully bail out when init_clock_gating-pointer is not set

[Chris Wilson]

On Mon, 20 Jun 2011 18:10:30 +0200, Wolfram Sang <w.sang@pengutronix.de> wrote:
> Commit 6067aa (drm/i915: split clock gating init into per-chipset
> functions) introduces an init_clock_gating-pointer. There is one case,
> however, where it does not get set, so that caused an OOPS (Bug 37252).
> Change the code to return -ENODEV in this case and propagate it to the
> upper layers.

That's better, the system will continue to boot now and we will have an
appropriate error message.

> -		} else
> -			dev_priv->display.update_wm = NULL;
> +		} else {
> +			dev_err(dev->dev, "Unknown type!\n");
DRM_ERROR("Unknown chipset: %lx\n", dev->pci_device);
> +			return -ENODEV;
> +		}

DRM_ERROR to be in keeping with the rest of the code, and a bit of context
in case the error is ever pasted into bugzilla.kernel.org.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH V2] drm/i915: gracefully bail out when init_clock_gating-pointer is not set

[Wolfram Sang]

Commit 6067aa (drm/i915: split clock gating init into per-chipset
functions) introduces an init_clock_gating-pointer. There is one case,
however, where it does not get set, so that caused an OOPS. Change the
code to return -ENODEV in this case and propagate it to the upper
layers.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---

Change since V1: changed error-message per Chris' suggestion.

Note: used %04x instead of %lx to fix compile warning.

 drivers/gpu/drm/i915/i915_dma.c      |    4 +++-
 drivers/gpu/drm/i915/i915_drv.h      |    2 +-
 drivers/gpu/drm/i915/intel_display.c |   20 ++++++++++++++------
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 0239e99..ace1df2 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1258,7 +1258,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (IS_GEN3(dev) && (I915_READ(ECOSKPD) & ECO_FLIP_DONE))
 		dev_priv->flip_pending_is_done = true;
 
-	intel_modeset_init(dev);
+	ret = intel_modeset_init(dev);
+	if (ret)
+		goto cleanup_vga_switcheroo;
 
 	ret = i915_load_gem_init(dev);
 	if (ret)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f63ee16..90ce405 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1303,7 +1303,7 @@ static inline void intel_unregister_dsm_handler(void) { return; }
 #endif /* CONFIG_ACPI */
 
 /* modesetting */
-extern void intel_modeset_init(struct drm_device *dev);
+extern int intel_modeset_init(struct drm_device *dev);
 extern void intel_modeset_gem_init(struct drm_device *dev);
 extern void intel_modeset_cleanup(struct drm_device *dev);
 extern int intel_modeset_vga_set_state(struct drm_device *dev, bool state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 81a9059..57a7ba7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7518,7 +7518,7 @@ void intel_init_clock_gating(struct drm_device *dev)
 }
 
 /* Set up chip specific display functions */
-static void intel_init_display(struct drm_device *dev)
+static int intel_init_display(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -7610,8 +7610,10 @@ static void intel_init_display(struct drm_device *dev)
 			}
 			dev_priv->display.init_clock_gating = ivybridge_init_clock_gating;
 
-		} else
-			dev_priv->display.update_wm = NULL;
+		} else {
+			DRM_ERROR("Unknown chipset: %04x\n", dev->pci_device);
+			return -ENODEV;
+		}
 	} else if (IS_PINEVIEW(dev)) {
 		if (!intel_get_cxsr_latency(IS_PINEVIEW_G(dev),
 					    dev_priv->is_ddr3,
@@ -7657,6 +7659,8 @@ static void intel_init_display(struct drm_device *dev)
 		else
 			dev_priv->display.get_fifo_size = i830_get_fifo_size;
 	}
+
+	return 0;
 }
 
 /*
@@ -7742,10 +7746,10 @@ static void i915_disable_vga(struct drm_device *dev)
 	POSTING_READ(vga_reg);
 }
 
-void intel_modeset_init(struct drm_device *dev)
+int intel_modeset_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int i;
+	int i, ret;
 
 	drm_mode_config_init(dev);
 
@@ -7756,7 +7760,9 @@ void intel_modeset_init(struct drm_device *dev)
 
 	intel_init_quirks(dev);
 
-	intel_init_display(dev);
+	ret = intel_init_display(dev);
+	if (ret)
+		return ret;
 
 	if (IS_GEN2(dev)) {
 		dev->mode_config.max_width = 2048;
@@ -7794,6 +7800,8 @@ void intel_modeset_init(struct drm_device *dev)
 	INIT_WORK(&dev_priv->idle_work, intel_idle_update);
 	setup_timer(&dev_priv->idle_timer, intel_gpu_idle_timer,
 		    (unsigned long)dev);
+
+	return 0;
 }
 
 void intel_modeset_gem_init(struct drm_device *dev)
-- 
1.7.2.5

Re: [PATCH V2] drm/i915: gracefully bail out when init_clock_gating-pointer is not set

[Jesse Barnes]

On Mon, 20 Jun 2011 19:36:11 +0200
Wolfram Sang <w.sang@pengutronix.de> wrote:

> Commit 6067aa (drm/i915: split clock gating init into per-chipset
> functions) introduces an init_clock_gating-pointer. There is one case,
> however, where it does not get set, so that caused an OOPS. Change the
> code to return -ENODEV in this case and propagate it to the upper
> layers.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---

Looks good, thanks Wolfram.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH V2] drm/i915: gracefully bail out when init_clock_gating-pointer is not set

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 994 bytes --]

On Mon, Jun 20, 2011 at 10:38:54AM -0700, Jesse Barnes wrote:
> On Mon, 20 Jun 2011 19:36:11 +0200
> Wolfram Sang <w.sang@pengutronix.de> wrote:
> 
> > Commit 6067aa (drm/i915: split clock gating init into per-chipset
> > functions) introduces an init_clock_gating-pointer. There is one case,
> > however, where it does not get set, so that caused an OOPS. Change the
> > code to return -ENODEV in this case and propagate it to the upper
> > layers.
> > 
> > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> 
> Looks good, thanks Wolfram.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Just wondering: I don't see it in linux-next or the i915-branches I am aware
of. Forgotten or intentional?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V2] drm/i915: gracefully bail out when init_clock_gating-pointer is not set

[Jesse Barnes]

On Sat, 2 Jul 2011 17:55:53 +0200
Wolfram Sang <w.sang@pengutronix.de> wrote:

> On Mon, Jun 20, 2011 at 10:38:54AM -0700, Jesse Barnes wrote:
> > On Mon, 20 Jun 2011 19:36:11 +0200
> > Wolfram Sang <w.sang@pengutronix.de> wrote:
> > 
> > > Commit 6067aa (drm/i915: split clock gating init into per-chipset
> > > functions) introduces an init_clock_gating-pointer. There is one case,
> > > however, where it does not get set, so that caused an OOPS. Change the
> > > code to return -ENODEV in this case and propagate it to the upper
> > > layers.
> > > 
> > > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> > > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > 
> > Looks good, thanks Wolfram.
> > 
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Just wondering: I don't see it in linux-next or the i915-branches I am aware
> of. Forgotten or intentional?

Bounce it over to Keith with the reviewed-by and he'll pick it up.

-- 
Jesse Barnes, Intel Open Source Technology Center