linux-kernel - Re: [PATCH v6 08/17] drm/msm/dsi/phy: Fix reading zero as PLL rates when unprepared

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <07af3f49-6992-4897-acec-b2d46158fa00@linaro.org>
Date: Wed, 18 Jun 2025 15:54:26 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Cc: Abhinav Kumar <quic_abhinavk@...cinc.com>, Sean Paul <sean@...rly.run>,
 Marijn Suijten <marijn.suijten@...ainline.org>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Krishna Manikandan <quic_mkrishn@...cinc.com>,
 Jonathan Marek <jonathan@...ek.ca>, Kuogee Hsieh <quic_khsieh@...cinc.com>,
 Neil Armstrong <neil.armstrong@...aro.org>,
 Dmitry Baryshkov <lumag@...nel.org>, Bjorn Andersson <andersson@...nel.org>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd
 <sboyd@...nel.org>, Rob Clark <robin.clark@....qualcomm.com>,
 linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 freedreno@...ts.freedesktop.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org,
 Abel Vesa <abel.vesa@...aro.org>, Srinivas Kandagatla <srini@...nel.org>
Subject: Re: [PATCH v6 08/17] drm/msm/dsi/phy: Fix reading zero as PLL rates
 when unprepared

On 18/06/2025 15:39, Dmitry Baryshkov wrote:
> On 18/06/2025 16:34, Krzysztof Kozlowski wrote:
>> On 18/06/2025 15:07, Dmitry Baryshkov wrote:
>>> On Wed, Jun 18, 2025 at 10:28:10AM +0200, Krzysztof Kozlowski wrote:
>>>> On 13/06/2025 16:04, Dmitry Baryshkov wrote:
>>>>> On 13/06/2025 17:02, Krzysztof Kozlowski wrote:
>>>>>> On 13/06/2025 15:55, Dmitry Baryshkov wrote:
>>>>>>>>    
>>>>>>>> @@ -361,24 +373,47 @@ static int dsi_pll_7nm_lock_status(struct dsi_pll_7nm *pll)
>>>>>>>>    
>>>>>>>>    static void dsi_pll_disable_pll_bias(struct dsi_pll_7nm *pll)
>>>>>>>>    {
>>>>>>>> +	unsigned long flags;
>>>>>>>>    	u32 data;
>>>>>>>>    
>>>>>>>> +	spin_lock_irqsave(&pll->pll_enable_lock, flags);
>>>>>>>> +	--pll->pll_enable_cnt;
>>>>>>>> +	if (pll->pll_enable_cnt < 0) {
>>>>>>>> +		spin_unlock_irqrestore(&pll->pll_enable_lock, flags);
>>>>>>>> +		DRM_DEV_ERROR_RATELIMITED(&pll->phy->pdev->dev,
>>>>>>>> +					  "bug: imbalance in disabling PLL bias\n");
>>>>>>>> +		return;
>>>>>>>> +	} else if (pll->pll_enable_cnt > 0) {
>>>>>>>> +		spin_unlock_irqrestore(&pll->pll_enable_lock, flags);
>>>>>>>> +		return;
>>>>>>>> +	} /* else: == 0 */
>>>>>>>> +
>>>>>>>>    	data = readl(pll->phy->base + REG_DSI_7nm_PHY_CMN_CTRL_0);
>>>>>>>>    	data &= ~DSI_7nm_PHY_CMN_CTRL_0_PLL_SHUTDOWNB;
>>>>>>>>    	writel(0, pll->phy->pll_base + REG_DSI_7nm_PHY_PLL_SYSTEM_MUXES);
>>>>>>>>    	writel(data, pll->phy->base + REG_DSI_7nm_PHY_CMN_CTRL_0);
>>>>>>>> +	spin_unlock_irqrestore(&pll->pll_enable_lock, flags);
>>>>>>>>    	ndelay(250);
>>>>>>>
>>>>>>> What is this ndelay protecting? Is is to let the hardware to wind down
>>>>>>> correctly? I'm worried about dsi_pll_disable_pll_bias() beng followed up
>>>>>>> by dsi_pll_enable_pll_bias() in another thread, which would mean that
>>>>>>> corresponding writes to the REG_DSI_7nm_PHY_CMN_CTRL_0 can come up
>>>>>>> without any delay between them.
>>>>>>>
>>>>>>
>>>>>> Great question, but why do you ask me? The code was there already and
>>>>>> MSM DRM drivers are not something I know and could provide context about.
>>>>>
>>>>> Because it's you who are changing the code as you've faced the issue
>>>>> with recalc_rate.
>>>>>
>>>> Heh, the answer is then: I don't know. I think authors of the code could
>>>> know.
>>>
>>> The 10nm HPG documents a 250ns interval between enabling PLL bias and
>>> and enabling the PLL via the CMN_PLL_CNTRL register. There is no extra
>>> delay between disabling the PLL, disabling FIFO and remobing PLL bias.
>>> Please adjust the code for 7nm and 10nm PHYs accordingly.
>>>
>>>
>>
>> I can drop this 250 ns here, if that's what you ask me. But fixing
>> anything in 10nm is not relevant to this patchset. You were already
>> asking me for different fixes for some different things and I find it
>> not acceptable anymore. Stop blocking this patchset with every little
>> existing issue.
> 
> I think that it is a common practice to ask to fix the issue in relevant 

No, it is not.

It is common practice to fix things everywhere, but you rejecting the
patches on that basis (coming again with some requests for unrelated
issues) is not common and not correct.

> pieces. For example, we frequently ask to fix all the DT files if there 
> was an issue / workaround reported against a selected set of those.

And you reject the submitted fix of one DT file based because
contributor did not fix the rest? Really? Since when do you employ such
practice?

> 
> In this case you can send a fix for 10nm separately, but please post a 
> fix for that platform too.
> 
>>
>> Or merge this code without this patch if a fix for reading PLL as zero
>> anyhow is questionable for you.
> 
> I think I've asked it at some point, to split the generic code parts and 
> the DSI enablement into two different patch series, so that they could 
> be picked up separately.
> 
> No, the fix is not questionable. The patch causes questions though.

I have no interests in fixing DRM code. This is not my subsystem, this
is not part I am responsible for. If it raises questions, why not
devoting your time to fix it?

Best regards,
Krzysztof