linux-kernel - Re: [PATCH v2] drm/msm/dp: skip checking LINK_STATUS

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <160435038402.884498.10403326257118588679@swboyd.mtv.corp.google.com>
Date:   Mon, 02 Nov 2020 12:53:04 -0800
From:   Stephen Boyd <swboyd@...omium.org>
To:     Kuogee Hsieh <khsieh@...eaurora.org>, robdclark@...il.com,
        sean@...rly.run
Cc:     tanmay@...eaurora.org, abhinavk@...eaurora.org,
        aravindh@...eaurora.org, khsieh@...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 v2] drm/msm/dp: skip checking LINK_STATUS_UPDATED bit

Quoting Kuogee Hsieh (2020-10-30 16:23:10)
> Some dongle will not clear LINK_STATUS_UPDATED bit after
> DPCD read which cause link training failed. This patch

$ git grep 'this patch' -- Documentation/process/submitting-patches.rst

> just read 6 bytes of DPCD link status from sink and return
> without checking LINK_STATUS_UPDATED bit.
> Link rate read back from sink need to be convert into
> really rate by timing 2.7Mb. 

This last sentence doesn't make sense to me, sorry. What is being said?

> For example 0x0A is equivalent
> to 2.7Gb. This patch also convert link rate correctly to fix
> phy compliance test link rate error.
> 
> Chanegs in V2:
> -- revise commit text
> 
> Fixes: fd4a29bed29b (drm/msm/dp: DisplayPort PHY compliance tests fixup)
> 

Shouldn't be any space here between SoB and Fixes tag.

> Signed-off-by: Kuogee Hsieh <khsieh@...eaurora.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_ctrl.c | 20 ++++++--------------
>  drivers/gpu/drm/msm/dp/dp_link.c | 24 +++++++++++-------------
>  2 files changed, 17 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 904698dfc7f7..844ba756a2c6 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1061,23 +1061,15 @@ static bool dp_ctrl_train_pattern_set(struct dp_ctrl_private *ctrl,
>  static int dp_ctrl_read_link_status(struct dp_ctrl_private *ctrl,
>                                     u8 *link_status)
>  {
> -       int len = 0;
> -       u32 const offset = DP_LANE_ALIGN_STATUS_UPDATED - DP_LANE0_1_STATUS;
> -       u32 link_status_read_max_retries = 100;
> -
> -       while (--link_status_read_max_retries) {
> -               len = drm_dp_dpcd_read_link_status(ctrl->aux,
> -                       link_status);
> -               if (len != DP_LINK_STATUS_SIZE) {
> -                       DRM_ERROR("DP link status read failed, err: %d\n", len);
> -                       return len;
> -               }
> +       int ret = 0, len;
>  
> -               if (!(link_status[offset] & DP_LINK_STATUS_UPDATED))
> -                       return 0;
> +       len = drm_dp_dpcd_read_link_status(ctrl->aux, link_status);
> +       if (len != DP_LINK_STATUS_SIZE) {
> +               DRM_ERROR("DP link status read failed, err: %d\n", len);
> +               ret = len;

So if this returns the integer 2 it's OK? Shouldn't it return some error
value?

>         }
>  
> -       return -ETIMEDOUT;
> +       return ret;
>  }
>  
>  static int dp_ctrl_link_train_1(struct dp_ctrl_private *ctrl,
> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
> index 49d7fad36fc4..64a002d100c7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_link.c
> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
> @@ -943,20 +944,17 @@ static u8 get_link_status(const u8 link_status[DP_LINK_STATUS_SIZE], int r)
>   */
>  static int dp_link_process_link_status_update(struct dp_link_private *link)
>  {
> -       if (!(get_link_status(link->link_status,
> -                               DP_LANE_ALIGN_STATUS_UPDATED) &
> -                               DP_LINK_STATUS_UPDATED) ||
> -                       (drm_dp_clock_recovery_ok(link->link_status,
> -                                       link->dp_link.link_params.num_lanes) &&
> -                       drm_dp_channel_eq_ok(link->link_status,
> -                                       link->dp_link.link_params.num_lanes)))
> -               return -EINVAL;
> +       bool channel_eq_done = drm_dp_channel_eq_ok(link->link_status,
> +                       link->dp_link.link_params.num_lanes);
> +
> +       bool clock_recovery_done = drm_dp_clock_recovery_ok(link->link_status,
> +                       link->dp_link.link_params.num_lanes);
>  
>         DRM_DEBUG_DP("channel_eq_done = %d, clock_recovery_done = %d\n",
> -                       drm_dp_clock_recovery_ok(link->link_status,
> -                       link->dp_link.link_params.num_lanes),
> -                       drm_dp_clock_recovery_ok(link->link_status,
> -                       link->dp_link.link_params.num_lanes));
> +                       channel_eq_done, clock_recovery_done);
> +
> +       if (channel_eq_done && clock_recovery_done)
> +               return -EINVAL;
>  
>         return 0;
>  }
> 
> base-commit: 03a9adc88c206b3857ce95f4f4d3b185d429fa31

What is this commit?