linux-kernel - Re: [PATCH] drm/msm/dp: check core_initialized flag at both host_init() and host

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: <846439be-77db-7311-3bb3-3647dc6d0622@quicinc.com>
Date:   Mon, 27 Feb 2023 12:05:53 -0800
From:   Abhinav Kumar <quic_abhinavk@...cinc.com>
To:     Stephen Boyd <swboyd@...omium.org>,
        Kuogee Hsieh <quic_khsieh@...cinc.com>, <agross@...nel.org>,
        <airlied@...il.com>, <andersson@...nel.org>, <daniel@...ll.ch>,
        <dianders@...omium.org>, <dmitry.baryshkov@...aro.org>,
        <robdclark@...il.com>, <sean@...rly.run>, <vkoul@...nel.org>
CC:     <quic_sbillaka@...cinc.com>, <linux-arm-msm@...r.kernel.org>,
        <dri-devel@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
        <freedreno@...ts.freedesktop.org>
Subject: Re: [PATCH] drm/msm/dp: check core_initialized flag at both
 host_init() and host_deinit()

Hi Stephen

On 2/27/2023 11:53 AM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2023-02-24 10:29:58)
>> There is a reboot/suspend test case where system suspend is forced during
>> system booting up. Since host_init() of external DP is executed at hpd
> 
> dp_display_host_init()?
> 
>> thread context, this test case may created a scenario that host_deinit()
> 
> dp_display_host_deinit()?
> 
ack for both.
>> from pm_suspend() run before host_init() if hpd thread has no chance to
>> run during booting up while suspend request command was issued.
>> At this scenario system will crash at aux register access at host_deinit()
>> since aux clock had not yet been enabled by host_init().  Therefore we
> 
> The aux clk is enabled in dp_power_clk_enable() right? Can you clarify?
> 

Yes, thats right. Its mapped to core_*** in the dts hence the name goes 
to the DP_CORE_PM case in the dp_power_clk_enable()

3092 				clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
3093 					 <&dispcc DISP_CC_MDSS_DP_AUX_CLK>,
3094 					 <&dispcc DISP_CC_MDSS_DP_LINK_CLK>,
3095 					 <&dispcc DISP_CC_MDSS_DP_LINK_INTF_CLK>,
3096 					 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>;
3097 				clock-names = "core_iface", "core_aux"

>> have to ensure aux clock enabled by checking core_initialized flag before
>> access aux registers at pm_suspend.
> 
> I'd much more like to get rid of 'core_initialized'. What is preventing
> us from enabling the power (i.e. dp_power_init()), or at least enough
> clks and pm runtime state, during probe? That would fix this problem and
> also clean things up. As I understand, the device is half initialized in
> probe and half initialized in the kthread. If we put all power
> management into the runtime PM ops and synced that state during probe so
> that runtime PM state matched device probe state we could make runtime
> PM be the only suspend function and then push the power state tracking
> into the device core.
> 

You are correct. https://patchwork.freedesktop.org/patch/523879/ will be 
doing that. Its still in review.

>>
>> Fixes: 989ebe7bc446 ("drm/msm/dp: do not initialize phy until plugin interrupt received")
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@...cinc.com>
> 
> The code looks OK to me, so
> 
> Reviewed-by: Stephen Boyd <swboyd@...omium.org>
> 
> once the commit text is cleaned up to indicate the proper function
> names.

Thanks , will fix all those.