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

Message-ID: <54y7vatcrenfty5n2z2i25w7dnjo7j4fhg2rb36f54e4dogd2b@geea3s4zlrmq>
Date: Wed, 5 Feb 2025 16:51:41 +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 Wed, Feb 05, 2025 at 02:42:03PM +0100, Krzysztof Kozlowski wrote:
> On 05/02/2025 12:23, Dmitry Baryshkov wrote:
> >>>>>>>> +
> >>>>>>>> +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
> >>
> >> You did not address my comment. Previous code was:
> >>
> >> (foo & (BIT(5) | BIT(4)) | BIT(5) | BIT(4)
> >>
> >> Just for shorter syntax it was written different way:
> >>
> >> foo | BIT(5) | BIT(4)
> > 
> > Previously it was a simple writel() with some bit magic. Now you call
> 
> 
> The mask was already there, just implied.
> 
> > dsi_pll_cmn_clk_cfg1_update() passing the register bit field through
> > the 'mask' argument. I'm asking to get those masks defined. Is it
> > possible?
> 
> Just like before, because implied mask is being removed due to code
> redundancy.
> 
> I repeat it for third time already.
> 
> > 
> > Yes, the code is equivalent and results in the same values being
> > written to the same registers.
> > At the same time you have added a logical entity, a masked write. I
> > want to be able to understand if bits 4 and 5 are a part of the same
> > register field or they belong to two different fields and can be
> 
> I know you want to understand it and this is achieved in separate patch,
> because understanding this is not related to this commit.
> 
> > written separately. I really don't understand why are we spending so
> > much time arguing about a simple #define. Okay, in case of drm/msm it
> > is not a #define, it is <reg><bitfield/></reg>. The net result is the
> > same.
> 
> I also don't get why simple fix could not be just applied and it has to
> become some sort of big refactoring.

Well, you have refactored that in this patch. Anyway. Please post the
next iteration, let's continue the dicussion there.

> 
> Best regards,
> Krzysztof

-- 
With best wishes
Dmitry