| Message ID | 20180903051500.10766-1-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Protect against wrong reg offset and warn. | expand |
Em Dom, 2018-09-02 às 22:15 -0700, Rodrigo Vivi escreveu: > In case we forget to change intel_port_is_tc > we would be trying to access registers with port == -1, > i.e. PORT_TC_NONE, what would be wrong offset. > There are some many things that would break if intel_port_is_tc() is incorrect that we would have to add a few dozen more WARNs. Why add this specific check at this specific case and not all other checks in other possible cases that could break? > So let's just add a protection and warn here. > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c > index 436c22de33b6..6a93fa9bbbd9 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4886,6 +4886,9 @@ static bool icl_tc_port_connected(struct > drm_i915_private *dev_priv, > bool is_legacy, is_typec, is_tbt; > u32 dpsp; > > + if (WARN_ON(tc_port == PORT_TC_NONE)) > + return false; > + > is_legacy = I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port); > > /*
On Tue, Sep 11, 2018 at 01:39:53PM -0700, Paulo Zanoni wrote: > Em Dom, 2018-09-02 às 22:15 -0700, Rodrigo Vivi escreveu: > > In case we forget to change intel_port_is_tc > > we would be trying to access registers with port == -1, > > i.e. PORT_TC_NONE, what would be wrong offset. > > > > There are some many things that would break if intel_port_is_tc() is > incorrect that we would have to add a few dozen more WARNs. Why add > this specific check at this specific case and not all other checks in > other possible cases that could break? because static analyzer tools just bother to inform us about this case here... but I can mark that as intentional/false-positive/impossible/don't-bother-anymore and just move one if you think this is really un impossible possibility > > > > So let's just add a protection and warn here. > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index 436c22de33b6..6a93fa9bbbd9 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -4886,6 +4886,9 @@ static bool icl_tc_port_connected(struct > > drm_i915_private *dev_priv, > > bool is_legacy, is_typec, is_tbt; > > u32 dpsp; > > > > + if (WARN_ON(tc_port == PORT_TC_NONE)) > > + return false; > > + > > is_legacy = I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port); > > > > /*
Em Ter, 2018-09-11 às 14:26 -0700, Rodrigo Vivi escreveu: > On Tue, Sep 11, 2018 at 01:39:53PM -0700, Paulo Zanoni wrote: > > Em Dom, 2018-09-02 às 22:15 -0700, Rodrigo Vivi escreveu: > > > In case we forget to change intel_port_is_tc > > > we would be trying to access registers with port == -1, > > > i.e. PORT_TC_NONE, what would be wrong offset. > > > > > > > There are some many things that would break if intel_port_is_tc() > > is > > incorrect that we would have to add a few dozen more WARNs. Why add > > this specific check at this specific case and not all other checks > > in > > other possible cases that could break? > > because static analyzer tools just bother to inform us about this > case here... > > but I can mark that as intentional/false-positive/impossible/don't- > bother-anymore > and just move one if you think this is really un impossible > possibility It's not impossible: every future platform that has TC has the potential to hit it until someone fixes it. So there is some value to the WARN. I was just wondering why add the code specifically here, maybe this is not the best place, but it is. Perhaps breaking intel_port_is_tc by purpose on ICL and trying to boot it would tell us the appropriate places to add WARNs and return :). > > > > > > > So let's just add a protection and warn here. > > > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > b/drivers/gpu/drm/i915/intel_dp.c > > > index 436c22de33b6..6a93fa9bbbd9 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -4886,6 +4886,9 @@ static bool icl_tc_port_connected(struct > > > drm_i915_private *dev_priv, > > > bool is_legacy, is_typec, is_tbt; > > > u32 dpsp; > > > > > > + if (WARN_ON(tc_port == PORT_TC_NONE)) > > > + return false; > > > + > > > is_legacy = I915_READ(SDEISR) & > > > SDE_TC_HOTPLUG_ICP(tc_port); > > > > > > /* > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 436c22de33b6..6a93fa9bbbd9 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4886,6 +4886,9 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv, bool is_legacy, is_typec, is_tbt; u32 dpsp; + if (WARN_ON(tc_port == PORT_TC_NONE)) + return false; + is_legacy = I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port); /*
In case we forget to change intel_port_is_tc we would be trying to access registers with port == -1, i.e. PORT_TC_NONE, what would be wrong offset. So let's just add a protection and warn here. Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 3 +++ 1 file changed, 3 insertions(+)