[PATCH 001/001] drivers/gpu/radeon: NULL pointer deference workaround
Mark Fortescue
mark at thurning-instruments.co.uk
Thu Sep 8 11:18:45 UTC 2016
On 08/09/16 11:25, Christian König wrote:
> Am 08.09.2016 um 11:09 schrieb Mark Fortescue:
>> Hi Christian,
>>
>> Thank you for the feedback.
>>
>> On 08/09/16 08:14, Christian König wrote:
>>> Am 07.09.2016 um 19:38 schrieb Mark Fortescue:
>>>>
>>>> On an LV-683 (AMD Dual-core G-T56N) Mini-ITX board, I get a Kernel
>>>> Oops because Connector 0 (LCD Panel interface) does not have DDC.
>>>
>>> I'm not an expert on this, but that is really odd cause even LCD Panels
>>> should have a DDC interface.
>>>
>>>>
>>>> Ubuntu 16.04 LTS Kernel (4.4 series):
>>>>
>>>> ...
>>>> [ 8.262990] [drm] ib test on ring 5 succeeded
>>>> [ 8.288897] [drm] Radeon Display Connectors
>>>> [ 8.293175] [drm] Connector 0:
>>>> [ 8.296252] [drm] DP-1
>>>
>>> Especially since the BIOS claims that this is a displayport connector
>>> and there is no physical way to have a DP without an DDC as far as I
>>> know.
>>>
>>> Please open a bug report on FDO and attach you BIOS image.
>>
>> FDO ? I am not familiar with this. Please can you enlighten me.
>>
>
> See here: http://bugs.freedesktop.org/
>
>> I do not have a BIOS image so will need some assistance in
>> understanding what is required here and how I extract the BIOS
>> information you are after.
>>
>
> Sorry my fault. Mullins is an APU, so you don't have a dedicated video
> BIOS. As usually I didn't got enough sleep :) But please open up a bug
> report anyway.
Know the problem of being awake too long :). I will raise a bug.
>
>> On this motherboard, DP-1 is a single channel 18bit LVDS LCD panel
>> interface and DP-2 is a DVI interface (which I can connect to my
>> monitor if testing this is useful). There are no displayport connectors.
>
> Yeah, but from the driver point of view there are only DP connectors on
> the chip. The LVDS and DVI are probably realized with external DP to
> whatever converter ICs.
>
That would explain why some similar boards have 24bit LVDS and others
only 18bit LVDS.
>>
>> On industrial motherboards, I have noticed that it is not uncommon to
>> hard code the information for the LCD panel into the BIOS so no DDC is
>> required. In this case, there is no LCD panel connected to the
>> interface anyway.
>>
>
> That is correct and as far as I know well supported by Radeon, but the
> crux is there should still be a DDC channel even if there isn't anything
> attached to it.
>
> See with displayport you got four LVDS channels to submit the actual
> picture and an AUX channel to configure and query the device. The DDC is
> just represented as certain packets over the AUX channel.
>
> If the AUX channel doesn't work or isn't connect then the link training
> wouldn't be possible as well and so you wouldn't be able to get any
> picture on the LVDS.
>
Interesting.
>> See http://www.commell.com.tw/product/SBC/LV-683.HTM for more
>> information on the board. Looking at the web site, there is a BIOS
>> image available form Commell if that is of use.
>
> Alex clearly needs to take a look on this. I think for the time being
> you could hack together a patch which just ignores DP connectors during
> probing if they don't have an associated DDC instead of changing the
> code everywhere the DDC object is required.
>
I will try to find the time to look at this in more detail. GPU/DRM is
not something I have done any real work on in the past so I do not know
how it all fits together. The overkill on the code changes is the
result, as that way I don't need to understand where and why the helper
function (radeon_dp_getsinktype) gets called or what other code paths
can be executed that could fail.
I will wait for Alex to have a think about it and respond before I do
any more - that way I can finish the work I am paid to do first :).
> Regards,
> Christian.
>
Regards
Mark.
>>
>>>
>>> Alex can probably take a look when he's back from vacation.
>>>
>>> Regards,
>>> Christian.
>>>
>>>> [ 8.298791] [drm] Encoders:
>>>> [ 8.301770] [drm] DFP1: INTERNAL_UNIPHY
>>>> [ 8.305973] [drm] Connector 1:
>>>> [ 8.309043] [drm] DP-2
>>>> [ 8.311598] [drm] HPD2
>>>> [ 8.314169] [drm] DDC: 0x6440 0x6440 0x6444 0x6444 0x6448 0x6448
>>>> 0x644c 0x644c
>>>> [ 8.321609] [drm] Encoders:
>>>> [ 8.324589] [drm] DFP2: INTERNAL_UNIPHY
>>>> [ 8.328793] [drm] Connector 2:
>>>> [ 8.331856] [drm] VGA-1
>>>> [ 8.342947] [drm] DDC: 0x64d8 0x64d8 0x64dc 0x64dc 0x64e0 0x64e0
>>>> 0x64e4 0x64e4
>>>> [ 8.350341] [drm] Encoders:
>>>> [ 8.353310] [drm] CRT1: INTERNAL_KLDSCP_DAC1
>>>> [ 8.358195] BUG: unable to handle kernel NULL pointer dereference at
>>>> 0000000000000409
>>>> [ 8.409733] [<ffffffffc02024da>] radeon_dp_getsinktype+0x1a/0x30
>>>> [radeon]
>>>> [ 8.416805] PGD 0
>>>> [ 8.418841] Oops: 0000 [#1] SMP
>>>> ...
>>>>
>>>> This patch prevents Kernel failures due to a connector not having a
>>>> DDC interface by changing the code so that ddc_bus is always checked
>>>> before use.
>>>> The problem was first identified using the uBuntu MATE 14.04 LTS (3.16
>>>> series kernels) but not dealt with at that time. On attempting to
>>>> install uBuntu MATE 16.04 LTS (4.4 series kernels), it became clear
>>>> that using various workarounds to allow the issue to be ignored were
>>>> not viable so more effort was put in to sorting the issue resulting in
>>>> this patch. See https://bugs.launchpad.net/bugs/1587885 for more
>>>> details.
>>>>
>>>> Signed-off-by: Mark Fortescue <mark at thurning-instruments.co.uk>
>>>> Tested-by: Mark Fortescue <mark at thurning-instruments.co.uk>
>>>>
>>>> ---
>>>>
>>>> Looks like Thunderbird may have made a mess of the patch when pasting
>>>> the contents into the mail message - my alternate mail client (pine)
>>>> also has strict line length handling and trashes non-MIME encoded
>>>> patches.
>>>>
>>>> This may not be the correct approach to solving the issue but it is
>>>> clean in that it ensures that ddc_bus is never used when NULL
>>>> regardless of how the code ended up at the point of use.
>>>>
>>>> If it helps with back porting, I have patches for the uBuntu 14.04 LTS
>>>> [3.13 series], uBuntu MATE 14.04 LTS [3.16 series] and uBuntu 16.04
>>>> LTS [4.4 series] kernels.
>>>>
>>>> Test Hardware:
>>>> Commell LV-683 Mini-ITX with onboard AMD Dual-core G-T56N
>>>> 4G Ram, 2x1TB Disk, HANNS-G HC194D 1280x1024 LCD (VGA).
>>>> 4.8.0-rc5 with patch boots without error.
>>>>
>>>> drivers/gpu/drm/radeon/atombios_dp.c | 60 ++++++++++++-------
>>>> drivers/gpu/drm/radeon/radeon_connectors.c | 46 +++++++-------
>>>> drivers/gpu/drm/radeon/radeon_dp_mst.c | 9 ++
>>>> drivers/gpu/drm/radeon/radeon_i2c.c | 3
>>>> 4 files changed, 73 insertions(+), 45 deletions(-)
>>>>
>>>> Patch:
>>>> diff --git a/drivers/gpu/drm/radeon/atombios_dp.c
>>>> b/drivers/gpu/drm/radeon/atombios_dp.c
>>>> index cead089a..98b3c0e 100644
>>>> --- a/drivers/gpu/drm/radeon/atombios_dp.c
>>>> +++ b/drivers/gpu/drm/radeon/atombios_dp.c
>>>> @@ -232,6 +232,9 @@ void radeon_dp_aux_init(struct radeon_connector
>>>> *radeon_connector)
>>>> struct radeon_device *rdev = dev->dev_private;
>>>> int ret;
>>>>
>>>> + if (!radeon_connector->ddc_bus)
>>>> + return;
>>>> +
>>>> radeon_connector->ddc_bus->rec.hpd = radeon_connector->hpd.hpd;
>>>> radeon_connector->ddc_bus->aux.dev = radeon_connector->base.kdev;
>>>> if (ASIC_IS_DCE5(rdev)) {
>>>> @@ -364,6 +367,9 @@ u8 radeon_dp_getsinktype(struct radeon_connector
>>>> *radeon_connector)
>>>> struct drm_device *dev = radeon_connector->base.dev;
>>>> struct radeon_device *rdev = dev->dev_private;
>>>>
>>>> + if (!radeon_connector->ddc_bus)
>>>> + return 0;
>>>> +
>>>> return radeon_dp_encoder_service(rdev,
>>>> ATOM_DP_ACTION_GET_SINK_TYPE, 0,
>>>> radeon_connector->ddc_bus->rec.i2c_id, 0);
>>>> }
>>>> @@ -376,6 +382,9 @@ static void radeon_dp_probe_oui(struct
>>>> radeon_connector *radeon_connector)
>>>> if (!(dig_connector->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
>>>> DP_OUI_SUPPORT))
>>>> return;
>>>>
>>>> + if (!radeon_connector->ddc_bus)
>>>> + return;
>>>> +
>>>> if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
>>>> DP_SINK_OUI, buf, 3) == 3)
>>>> DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
>>>> buf[0], buf[1], buf[2]);
>>>> @@ -391,6 +400,9 @@ bool radeon_dp_getdpcd(struct radeon_connector
>>>> *radeon_connector)
>>>> u8 msg[DP_DPCD_SIZE];
>>>> int ret, i;
>>>>
>>>> + if (!radeon_connector->ddc_bus)
>>>> + return false;
>>>> +
>>>> for (i = 0; i < 7; i++) {
>>>> ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
>>>> DP_DPCD_REV, msg,
>>>> DP_DPCD_SIZE);
>>>> @@ -428,24 +440,26 @@ int radeon_dp_get_panel_mode(struct drm_encoder
>>>> *encoder,
>>>>
>>>> dig_connector = radeon_connector->con_priv;
>>>>
>>>> - if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
>>>> - /* DP bridge chips */
>>>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>> - DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>>> - if (tmp & 1)
>>>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>>> - else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
>>>> - (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
>>>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
>>>> - else
>>>> - panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
>>>> - }
>>>> - } else if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
>>>> - /* eDP */
>>>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>> - DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>>> - if (tmp & 1)
>>>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>>> + if (radeon_connector->ddc_bus) {
>>>> + if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
>>>> + /* DP bridge chips */
>>>> + if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>> + DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>>> + if (tmp & 1)
>>>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>>> + else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
>>>> + (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
>>>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
>>>> + else
>>>> + panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
>>>> + }
>>>> + } else if (connector->connector_type ==
>>>> DRM_MODE_CONNECTOR_eDP) {
>>>> + /* eDP */
>>>> + if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>> + DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>>> + if (tmp & 1)
>>>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>>> + }
>>>> }
>>>> }
>>>>
>>>> @@ -511,6 +525,9 @@ bool radeon_dp_needs_link_train(struct
>>>> radeon_connector *radeon_connector)
>>>> u8 link_status[DP_LINK_STATUS_SIZE];
>>>> struct radeon_connector_atom_dig *dig =
>>>> radeon_connector->con_priv;
>>>>
>>>> + if (!radeon_connector->ddc_bus)
>>>> + return false;
>>>> +
>>>> if
>>>> (drm_dp_dpcd_read_link_status(&radeon_connector->ddc_bus->aux,
>>>> link_status)
>>>> <= 0)
>>>> return false;
>>>> @@ -531,7 +548,7 @@ void radeon_dp_set_rx_power_state(struct
>>>> drm_connector *connector,
>>>> dig_connector = radeon_connector->con_priv;
>>>>
>>>> /* power up/down the sink */
>>>> - if (dig_connector->dpcd[0] >= 0x11) {
>>>> + if (radeon_connector->ddc_bus && dig_connector->dpcd[0] >= 0x11) {
>>>> drm_dp_dpcd_writeb(&radeon_connector->ddc_bus->aux,
>>>> DP_SET_POWER, power_state);
>>>> usleep_range(1000, 2000);
>>>> @@ -834,7 +851,8 @@ void radeon_dp_link_train(struct drm_encoder
>>>> *encoder,
>>>> else
>>>> dp_info.enc_id |= ATOM_DP_CONFIG_LINK_A;
>>>>
>>>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>> DP_MAX_LANE_COUNT, &tmp)
>>>> + if (radeon_connector->ddc_bus &&
>>>> + drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>> DP_MAX_LANE_COUNT, &tmp)
>>>> == 1) {
>>>> if (ASIC_IS_DCE5(rdev) && (tmp & DP_TPS3_SUPPORTED))
>>>> dp_info.tp3_supported = true;
>>>> @@ -850,7 +868,7 @@ void radeon_dp_link_train(struct drm_encoder
>>>> *encoder,
>>>> dp_info.connector = connector;
>>>> dp_info.dp_lane_count = dig_connector->dp_lane_count;
>>>> dp_info.dp_clock = dig_connector->dp_clock;
>>>> - dp_info.aux = &radeon_connector->ddc_bus->aux;
>>>> + dp_info.aux = radeon_connector->ddc_bus ?
>>>> &radeon_connector->ddc_bus->aux : 0;
>>>>
>>>> if (radeon_dp_link_train_init(&dp_info))
>>>> goto done;
>>>> diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c
>>>> b/drivers/gpu/drm/radeon/radeon_connectors.c
>>>> index b79f3b0..cec30c9 100644
>>>> --- a/drivers/gpu/drm/radeon/radeon_connectors.c
>>>> +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
>>>> @@ -328,31 +328,32 @@ static void radeon_connector_get_edid(struct
>>>> drm_connector *connector)
>>>> if (radeon_connector->router.ddc_valid)
>>>> radeon_router_select_ddc_port(radeon_connector);
>>>>
>>>> - if
>>>> ((radeon_connector_encoder_get_dp_bridge_encoder_id(connector) !=
>>>> - ENCODER_OBJECT_ID_NONE) &&
>>>> - radeon_connector->ddc_bus->has_aux) {
>>>> - radeon_connector->edid = drm_get_edid(connector,
>>>> - &radeon_connector->ddc_bus->aux.ddc);
>>>> - } else if ((connector->connector_type ==
>>>> DRM_MODE_CONNECTOR_DisplayPort) ||
>>>> - (connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
>>>> - struct radeon_connector_atom_dig *dig =
>>>> radeon_connector->con_priv;
>>>> -
>>>> - if ((dig->dp_sink_type == CONNECTOR_OBJECT_ID_DISPLAYPORT ||
>>>> - dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
>>>> - radeon_connector->ddc_bus->has_aux)
>>>> - radeon_connector->edid =
>>>> drm_get_edid(&radeon_connector->base,
>>>> + if (radeon_connector->ddc_bus) {
>>>> + if
>>>> ((radeon_connector_encoder_get_dp_bridge_encoder_id(connector) !=
>>>> + ENCODER_OBJECT_ID_NONE) &&
>>>> + radeon_connector->ddc_bus->has_aux) {
>>>> + radeon_connector->edid = drm_get_edid(connector,
>>>> &radeon_connector->ddc_bus->aux.ddc);
>>>> - else if (radeon_connector->ddc_bus)
>>>> + } else if ((connector->connector_type ==
>>>> DRM_MODE_CONNECTOR_DisplayPort) ||
>>>> + (connector->connector_type ==
>>>> DRM_MODE_CONNECTOR_eDP)) {
>>>> + struct radeon_connector_atom_dig *dig =
>>>> radeon_connector->con_priv;
>>>> +
>>>> + if ((dig->dp_sink_type ==
>>>> CONNECTOR_OBJECT_ID_DISPLAYPORT ||
>>>> + dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
>>>> + radeon_connector->ddc_bus->has_aux)
>>>> + radeon_connector->edid =
>>>> drm_get_edid(&radeon_connector->base,
>>>> + &radeon_connector->ddc_bus->aux.ddc);
>>>> + else
>>>> + radeon_connector->edid =
>>>> drm_get_edid(&radeon_connector->base,
>>>> + &radeon_connector->ddc_bus->adapter);
>>>> + } else if (vga_switcheroo_handler_flags() &
>>>> VGA_SWITCHEROO_CAN_SWITCH_DDC &&
>>>> + connector->connector_type == DRM_MODE_CONNECTOR_LVDS) {
>>>> + radeon_connector->edid =
>>>> drm_get_edid_switcheroo(&radeon_connector->base,
>>>> + &radeon_connector->ddc_bus->adapter);
>>>> + } else {
>>>> radeon_connector->edid =
>>>> drm_get_edid(&radeon_connector->base,
>>>> &radeon_connector->ddc_bus->adapter);
>>>> - } else if (vga_switcheroo_handler_flags() &
>>>> VGA_SWITCHEROO_CAN_SWITCH_DDC &&
>>>> - connector->connector_type == DRM_MODE_CONNECTOR_LVDS &&
>>>> - radeon_connector->ddc_bus) {
>>>> - radeon_connector->edid =
>>>> drm_get_edid_switcheroo(&radeon_connector->base,
>>>> - &radeon_connector->ddc_bus->adapter);
>>>> - } else if (radeon_connector->ddc_bus) {
>>>> - radeon_connector->edid = drm_get_edid(&radeon_connector->base,
>>>> - &radeon_connector->ddc_bus->adapter);
>>>> + }
>>>> }
>>>>
>>>> if (!radeon_connector->edid) {
>>>> @@ -1312,6 +1313,7 @@ radeon_dvi_detect(struct drm_connector
>>>> *connector, bool force)
>>>> continue;
>>>> list_radeon_connector =
>>>> to_radeon_connector(list_connector);
>>>> if (list_radeon_connector->shared_ddc &&
>>>> + radeon_connector->ddc_bus &&
>>>> (list_radeon_connector->ddc_bus->rec.i2c_id ==
>>>> radeon_connector->ddc_bus->rec.i2c_id)) {
>>>> /* cases where both connectors are digital */
>>>> diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>>> b/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>>> index de504ea..89e91f3 100644
>>>> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>>> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>>> @@ -661,7 +661,7 @@ radeon_dp_mst_init(struct radeon_connector
>>>> *radeon_connector)
>>>> {
>>>> struct drm_device *dev = radeon_connector->base.dev;
>>>>
>>>> - if (!radeon_connector->ddc_bus->has_aux)
>>>> + if (!radeon_connector->ddc_bus ||
>>>> !radeon_connector->ddc_bus->has_aux)
>>>> return 0;
>>>>
>>>> radeon_connector->mst_mgr.cbs = &mst_cbs;
>>>> @@ -688,6 +688,9 @@ radeon_dp_mst_probe(struct radeon_connector
>>>> *radeon_connector)
>>>> if (dig_connector->dpcd[DP_DPCD_REV] < 0x12)
>>>> return 0;
>>>>
>>>> + if (!radeon_connector->ddc_bus ||
>>>> !radeon_connector->ddc_bus->has_aux)
>>>> + return 0;
>>>> +
>>>> ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
>>>> DP_MSTM_CAP, msg,
>>>> 1);
>>>> if (ret) {
>>>> @@ -711,7 +714,9 @@ radeon_dp_mst_check_status(struct radeon_connector
>>>> *radeon_connector)
>>>> struct radeon_connector_atom_dig *dig_connector =
>>>> radeon_connector->con_priv;
>>>> int retry;
>>>>
>>>> - if (dig_connector->is_mst) {
>>>> + if (dig_connector->is_mst &&
>>>> + radeon_connector->ddc_bus &&
>>>> + radeon_connector->ddc_bus->has_aux) {
>>>> u8 esi[16] = { 0 };
>>>> int dret;
>>>> int ret = 0;
>>>> diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c
>>>> b/drivers/gpu/drm/radeon/radeon_i2c.c
>>>> index 9590bcd..c18f7ba 100644
>>>> --- a/drivers/gpu/drm/radeon/radeon_i2c.c
>>>> +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
>>>> @@ -63,6 +63,9 @@ bool radeon_ddc_probe(struct radeon_connector
>>>> *radeon_connector, bool use_aux)
>>>> if (radeon_connector->router.ddc_valid)
>>>> radeon_router_select_ddc_port(radeon_connector);
>>>>
>>>> + if (!radeon_connector->ddc_bus)
>>>> + return false;
>>>> +
>>>> if (use_aux) {
>>>> ret = i2c_transfer(&radeon_connector->ddc_bus->aux.ddc,
>>>> msgs, 2);
>>>> } else {
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
More information about the dri-devel
mailing list