| Message ID | 1385048853-1579-2-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
I think it should be a return instead a WARN. Myabe I'm just sleeping yet, but I think the delayed work will execute this even for non HAS_PC8 and warn will always ring on non HSW. But even if I'm sleeping, since it really touch hsw registers wouldn't be safer a return instead of a warn anyway? On Thu, Nov 21, 2013 at 01:47:15PM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > We already have some checks and shouldn't be reaching these places on > !HAS_PC8 platforms, but add a WARN, just in case. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index e85d838..5566de5 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6623,6 +6623,8 @@ void hsw_enable_pc8_work(struct work_struct *__work) > struct drm_device *dev = dev_priv->dev; > uint32_t val; > > + WARN_ON(!HAS_PC8(dev)); > + > if (dev_priv->pc8.enabled) > return; > > @@ -6668,6 +6670,8 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv) > if (dev_priv->pc8.disable_count != 1) > return; > > + WARN_ON(!HAS_PC8(dev)); > + > cancel_delayed_work_sync(&dev_priv->pc8.enable_work); > if (!dev_priv->pc8.enabled) > return; > -- > 1.8.3.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2013/11/29 Rodrigo Vivi <rodrigo.vivi@gmail.com>: > I think it should be a return instead a WARN. > Myabe I'm just sleeping yet, but I think the delayed work will execute this even for non HAS_PC8 and warn will always ring on non HSW. > But even if I'm sleeping, since it really touch hsw registers wouldn't be safer a return instead of a warn anyway? The refcount should never reach zero on platforms that don't have PC8, so this should never happen. Also, functions that lead to this function also have early returns, so we really shouldn't hit this thing at all. The early return will hide the problem, while the WARN will tell me my design is wrong and I need to fix bugs :) > > > On Thu, Nov 21, 2013 at 01:47:15PM -0200, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> We already have some checks and shouldn't be reaching these places on >> !HAS_PC8 platforms, but add a WARN, just in case. >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index e85d838..5566de5 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -6623,6 +6623,8 @@ void hsw_enable_pc8_work(struct work_struct *__work) >> struct drm_device *dev = dev_priv->dev; >> uint32_t val; >> >> + WARN_ON(!HAS_PC8(dev)); >> + >> if (dev_priv->pc8.enabled) >> return; >> >> @@ -6668,6 +6670,8 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv) >> if (dev_priv->pc8.disable_count != 1) >> return; >> >> + WARN_ON(!HAS_PC8(dev)); >> + >> cancel_delayed_work_sync(&dev_priv->pc8.enable_work); >> if (!dev_priv->pc8.enabled) >> return; >> -- >> 1.8.3.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Nov 29, 2013 at 10:55 AM, Paulo Zanoni <przanoni@gmail.com> wrote: > 2013/11/29 Rodrigo Vivi <rodrigo.vivi@gmail.com>: >> I think it should be a return instead a WARN. >> Myabe I'm just sleeping yet, but I think the delayed work will execute this even for non HAS_PC8 and warn will always ring on non HSW. >> But even if I'm sleeping, since it really touch hsw registers wouldn't be safer a return instead of a warn anyway? > > The refcount should never reach zero on platforms that don't have PC8, > so this should never happen. Also, functions that lead to this > function also have early returns, so we really shouldn't hit this > thing at all. > > The early return will hide the problem, while the WARN will tell me my > design is wrong and I need to fix bugs :) Makes sense. Feel free to use Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> And sorry about the bad smtp and warns it may caused for everybody. > >> >> >> On Thu, Nov 21, 2013 at 01:47:15PM -0200, Paulo Zanoni wrote: >>> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >>> >>> We already have some checks and shouldn't be reaching these places on >>> !HAS_PC8 platforms, but add a WARN, just in case. >>> >>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_display.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>> index e85d838..5566de5 100644 >>> --- a/drivers/gpu/drm/i915/intel_display.c >>> +++ b/drivers/gpu/drm/i915/intel_display.c >>> @@ -6623,6 +6623,8 @@ void hsw_enable_pc8_work(struct work_struct *__work) >>> struct drm_device *dev = dev_priv->dev; >>> uint32_t val; >>> >>> + WARN_ON(!HAS_PC8(dev)); >>> + >>> if (dev_priv->pc8.enabled) >>> return; >>> >>> @@ -6668,6 +6670,8 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv) >>> if (dev_priv->pc8.disable_count != 1) >>> return; >>> >>> + WARN_ON(!HAS_PC8(dev)); >>> + >>> cancel_delayed_work_sync(&dev_priv->pc8.enable_work); >>> if (!dev_priv->pc8.enabled) >>> return; >>> -- >>> 1.8.3.1 >>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e85d838..5566de5 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6623,6 +6623,8 @@ void hsw_enable_pc8_work(struct work_struct *__work) struct drm_device *dev = dev_priv->dev; uint32_t val; + WARN_ON(!HAS_PC8(dev)); + if (dev_priv->pc8.enabled) return; @@ -6668,6 +6670,8 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv) if (dev_priv->pc8.disable_count != 1) return; + WARN_ON(!HAS_PC8(dev)); + cancel_delayed_work_sync(&dev_priv->pc8.enable_work); if (!dev_priv->pc8.enabled) return;