linux-kernel - Re: [PATCH v15 1/4] drm/msm/dp: do not initialize phy until plugin interrupt received

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <f5a282ed-0b4a-9fb2-fc04-17842745feaa@quicinc.com>
Date:   Fri, 14 Jan 2022 14:28:52 -0800
From:   Kuogee Hsieh <quic_khsieh@...cinc.com>
To:     Stephen Boyd <swboyd@...omium.org>, <agross@...nel.org>,
        <airlied@...ux.ie>, <bjorn.andersson@...aro.org>,
        <daniel@...ll.ch>, <dmitry.baryshkov@...aro.org>,
        <dri-devel@...ts.freedesktop.org>, <robdclark@...il.com>,
        <sean@...rly.run>, <vkoul@...nel.org>
CC:     <quic_abhinavk@...cinc.com>, <aravindh@...eaurora.org>,
        <quic_sbillaka@...cinc.com>, <freedreno@...ts.freedesktop.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v15 1/4] drm/msm/dp: do not initialize phy until plugin
 interrupt received


On 1/14/2022 1:41 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-01-14 13:11:47)
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 7cc4d21..7cd6222 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -696,12 +699,9 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)
>>           * 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 */
>> -                       dp->core_initialized = false;
>> -               }
>> -       }
>> +       if (dp->core_initialized)
> When is this condition false? The irq isn't unmasked until the core has
> been initialized. On the resume path I suppose the irq is enabled in
> dp_display_host_init() calling dp_ctrl_reset_irq_ctrl(), and then we
> could immediately get the interrupt but it will block on the event_mutex
> lock.

This is left over form Lazor.

I remember that there is an extreme case that several irq_hpd interrupts 
happen right after dongle plug in  (happen at resume too) and sometime 
cause system crash at dpcd read due to AHB clock is not enabled yet. It 
took some time to debug it.

 From looking into code, it does not look likely it will happen. But it 
did happen at real world.

So that I would like to keep this condition checking.


>> +               dp_display_usbpd_attention_cb(&dp->pdev->dev);
>> +
>>          DRM_DEBUG_DP("hpd_state=%d\n", state);
>>
>>          mutex_unlock(&dp->event_mutex);
>> @@ -1363,14 +1373,16 @@ static int dp_pm_suspend(struct device *dev)
>>                  if (dp_power_clk_status(dp->power, DP_CTRL_PM))
>>                          dp_ctrl_off_link_stream(dp->ctrl);
>>
>> +               dp_display_host_phy_exit(dp);
>> +
>> +               /* host_init will be called at pm_resume */
>>                  dp_display_host_deinit(dp);
>> +       } else {
>> +               dp_display_host_phy_exit(dp);
> I fail to see where this condition happens. Can we suspend the device
> without the irq being installed?

Agree, with this new mechanism it should not happen.

Will remove it.

>>          }
>>
>>          dp->hpd_state = ST_SUSPENDED;
>>
>> -       /* host_init will be called at pm_resume */
>> -       dp->core_initialized = false;
>> -
>>          DRM_DEBUG_DP("After, core_inited=%d power_on=%d\n",
>>                          dp->core_initialized, dp_display->power_on);
>>