linux-kernel - Re: [PATCH v4 12/12] interconnect: qcom: icc-rpm: Allow negative QoS offset

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]

Date:   Wed, 15 Feb 2023 02:31:14 +0100
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        linux-arm-msm@...r.kernel.org, andersson@...nel.org,
        agross@...nel.org
Cc:     marijn.suijten@...ainline.org, Georgi Djakov <djakov@...nel.org>,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 12/12] interconnect: qcom: icc-rpm: Allow negative QoS
 offset



On 15.02.2023 02:16, Dmitry Baryshkov wrote:
> On 14/02/2023 16:37, Konrad Dybcio wrote:
>> In some very very very very unfortunate cases, the correct offset of
>> the QoS registers will be.. negative. One such case is MSM8998, where
>> The DDR BWMON occupies what-would-be-the-BIMC-base which we usually
>> take into account with the register calculation, making the actual
>> BIMC node start at what-would-be-the-BIMC-base+0x300.
>>
>> In order to keep the calculation code sane, the simplest - however
>> ugly it may be - solution is to allow the offset to be negative.
> 
> I'm not sure how does this work, as e.g. qcom_icc_set_qnoc_qos() will try to access an address before the first register. Most probably this patch should go together with the rest of msm8998 patches.
In our case BIMC is the offender and its defines all base on
M_BKE_REG_BASE(n) (0x300 + (0x4000 * n)), so removing 0x300
is perfectly ok.

> 
> A slightly better solution to making qos_offset negative might be to make bwmon a child node of the icc.
That sounds like a lot of complex changes and accounting for old
/ different DTs who don't have the bwmon as a child of icc :/

Konrad
> 
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@...aro.org>
>> ---
>>   drivers/interconnect/qcom/icc-rpm.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
>> index 77e263b93c27..5e4215f25c2e 100644
>> --- a/drivers/interconnect/qcom/icc-rpm.h
>> +++ b/drivers/interconnect/qcom/icc-rpm.h
>> @@ -39,7 +39,7 @@ struct qcom_icc_provider {
>>       int num_intf_clks;
>>       enum qcom_icc_type type;
>>       struct regmap *regmap;
>> -    unsigned int qos_offset;
>> +    int qos_offset;
>>       u64 bus_clk_rate[2];
>>       bool keep_alive;
>>       struct clk_bulk_data bus_clks[2];
>> @@ -105,7 +105,7 @@ struct qcom_icc_desc {
>>       bool keep_alive;
>>       enum qcom_icc_type type;
>>       const struct regmap_config *regmap_cfg;
>> -    unsigned int qos_offset;
>> +    int qos_offset;
>>   };
>>     /* Valid for all bus types */
>