linux-kernel - Re: [PATCH v2 02/13] dt-bindings: arm: msm: Fix register regions used for LLCC banks

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]

Date:   Tue, 13 Dec 2022 19:34:20 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc:     andersson@...nel.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, bp@...en8.de,
        tony.luck@...el.com, quic_saipraka@...cinc.com,
        konrad.dybcio@...aro.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, james.morse@....com,
        mchehab@...nel.org, rric@...nel.org, linux-edac@...r.kernel.org,
        quic_ppareek@...cinc.com, luca.weiss@...rphone.com,
        stable@...r.kernel.org
Subject: Re: [PATCH v2 02/13] dt-bindings: arm: msm: Fix register regions used
 for LLCC banks

On 13/12/2022 18:30, Manivannan Sadhasivam wrote:
> On Tue, Dec 13, 2022 at 05:24:45PM +0100, Krzysztof Kozlowski wrote:
>> On 12/12/2022 13:33, Manivannan Sadhasivam wrote:
>>> Register regions of the LLCC banks are located at separate addresses.
>>> Currently, the binding just lists the LLCC0 base address and specifies
>>> the size to cover all banks. This is not the correct approach since,
>>> there are holes and other registers located in between.
>>>
>>> So let's specify the base address of each LLCC bank and get rid of
>>> reg-names property as it is not needed anymore. It should be noted that
>>> the bank count differs for each SoC, so that also needs to be taken into
>>> account in the binding.
>>>
>>> Cc: <stable@...r.kernel.org> # 4.19
>>> Fixes: 7e5700ae64f6 ("dt-bindings: Documentation for qcom, llcc")
>>> Reported-by: Parikshit Pareek <quic_ppareek@...cinc.com>
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
>>> ---
>>>  .../bindings/arm/msm/qcom,llcc.yaml           | 97 ++++++++++++++++---
>>>  1 file changed, 83 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
>>> index d1df49ffcc1b..260bc87629a7 100644
>>> --- a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
>>> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
>>> @@ -33,14 +33,8 @@ properties:
>>>        - qcom,sm8550-llcc
>>>  
>>>    reg:
>>> -    items:
>>> -      - description: LLCC base register region
>>> -      - description: LLCC broadcast base register region
>>> -
>>> -  reg-names:
>>> -    items:
>>> -      - const: llcc_base
>>> -      - const: llcc_broadcast_base
>>> +    minItems: 2
>>> +    maxItems: 9
>>>  
>>>    interrupts:
>>>      maxItems: 1
>>> @@ -48,7 +42,76 @@ properties:
>>>  required:
>>>    - compatible
>>>    - reg
>>> -  - reg-names
>>> +
>>> +allOf:
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - qcom,sc7180-llcc
>>> +              - qcom,sm6350-llcc
>>> +    then:
>>> +      properties:
>>> +        reg:
>>> +          items:
>>> +            - description: LLCC0 base register region
>>> +            - description: LLCC broadcast base register region
>>> +
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - qcom,sc7280-llcc
>>> +    then:
>>> +      properties:
>>> +        reg:
>>> +          items:
>>> +            - description: LLCC0 base register region
>>> +            - description: LLCC1 base register region
>>> +            - description: LLCC broadcast base register region
>>
>> This will break all existing users (all systems, bootloaders/firmwares),
>> so you need to explain that in commit msg - why breaking is allowed, who
>> is or is not going to be affected etc. Otherwise judging purely by
>> bindings this is an ABI break.
>>
>> Reason "This is not the correct approach since, there are holes and
>> other registers located in between." is not enough, because this
>> suggests previous approach was just not the best and you have something
>> better. Better is not a reason for ABI break.
>>
> 
> Maybe I need to reword the commit message a bit. But clearly the binding was
> wrong for rest of the SoCs other than SDM845 as the total size of the LLCC
> region includes registers of other peripherals like memory controller.
> 
> In that case, will you let the binding to be wrong or fix it?

Sure it needs fixing, but as I said you need to explain why breaking ABI
is okay and who/where is going to be affected.

Best regards,
Krzysztof