linux-kernel - Re: [PATCH v2 2/4] drm/msm/dsi/phy: Protect PHY_CMN_CLK

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: <t4zn2gv3mbn3nqlh5h3l3ej5zxs4wa74ncgymonci7v45w47ai@qmmtxbelichw>
Date: Wed, 5 Feb 2025 04:51:12 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: Rob Clark <robdclark@...il.com>, 
	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>, linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org, 
	freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/4] drm/msm/dsi/phy: Protect PHY_CMN_CLK_CFG1 against
 clock driver

On Tue, Feb 04, 2025 at 04:46:04PM +0100, Krzysztof Kozlowski wrote:
> On 04/02/2025 15:26, Dmitry Baryshkov wrote:
> > On Tue, Feb 04, 2025 at 10:21:25AM +0100, Krzysztof Kozlowski wrote:
> >> On 03/02/2025 18:41, Dmitry Baryshkov wrote:
> >>> On Mon, Feb 03, 2025 at 06:29:19PM +0100, Krzysztof Kozlowski wrote:
> >>>> PHY_CMN_CLK_CFG1 register is updated by the PHY driver and by a mux
> >>>> clock from Common Clock Framework:
> >>>> devm_clk_hw_register_mux_parent_hws().  There could be a path leading to
> >>>> concurrent and conflicting updates between PHY driver and clock
> >>>> framework, e.g. changing the mux and enabling PLL clocks.
> >>>>
> >>>> Add dedicated spinlock to be sure all PHY_CMN_CLK_CFG1 updates are
> >>>> synchronized.
> >>>>
> >>>> Fixes: 1ef7c99d145c ("drm/msm/dsi: add support for 7nm DSI PHY/PLL")
> >>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> >>>>
> >>>> ---
> >>>>
> >>>> Changes in v2:
> >>>> 1. Store BIT(4) and BIT(5) in local var in dsi_pll_enable_global_clk()
> >>>> ---
> >>>>  drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 35 +++++++++++++++++++------------
> >>>>  1 file changed, 22 insertions(+), 13 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> >>>> index c164f845653816291ad96c863257f75462ef58e7..e26f53f7cde8f0f6419a633f5d39784dc2e5bb98 100644
> >>>> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> >>>> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> >>>> @@ -83,6 +83,9 @@ struct dsi_pll_7nm {
> >>>>  	/* protects REG_DSI_7nm_PHY_CMN_CLK_CFG0 register */
> >>>>  	spinlock_t postdiv_lock;
> >>>>  
> >>>> +	/* protects REG_DSI_7nm_PHY_CMN_CLK_CFG1 register */
> >>>> +	spinlock_t pclk_mux_lock;
> >>>> +
> >>>>  	struct pll_7nm_cached_state cached_state;
> >>>>  
> >>>>  	struct dsi_pll_7nm *slave;
> >>>> @@ -381,22 +384,32 @@ static void dsi_pll_cmn_clk_cfg0_write(struct dsi_pll_7nm *pll, u32 val)
> >>>>  	spin_unlock_irqrestore(&pll->postdiv_lock, flags);
> >>>>  }
> >>>>  
> >>>> -static void dsi_pll_disable_global_clk(struct dsi_pll_7nm *pll)
> >>>> +static void dsi_pll_cmn_clk_cfg1_update(struct dsi_pll_7nm *pll, u32 mask,
> >>>> +					u32 val)
> >>>>  {
> >>>> +	unsigned long flags;
> >>>>  	u32 data;
> >>>>  
> >>>> +	spin_lock_irqsave(&pll->pclk_mux_lock, flags);
> >>>>  	data = readl(pll->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1);
> >>>> -	writel(data & ~BIT(5), pll->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1);
> >>>> +	data &= ~mask;
> >>>> +	data |= val & mask;
> >>>> +
> >>>> +	writel(data, pll->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1);
> >>>> +	spin_unlock_irqrestore(&pll->pclk_mux_lock, flags);
> >>>> +}
> >>>> +
> >>>> +static void dsi_pll_disable_global_clk(struct dsi_pll_7nm *pll)
> >>>> +{
> >>>> +	dsi_pll_cmn_clk_cfg1_update(pll, BIT(5), 0);
> >>>>  }
> >>>>  
> >>>>  static void dsi_pll_enable_global_clk(struct dsi_pll_7nm *pll)
> >>>>  {
> >>>> -	u32 data;
> >>>> +	u32 cfg_1 = BIT(5) | BIT(4);
> >>>
> >>> Please define these two bits too.
> >>
> >> Why? They were not defined before. This only moving existing code.
> > 
> > Previously it was just a bit magic. Currently you are adding them as
> 
> No, previous code:
> 
> writel(data | BIT(5) | BIT(4), pll->phy->base +
> REG_DSI_7nm_PHY_CMN_CLK_CFG1);
> 
> This is a mask and update in the same time, because:
> 	(data & (BIT(5) | BIT(4)) | BIT(5) | BIT(4)
> is just redudant.
> 
> I did not do any logical change, I did not add any mask or field.
> Everything was already there.

Yes... and no. Previously it was just writel(foo | BIT(5) | BIT(4)). Now
your code adds BIT(5) as a 'mask' parameter. Is it a correct mask for
that field? That's why I'm asking you to define those - you have changed
bitwrites to the masked bit writes. Masks should be defined.

> 
> 
> > masks. I want to know if BIT(4) and BIT(5) are parts of the same
> > bitfield (2 bits wide) or if they define two different bits.
> 
> While in general you are right, it does not matter for this fix. If this
> are separate bitfields - fix is correct. If this is one bitfield - fix
> is still correct. You could claim that if this was one bitfield, using
> 2xBIT() is not logical, but this was there already, so again my fix is
> only fixing and keeping entire logic or inconsistencies intact.

-- 
With best wishes
Dmitry