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);
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).
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...
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.
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
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?
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
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)
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.
} elsedev_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
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)
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
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?
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.
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Jesse Barnes -
Wolfram Sang