| 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
| ||
|
Message-ID: <4da92917bb65490f500faf7bf9b7003f@codeaurora.org>
Date: Wed, 21 Apr 2021 10:43:42 -0700
From: khsieh@...eaurora.org
To: Stephen Boyd <swboyd@...omium.org>
Cc: robdclark@...il.com, sean@...rly.run, abhinavk@...eaurora.org,
aravindh@...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 v3 3/3] drm/msm/dp: check main link status before start
aux read
On 2021-04-20 16:38, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2021-04-16 10:38:51)
>> Maybe when the cable is disconnected the DP phy should be shutdown and
>> some bit in the phy could effectively "cut off" the aux channel and
>> then
>> NAKs would start coming through here in the DP controller I/O register
>> space. This patch have DP aux channel read/write to return NAK
>> immediately
>> if DP controller connection status is in unplugged state.
>>
>> Changes in V3:
>> -- check core_initialized before handle irq_hpd
>> Signed-off-by: Kuogee Hsieh <khsieh@...eaurora.org>
>> ---
>> drivers/gpu/drm/msm/dp/dp_aux.c | 5 +++++
>> drivers/gpu/drm/msm/dp/dp_display.c | 14 ++++++++++----
>> drivers/gpu/drm/msm/dp/dp_link.c | 20 +++++++++++++++-----
>> 3 files changed, 30 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>> index 7c22bfe..fae3806 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>> @@ -343,6 +343,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
>> *dp_aux,
>>
>> mutex_lock(&aux->mutex);
>>
>> + if (!dp_catalog_link_is_connected(aux->catalog)) {
>> + ret = -ETIMEDOUT;
>> + goto unlock_exit;
>> + }
>> +
>
> This still makes me concerned. Any possibility to not do this and have
> the phy cut the connection off and have this transfer timeout
> immediately?
no, we have to wait hardware AUX_NACK timeout.
only this or the abort flag used last time.
Last time you have kernel crash because of service irq_hpd while clock
is turned off.
I have add core_initialized checking wiinin irq_hpd to prevent this.
I think abort flag approach is safer.
>
>> aux->native = msg->request & (DP_AUX_NATIVE_WRITE &
>> DP_AUX_NATIVE_READ);
>>
>> /* Ignore address only message */
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 1784e11..db3f45e 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -571,7 +571,7 @@ static int dp_hpd_plug_handle(struct
>> dp_display_private *dp, u32 data)
>> dp->hpd_state = ST_DISCONNECTED;
>>
>> if (ret == -ECONNRESET) { /* cable unplugged */
>> - dp->core_initialized = false;
>> + DRM_ERROR("dongle unplugged = %d\n", ret);
>
> Is this a debug message?
>
>> }
>>
>> } else {
>> @@ -711,9 +711,15 @@ static int dp_irq_hpd_handle(struct
>> dp_display_private *dp, u32 data)
>> return 0;
>> }
>>
>> - ret = dp_display_usbpd_attention_cb(&dp->pdev->dev);
>> - if (ret == -ECONNRESET) { /* cable unplugged */
>> - dp->core_initialized = false;
>> + /*
>> + * dp core (ahb/aux clks) must be initialized before
>> + * irq_hpd be handled
>> + */
>> + if (dp->core_initialized) {
>> + ret = dp_display_usbpd_attention_cb(&dp->pdev->dev);
>> + if (ret == -ECONNRESET) { /* cable unplugged */
>> + DRM_ERROR("dongle unplugged = %d\n", ret);
>
> Another debug message?
>
>> + }
>> }
>>
>> mutex_unlock(&dp->event_mutex);
>> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c
>> b/drivers/gpu/drm/msm/dp/dp_link.c
>> index be986da..53ecae6 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_link.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
>> @@ -737,18 +737,25 @@ static int dp_link_parse_sink_count(struct
>> dp_link *dp_link)
>> return 0;
>> }
>>
>> -static void dp_link_parse_sink_status_field(struct dp_link_private
>> *link)
>> +static int dp_link_parse_sink_status_field(struct dp_link_private
>> *link)
>> {
>> int len = 0;
>>
>> link->prev_sink_count = link->dp_link.sink_count;
>> - dp_link_parse_sink_count(&link->dp_link);
>> + len = dp_link_parse_sink_count(&link->dp_link);
>> + if (len < 0) {
>> + DRM_ERROR("DP parse sink count failed\n");
>> + return len;
>> + }
>>
>> len = drm_dp_dpcd_read_link_status(link->aux,
>> link->link_status);
>> - if (len < DP_LINK_STATUS_SIZE)
>> + if (len < DP_LINK_STATUS_SIZE) {
>> DRM_ERROR("DP link status read failed\n");
>> - dp_link_parse_request(link);
>> + return len;
>> + }
>> +
>> + return dp_link_parse_request(link);
>> }
>>
>> /**
>> @@ -1032,7 +1039,10 @@ int dp_link_process_request(struct dp_link
>> *dp_link)
>>
>> dp_link_reset_data(link);
>>
>> - dp_link_parse_sink_status_field(link);
>> + ret = dp_link_parse_sink_status_field(link);
>> + if (ret) {
>> + return ret;
>> + }
>>
>> if (link->request.test_requested == DP_TEST_LINK_EDID_READ) {
>> dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
>> --
>
> Can you split this part off into another patch? It seems to stand on
> its
> own as it makes the code more robust to transfer errors in the sink
> parsing code.
Powered by blists - more mailing lists