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: <1ac208a7-ec63-42fe-b496-5a2e7fd615f2@linaro.org>
Date: Wed, 5 Feb 2025 14:42:03 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Dmitry Baryshkov <dmitry.baryshkov@...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 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.

Best regards,
Krzysztof