[PATCH] drm/i915: Protect against wrong reg offset and warn.

[PATCH] drm/i915: Protect against wrong reg offset and warn.

[Rodrigo Vivi]

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(+)

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);
 
 	/*
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Protect against wrong reg offset and warn.

[Patchwork]

== Series Details ==

Series: drm/i915: Protect against wrong reg offset and warn.
URL   : https://patchwork.freedesktop.org/series/49054/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4753 -> Patchwork_10065 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/49054/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_10065 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_chamelium@dp-edid-read:
      fi-kbl-7500u:       PASS -> FAIL (fdo#103841)

    igt@kms_pipe_crc_basic@read-crc-pipe-b:
      {fi-byt-clapper}:   PASS -> FAIL (fdo#107362)

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-blb-e6850:       INCOMPLETE (fdo#107718) -> PASS

    igt@kms_chamelium@hdmi-hpd-fast:
      fi-kbl-7500u:       FAIL (fdo#102672, fdo#103841) -> SKIP

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       DMESG-WARN (fdo#102614) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      {fi-byt-clapper}:   FAIL (fdo#107362, fdo#103191) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         FAIL (fdo#104008) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#102672 https://bugs.freedesktop.org/show_bug.cgi?id=102672
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718


== Participating hosts (49 -> 45) ==

  Additional (1): fi-skl-6700hq 
  Missing    (5): fi-kbl-7560u fi-byt-squawks fi-bsw-cyan fi-ilk-m540 fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4753 -> Patchwork_10065

  CI_DRM_4753: 0892613a442a70a96cba33b12bb344033b557879 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4619: 9e5fa9112546e5767d57237db8eace7c815b1996 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10065: 1622c0e1a6174b89f1a350b0e939497a7e92ab81 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

1622c0e1a617 drm/i915: Protect against wrong reg offset and warn.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10065/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Protect against wrong reg offset and warn.

[Patchwork]

== Series Details ==

Series: drm/i915: Protect against wrong reg offset and warn.
URL   : https://patchwork.freedesktop.org/series/49054/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4753_full -> Patchwork_10065_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_10065_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_setmode@basic:
      shard-apl:          PASS -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@kms_vblank@pipe-b-ts-continuation-suspend:
      shard-apl:          INCOMPLETE (fdo#103927) -> PASS

    igt@perf@polling:
      shard-hsw:          FAIL (fdo#102252) -> PASS

    
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4753 -> Patchwork_10065

  CI_DRM_4753: 0892613a442a70a96cba33b12bb344033b557879 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4619: 9e5fa9112546e5767d57237db8eace7c815b1996 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10065: 1622c0e1a6174b89f1a350b0e939497a7e92ab81 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10065/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Protect against wrong reg offset and warn.

[Paulo Zanoni]

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);
>  
>  	/*
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Protect against wrong reg offset and warn.

[Rodrigo Vivi]

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);
> >  
> >  	/*
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Protect against wrong reg offset and warn.

[Paulo Zanoni]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx