| 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: <c5503020-0224-428a-9452-f8d668b47caf@quicinc.com>
Date: Mon, 10 Feb 2025 14:17:48 -0800
From: Abhinav Kumar <quic_abhinavk@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Marijn Suijten
<marijn.suijten@...ainline.org>
CC: Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>,
David Airlie
<airlied@...il.com>, Vinod Koul <vkoul@...nel.org>,
Simona Vetter
<simona@...ll.ch>, <linux-arm-msm@...r.kernel.org>,
<dri-devel@...ts.freedesktop.org>, <freedreno@...ts.freedesktop.org>,
<linux-kernel@...r.kernel.org>,
Jordan Crouse <jordan@...micpenguin.net>,
<~postmarketos/upstreaming@...ts.sr.ht>,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>,
Martin Botka
<martin.botka@...ainline.org>,
Jami Kettunen <jami.kettunen@...ainline.org>,
Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Subject: Re: [PATCH v2 2/3] drm/msm/dsi: Set PHY usescase (and mode) before
registering DSI host
On 2/10/2025 6:24 AM, Dmitry Baryshkov wrote:
> On Mon, Feb 10, 2025 at 01:54:29PM +0100, Marijn Suijten wrote:
>> On 2025-02-10 01:11:59, Dmitry Baryshkov wrote:
>>> On Sun, Feb 09, 2025 at 10:42:53PM +0100, Marijn Suijten wrote:
>>>> Ordering issues here cause an uninitialized (default STANDALONE)
>>>> usecase to be programmed (which appears to be a MUX) in some cases
>>>> when msm_dsi_host_register() is called, leading to the slave PLL in
>>>> bonded-DSI mode to source from a clock parent (dsi1vco) that is off.
>>>>
>>>> This should seemingly not be a problem as the actual dispcc clocks from
>>>> DSI1 that are muxed in the clock tree of DSI0 are way further down, this
>>>> bit still seems to have an effect on them somehow and causes the right
>>>> side of the panel controlled by DSI1 to not function.
>>>>
>>>> In an ideal world this code is refactored to no longer have such
>>>> error-prone calls "across subsystems", and instead model the "PLL src"
>>>> register field as a regular mux so that changing the clock parents
>>>> programmatically or in DTS via `assigned-clock-parents` has the
>>>> desired effect.
>>>> But for the avid reader, the clocks that we *are* muxing into DSI0's
>>>> tree are way further down, so if this bit turns out to be a simple mux
>>>> between dsiXvco and out_div, that shouldn't have any effect as this
>>>> whole tree is off anyway.
>>>>
>>>> Signed-off-by: Marijn Suijten <marijn.suijten@...ainline.org>
>>>> ---
>>>> drivers/gpu/drm/msm/dsi/dsi_manager.c | 30 +++++++++++++++++++-----------
>>>> 1 file changed, 19 insertions(+), 11 deletions(-)
>>>
>>>
>>> Fixes: 57bf43389337 ("drm/msm/dsi: Pass down use case to PHY")
>>
>> I'm not exactly confident about that. Abhinav pointed out in
>> https://gitlab.freedesktop.org/drm/msm/-/issues/41#note_2375646 that
>> msm_dsi_host_register() was not supposed to be enabling the PHY, which I
>> provided a counter-stacktrace for to show that is indeed the case.
>>
>> Either this was always a problem that's only become visible now (and it's an
>> issue with that patch), or a different change causes msm_dsi_host_register()
>> to enable the PHY and program the usecase too early?
>
> As currently usecase is being programmed after the DSI host being
> registered, there might be a race condition between panel driver probe
> _and_ usecase programming.
>
>>
>> What do you think?
>>
>> - Marijn
>>
Yes I agree with Dmitry's explanation. The race condition between the
two can cause this. Hence I am also fine with this change.
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
>>>
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>> index a210b7c9e5ca281a46fbdb226e25832719a684ea..b93205c034e4acc73d536deeddce6ebd694b4a80 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>> @@ -74,17 +74,33 @@ static int dsi_mgr_setup_components(int id)
>>>> int ret;
>>>>
>>>> if (!IS_BONDED_DSI()) {
>>>> + /* Set the usecase before calling msm_dsi_host_register(), which would
>>>> + * already program the PLL source mux based on a default usecase.
>>>> + */
>>>> + msm_dsi_phy_set_usecase(msm_dsi->phy, MSM_DSI_PHY_STANDALONE);
>>>> + msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
>>>> +
>>>> ret = msm_dsi_host_register(msm_dsi->host);
>>>> if (ret)
>>>> return ret;
>>>> -
>>>> - msm_dsi_phy_set_usecase(msm_dsi->phy, MSM_DSI_PHY_STANDALONE);
>>>> - msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
>>>> } else if (other_dsi) {
>>>> struct msm_dsi *master_link_dsi = IS_MASTER_DSI_LINK(id) ?
>>>> msm_dsi : other_dsi;
>>>> struct msm_dsi *slave_link_dsi = IS_MASTER_DSI_LINK(id) ?
>>>> other_dsi : msm_dsi;
>>>> +
>>>> + /* PLL0 is to drive both DSI link clocks in bonded DSI mode.
>>>> + *
>>>> + /* Set the usecase before calling msm_dsi_host_register(), which would
>>>> + * already program the PLL source mux based on a default usecase.
>>>> + */
>>>> + msm_dsi_phy_set_usecase(clk_master_dsi->phy,
>>>> + MSM_DSI_PHY_MASTER);
>>>> + msm_dsi_phy_set_usecase(clk_slave_dsi->phy,
>>>> + MSM_DSI_PHY_SLAVE);
>>>> + msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
>>>> + msm_dsi_host_set_phy_mode(other_dsi->host, other_dsi->phy);
>>>> +
>>>> /* Register slave host first, so that slave DSI device
>>>> * has a chance to probe, and do not block the master
>>>> * DSI device's probe.
>>>> @@ -98,14 +114,6 @@ static int dsi_mgr_setup_components(int id)
>>>> ret = msm_dsi_host_register(master_link_dsi->host);
>>>> if (ret)
>>>> return ret;
>>>> -
>>>> - /* PLL0 is to drive both 2 DSI link clocks in bonded DSI mode. */
>>>> - msm_dsi_phy_set_usecase(clk_master_dsi->phy,
>>>> - MSM_DSI_PHY_MASTER);
>>>> - msm_dsi_phy_set_usecase(clk_slave_dsi->phy,
>>>> - MSM_DSI_PHY_SLAVE);
>>>> - msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
>>>> - msm_dsi_host_set_phy_mode(other_dsi->host, other_dsi->phy);
>>>> }
>>>>
>>>> return 0;
>>>>
>>>> --
>>>> 2.48.1
>>>>
>>>
>>> --
>>> With best wishes
>>> Dmitry
>
Powered by blists - more mailing lists