[Intel-gfx] [PATCH] drm/dp/mst: fix kernel oops when turning off secondary monitor

[Pierre-Louis Bossart]

On 12/5/16 3:39 PM, Pandiyan, Dhinakaran wrote:
> On Mon, 2016-12-05 at 08:02 +0000, Chris Wilson wrote:
>> On Sun, Dec 04, 2016 at 07:31:18PM -0600, Pierre-Louis Bossart wrote:
>>> 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.
>>>
>>> Fix by adding test condition to make sure both variables
>>> are used consistently. This removes the kernel oops.
>>>
>>> There are still annoying cases where the primary display goes black
>>> when the secondary display is turned off but it can be recovered from
>>> by playing with the monitor inputs and power buttons.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98990
>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
>>> ---
>>>  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 aa64448..5481fde 100644
>>> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
>>> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>>> @@ -1815,7 +1815,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
>>>  				drm_dp_create_payload_step1(mgr, mgr->proposed_vcpis[i]->vcpi, &req_payload);
>>>  				mgr->payloads[i].num_slots = req_payload.num_slots;
>>>  				mgr->payloads[i].vcpi = req_payload.vcpi;
>>> -			} else if (mgr->payloads[i].num_slots) {
>>> +			} else if (mgr->payloads[i].num_slots && port != NULL) {
>>>  				mgr->payloads[i].num_slots = 0;
>>>  				drm_dp_destroy_payload_step1(mgr, port, port->vcpi.vcpi, &mgr->payloads[i]);
>>
>> s/port->vcpi.vcpi/mgr->payloads[i].vcpi/ here looks to be the correct
>> fix.
>> -Chris
>>
>
> Hmm, not sure if that is the correct fix either. With port = NULL,
> doesn't look like we send drm_dp_payload_send_msg(..., pbn = 0).
> Although, we do update the payload table via DPCD
>
> Also, if port is set to  NULL, I wonder if we are messing up the
> reference counting. Because, this is done below.
> ...
> 	if (port)
> 		drm_dp_put_port(port);
> ...

Chris' suggested fix works better than my initial proposal, I just sent 
the update as a V2. It may not be correct or complete but then someone 
smarter than me needs to take over. I am just an audio guy...