| Message ID | 1308176679-13766-1-git-send-email-w.sang@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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);
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(-)