| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 03 Nov 2020 09:34:15 -0800
From: khsieh@...eaurora.org
To: Stephen Boyd <swboyd@...omium.org>
Cc: robdclark@...il.com, sean@...rly.run, tanmay@...eaurora.org,
abhinavk@...eaurora.org, aravindh@...eaurora.org,
rnayak@...eaurora.org, airlied@...ux.ie, daniel@...ll.ch,
linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org,
freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/msm/dp: deinitialize mainlink if link training
failedo
On 2020-11-02 12:59, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2020-10-30 16:22:53)
>> DP compo phy have to be enable to start link training. When
>> link training failed phy need to be disabled so that next
>> link trainng can be proceed smoothly at next plug in. This
>
> s/trainng/training/
>
>> patch de initialize mainlink to disable phy if link training
>
> s/de/de-/
>
>> failed. This prevent system crash due to
>> disp_cc_mdss_dp_link_intf_clk stuck at "off" state. This patch
>> also perform checking power_on flag at dp_display_enable() and
>> dp_display_disable() to avoid crashing when unplug cable while
>> display is off.
>>
>> Fixes: fdaf9a5e3c15 (drm/msm/dp: fixes wrong connection state caused
>> by failure of link train
>>
>
> Drop newline please.
>
>> Signed-off-by: Kuogee Hsieh <khsieh@...eaurora.org>
>> ---
>
> Can you send this as a patch series? There were three patches sent near
> each other and presumably they're related.
>
>> drivers/gpu/drm/msm/dp/dp_ctrl.c | 34
>> +++++++++++++++++++++++++++--
>> drivers/gpu/drm/msm/dp/dp_display.c | 13 +++++++++++
>> 2 files changed, 45 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index cee161c8ecc6..904698dfc7f7 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1468,6 +1468,29 @@ static int dp_ctrl_reinitialize_mainlink(struct
>> dp_ctrl_private *ctrl)
>> return ret;
>> }
>>
>> +static int dp_ctrl_deinitialize_mainlink(struct dp_ctrl_private
>> *ctrl)
>> +{
>> + struct dp_io *dp_io;
>> + struct phy *phy;
>> + int ret = 0;
>
> Please drop this initialization to 0.
>
>> +
>> + dp_io = &ctrl->parser->io;
>> + phy = dp_io->phy;
>> +
>> + dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
>> +
>> + dp_catalog_ctrl_reset(ctrl->catalog);
>> +
>> + ret = dp_power_clk_enable(ctrl->power, DP_CTRL_PM, false);
>
> As it's overwritten here.
>
>> + if (ret)
>> + DRM_ERROR("Failed to disable link clocks. ret=%d\n",
>> ret);
>> +
>> + phy_power_off(phy);
>> + phy_exit(phy);
>> +
>> + return -ECONNRESET;
>
> Isn't this an error for networking connections getting reset? Really it
> should return 0 because it didn't fail.
>
>> +}
>> +
>> static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
>> {
>> int ret = 0;
>> @@ -1648,8 +1671,7 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
>> if (rc)
>> return rc;
>>
>> - while (--link_train_max_retries &&
>> - !atomic_read(&ctrl->dp_ctrl.aborted)) {
>> + while (--link_train_max_retries) {
>> rc = dp_ctrl_reinitialize_mainlink(ctrl);
>> if (rc) {
>> DRM_ERROR("Failed to reinitialize mainlink.
>> rc=%d\n",
>> @@ -1664,6 +1686,9 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
>> break;
>> } else if (training_step == DP_TRAINING_1) {
>> /* link train_1 failed */
>> + if
>> (!dp_catalog_hpd_get_state_status(ctrl->catalog))
>> + break; /* link cable
>> unplugged */
>> +
>> rc = dp_ctrl_link_rate_down_shift(ctrl);
>> if (rc < 0) { /* already in RBR = 1.6G */
>> if (cr.lane_0_1 & DP_LANE0_1_CR_DONE)
>> {
>> @@ -1683,6 +1708,9 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
>> }
>> } else if (training_step == DP_TRAINING_2) {
>> /* link train_2 failed, lower lane rate */
>> + if
>> (!dp_catalog_hpd_get_state_status(ctrl->catalog))
>
> Maybe make a function called dp_catalog_link_disconnected()? Then the
> comment isn't needed.
>
>> + break; /* link cable
>> unplugged */
>> +
>> rc = dp_ctrl_link_lane_down_shift(ctrl);
>> if (rc < 0) {
>> /* end with failure */
>> @@ -1703,6 +1731,8 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
>> */
>> if (rc == 0) /* link train successfully */
>> dp_ctrl_push_idle(dp_ctrl);
>> + else
>> + rc = dp_ctrl_deinitialize_mainlink(ctrl);
>
> So if it fails we deinitialize and then return success? Shouldn't we
> keep the error code from the link train attempt instead of overwrite it
> with (most likely) zero? I see that it returns -ECONNRESET but that's
> really odd and seeing this code here means you have to look at the
> function to figure out that it's still returning an error code. Please
> don't do that, just ignore the error code from this function.
>
There are two possible failure cases at plugin request, link training
failed and read dpcd/edid failed.
It does not need to enable phy/pll to perform aux read/write from/to
dpcd or edid.
on the other hand, phy/pll need to be enabled to perform link training.
If link training failed,
then phy/pll need to be disabled so that phy/pll can be enabled next
link training correctly.
Link training failed error has to be propagated back to the top caller
so that dp_display_host_init()
will be called again at next plugin request.
>>
>> return rc;
>> }
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 3eb0d428abf7..13b66266cd69 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -529,6 +529,11 @@ static int dp_hpd_plug_handle(struct
>> dp_display_private *dp, u32 data)
>> if (ret) { /* link train failed */
>> hpd->hpd_high = 0;
>> dp->hpd_state = ST_DISCONNECTED;
>> +
>> + if (ret == -ECONNRESET) { /* cable unplugged */
>> + dp->core_initialized = false;
>> + }
>
> Style: Drop braces on single line if statements.
>
>> +
>> } else {
>> /* start sentinel checking in case of missing uevent
>> */
>> dp_add_event(dp, EV_CONNECT_PENDING_TIMEOUT, 0, tout);
>> @@ -794,6 +799,11 @@ static int dp_display_enable(struct
>> dp_display_private *dp, u32 data)
>>
>> dp_display = g_dp_display;
>>
>> + if (dp_display->power_on) {
>> + DRM_DEBUG_DP("Link already setup, return\n");
>> + return 0;
>> + }
>> +
>> rc = dp_ctrl_on_stream(dp->ctrl);
>> if (!rc)
>> dp_display->power_on = true;
>> @@ -826,6 +836,9 @@ static int dp_display_disable(struct
>> dp_display_private *dp, u32 data)
>>
>> dp_display = g_dp_display;
>>
>> + if (!dp_display->power_on)
>> + return -EINVAL;
>> +
>> /* wait only if audio was enabled */
>> if (dp_display->audio_enabled) {
>> if (!wait_for_completion_timeout(&dp->audio_comp,
>>
>> base-commit: fd4a29bed29b3d8f15942fdf77e7a0a52796d836
>
> What is this commit?
Powered by blists - more mailing lists