Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <aa7cab9c-8fe2-b42e-84ba-1aeb79d50190@gmail.com>
Date:   Thu, 10 Nov 2022 14:12:01 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.1
Subject: Re: [PATCH v28 05/11] soc: mediatek: refine code to use
 mtk_mmsys_update_bits API
Content-Language: en-US
To:     =?UTF-8?B?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= 
        <nfraprado@collabora.com>
Cc:     "Nancy.Lin" <nancy.lin@mediatek.com>,
        Rob Herring <robh+dt@kernel.org>,
        Chun-Kuang Hu <chunkuang.hu@kernel.org>,
        Philipp Zabel <p.zabel@pengutronix.de>, wim@linux-watchdog.org,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>, linux@roeck-us.net,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        "jason-jh . lin" <jason-jh.lin@mediatek.com>,
        Yongqiang Niu <yongqiang.niu@mediatek.com>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org,
        dri-devel@lists.freedesktop.org, llvm@lists.linux.dev,
        singo.chang@mediatek.com,
        Project_Global_Chrome_Upstream_Group@mediatek.com
References: <20221107072243.15748-1-nancy.lin@mediatek.com>
 <20221107072243.15748-6-nancy.lin@mediatek.com>
 <58d55016-afc7-e560-32cf-8228794b5752@gmail.com>
 <20221108194351.u63k4mqzq3bkhpl7@notapiano>
From:   Matthias Brugger <matthias.bgg@gmail.com>
In-Reply-To: <20221108194351.u63k4mqzq3bkhpl7@notapiano>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 08/11/2022 20:43, Nícolas F. R. A. Prado wrote:
> On Tue, Nov 08, 2022 at 06:37:19PM +0100, Matthias Brugger wrote:
>>
>>
>> On 07/11/2022 08:22, Nancy.Lin wrote:
>>> Simplify code for update  mmsys reg.
>>>
>>> Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
>>> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
>>> Tested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>> Tested-by: Bo-Chen Chen <rex-bc.chen@mediatek.com>
>>> Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>>> ---
>>>    drivers/soc/mediatek/mtk-mmsys.c | 45 ++++++++++++--------------------
>>>    1 file changed, 16 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
>>> index 9a327eb5d9d7..73c8bd27e6ae 100644
>>> --- a/drivers/soc/mediatek/mtk-mmsys.c
>>> +++ b/drivers/soc/mediatek/mtk-mmsys.c
>>> @@ -99,22 +99,27 @@ struct mtk_mmsys {
>>>    	struct reset_controller_dev rcdev;
>>>    };
>>> +static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32 offset, u32 mask, u32 val)
>>> +{
>>> +	u32 tmp;
>>> +
>>> +	tmp = readl_relaxed(mmsys->regs + offset);
>>> +	tmp = (tmp & ~mask) | (val & mask);
>>
>> I'm not sure about the change in the implementation of
>> mtk_mmsys_update_bits(). Nicolas tried to explain it to me on IRC but I
>> wasn't totally convincing. As we have to go for at least another round of
>> this patches, I'd like to get a clear understanding while it is needed that
>> val bits are set to 1 in the mask.
> 
> The point here was to make sure that mtk_mmsys_update_bits() didn't allow
> setting bits outside of the mask, since that's never what you want: the entire
> point of having a mask is to specify the bits that should be updated (and the
> ones that should be kept unchanged). So for example if you had
> 
> mask = 0x0ff0
> val  = 0x00ff
> 
> the previous implementation would happily overwrite the 4 least significant bits
> on the destination register, despite them not being present in the mask, which
> is wrong.
> 
> This wrong behavior could easily lead to hard to trace bugs as soon as a badly
> formatted/wrong val is passed and an unrelated bit updated due to the mask being
> ignored.
> 
> For reference, _regmap_update_bits() does the same masking of the value [1].
> 
> That said, given that this function already existed and was just being moved,
> it would've been cleaner to make this change in a separate commit.
> 

Would have been better, but we can leave it as it.

Regards,
Matthias

> [1] https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap.c#L3122
> 
> Thanks,
> Nícolas
> 
>>
>> Regards,
>> Matthias
>>
>>> +	writel_relaxed(tmp, mmsys->regs + offset);
>>> +}
> [..]
>>> -static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32 offset, u32 mask, u32 val)
>>> -{
>>> -	u32 tmp;
>>> -
>>> -	tmp = readl_relaxed(mmsys->regs + offset);
>>> -	tmp = (tmp & ~mask) | val;
>>> -	writel_relaxed(tmp, mmsys->regs + offset);
>>> -}
>>> -
> [..]