| Message ID | 1487076561-2169-1-git-send-email-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 14, 2017 at 02:49:21PM +0200, Jani Nikula wrote: > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > > 100% reproducible issue found on SKL SkullCanyon NUC with two external > DP daisy-chained monitors in DP/MST mode. When turning off or changing > the input of the second monitor the machine stops with a kernel > oops. This issue happened with 4.8.8 as well as drm/drm-intel-nightly. > > This issue is traced to an inconsistent control flow in > drm_dp_update_payload_part1(): the 'port' pointer is set to NULL at the > same time as 'req_payload.num_slots' is set to zero, but the pointer is > dereferenced even when req_payload.num_slot is zero. > > The problematic dereference was introduced in commit dfda0df34 > ("drm/mst: rework payload table allocation to conform better") and may > impact all versions since v3.18 > > The fix suggested by Chris Wilson removes the kernel oops and was found to > work well after 10mn of monkey-testing with the second monitor power and > input buttons > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98990 > Fixes: dfda0df34264 ("drm/mst: rework payload table allocation to conform better.") > Cc: Dave Airlie <airlied@redhat.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Nathan D Ciobanu <nathan.d.ciobanu@linux.intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Sean Paul <seanpaul@chromium.org> > Cc: <stable@vger.kernel.org> # v3.18+ > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@linux.intel.com> > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> You haz drm-misc commit rights, pls use them :-) Since it doesn't have deps, probably simplest to smash into drm-misc-fixes and then send a pull req to Dave right away. If you want, you can roll -fixes forward to -rc8 while at it. -Daniel > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index 122a1b04bebc..f2cc375907d0 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -1817,7 +1817,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) > mgr->payloads[i].vcpi = req_payload.vcpi; > } else if (mgr->payloads[i].num_slots) { > mgr->payloads[i].num_slots = 0; > - drm_dp_destroy_payload_step1(mgr, port, port->vcpi.vcpi, &mgr->payloads[i]); > + drm_dp_destroy_payload_step1(mgr, port, mgr->payloads[i].vcpi, &mgr->payloads[i]); > req_payload.payload_state = mgr->payloads[i].payload_state; > mgr->payloads[i].start_slot = 0; > } > -- > 2.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 14 Feb 2017, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Feb 14, 2017 at 02:49:21PM +0200, Jani Nikula wrote: >> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> >> 100% reproducible issue found on SKL SkullCanyon NUC with two external >> DP daisy-chained monitors in DP/MST mode. When turning off or changing >> the input of the second monitor the machine stops with a kernel >> oops. This issue happened with 4.8.8 as well as drm/drm-intel-nightly. >> >> This issue is traced to an inconsistent control flow in >> drm_dp_update_payload_part1(): the 'port' pointer is set to NULL at the >> same time as 'req_payload.num_slots' is set to zero, but the pointer is >> dereferenced even when req_payload.num_slot is zero. >> >> The problematic dereference was introduced in commit dfda0df34 >> ("drm/mst: rework payload table allocation to conform better") and may >> impact all versions since v3.18 >> >> The fix suggested by Chris Wilson removes the kernel oops and was found to >> work well after 10mn of monkey-testing with the second monitor power and >> input buttons >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98990 >> Fixes: dfda0df34264 ("drm/mst: rework payload table allocation to conform better.") >> Cc: Dave Airlie <airlied@redhat.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Nathan D Ciobanu <nathan.d.ciobanu@linux.intel.com> >> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >> Cc: Sean Paul <seanpaul@chromium.org> >> Cc: <stable@vger.kernel.org> # v3.18+ >> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@linux.intel.com> >> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > You haz drm-misc commit rights, pls use them :-) Sure, I just wanted to have a fresh CI run on this. > Since it doesn't have deps, probably simplest to smash into drm-misc-fixes > and then send a pull req to Dave right away. If you want, you can roll > -fixes forward to -rc8 while at it. Will do. BR, Jani. > -Daniel > >> --- >> drivers/gpu/drm/drm_dp_mst_topology.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c >> index 122a1b04bebc..f2cc375907d0 100644 >> --- a/drivers/gpu/drm/drm_dp_mst_topology.c >> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c >> @@ -1817,7 +1817,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) >> mgr->payloads[i].vcpi = req_payload.vcpi; >> } else if (mgr->payloads[i].num_slots) { >> mgr->payloads[i].num_slots = 0; >> - drm_dp_destroy_payload_step1(mgr, port, port->vcpi.vcpi, &mgr->payloads[i]); >> + drm_dp_destroy_payload_step1(mgr, port, mgr->payloads[i].vcpi, &mgr->payloads[i]); >> req_payload.payload_state = mgr->payloads[i].payload_state; >> mgr->payloads[i].start_slot = 0; >> } >> -- >> 2.1.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 15 Feb 2017, Jani Nikula <jani.nikula@intel.com> wrote: > On Tue, 14 Feb 2017, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Tue, Feb 14, 2017 at 02:49:21PM +0200, Jani Nikula wrote: >>> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >>> >>> 100% reproducible issue found on SKL SkullCanyon NUC with two external >>> DP daisy-chained monitors in DP/MST mode. When turning off or changing >>> the input of the second monitor the machine stops with a kernel >>> oops. This issue happened with 4.8.8 as well as drm/drm-intel-nightly. >>> >>> This issue is traced to an inconsistent control flow in >>> drm_dp_update_payload_part1(): the 'port' pointer is set to NULL at the >>> same time as 'req_payload.num_slots' is set to zero, but the pointer is >>> dereferenced even when req_payload.num_slot is zero. >>> >>> The problematic dereference was introduced in commit dfda0df34 >>> ("drm/mst: rework payload table allocation to conform better") and may >>> impact all versions since v3.18 >>> >>> The fix suggested by Chris Wilson removes the kernel oops and was found to >>> work well after 10mn of monkey-testing with the second monitor power and >>> input buttons >>> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98990 >>> Fixes: dfda0df34264 ("drm/mst: rework payload table allocation to conform better.") >>> Cc: Dave Airlie <airlied@redhat.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Nathan D Ciobanu <nathan.d.ciobanu@linux.intel.com> >>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >>> Cc: Sean Paul <seanpaul@chromium.org> >>> Cc: <stable@vger.kernel.org> # v3.18+ >>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@linux.intel.com> >>> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >>> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> >> You haz drm-misc commit rights, pls use them :-) > > Sure, I just wanted to have a fresh CI run on this. > >> Since it doesn't have deps, probably simplest to smash into drm-misc-fixes >> and then send a pull req to Dave right away. If you want, you can roll >> -fixes forward to -rc8 while at it. > > Will do. And pushed to drm-misc-fixes, thanks for the patch, review and testing. BR, Jani.
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 122a1b04bebc..f2cc375907d0 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1817,7 +1817,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) mgr->payloads[i].vcpi = req_payload.vcpi; } else if (mgr->payloads[i].num_slots) { mgr->payloads[i].num_slots = 0; - drm_dp_destroy_payload_step1(mgr, port, port->vcpi.vcpi, &mgr->payloads[i]); + drm_dp_destroy_payload_step1(mgr, port, mgr->payloads[i].vcpi, &mgr->payloads[i]); req_payload.payload_state = mgr->payloads[i].payload_state; mgr->payloads[i].start_slot = 0; }