linux-kernel - Re: [PATCH v5 1/2] dt-bindings: usb: dwc3: Clean up hs_phy

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: <67c7c84c-c631-468e-ae67-1c31d41a605b@quicinc.com>
Date: Tue, 26 Dec 2023 15:33:09 +0530
From: Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        "Bjorn
 Andersson" <bjorn.andersson@...aro.org>,
        Konrad Dybcio
	<konrad.dybcio@...aro.org>,
        Wesley Cheng <quic_wcheng@...cinc.com>,
        "Johan
 Hovold" <johan@...nel.org>
CC: <linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        Conor Dooley
	<conor+dt@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        Thinh Nguyen
	<Thinh.Nguyen@...opsys.com>, <quic_ppratap@...cinc.com>,
        <quic_jackp@...cinc.com>, Andy Gross <agross@...nel.org>
Subject: Re: [PATCH v5 1/2] dt-bindings: usb: dwc3: Clean up hs_phy_irq in
 binding



On 12/26/2023 3:03 PM, Krzysztof Kozlowski wrote:
> On 26/12/2023 06:37, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 12/25/2023 6:35 PM, Krzysztof Kozlowski wrote:
>>> On 22/12/2023 07:36, Krishna Kurapati wrote:
>>>> The high speed related interrupts present on QC targets are as follows:
>>>>
>>>
>>>
>>>>    
>>>>      interrupt-names:
>>>> -    minItems: 1
>>>> -    maxItems: 4
>>>> +    minItems: 2
>>>> +    maxItems: 5
>>>>    
>>>>      qcom,select-utmi-as-pipe-clk:
>>>>        description:
>>>> @@ -361,60 +378,21 @@ allOf:
>>>>            compatible:
>>>>              contains:
>>>>                enum:
>>>> -              - qcom,ipq4019-dwc3
>>>
>>> Why do you remove it, without adding it somewhere else. Nothing in the
>>> commit msg explains it.
>>>
>>
>> Apologies, Will check and add it back.
> 
> Please check your patch for other entries. I just took first compatible
> which turns out to be gone. I did not check the reset and I don't want
> to keep checking.
> > ...
> 
>>>> -    then:
>>>> -      properties:
>>>> -        interrupts:
>>>> -          minItems: 1
>>>> -          maxItems: 2
>>>> -        interrupt-names:
>>>> -          minItems: 1
>>>> -          items:
>>>> -            - const: hs_phy_irq
>>>> -            - const: ss_phy_irq
>>>> -
>>>> -  - if:
>>>> -      properties:
>>>> -        compatible:
>>>> -          contains:
>>>> -            enum:
>>>> -              - qcom,sc7280-dwc3
>>>> +              - qcom,sm6115-dwc3
>>>> +              - qcom,sm6125-dwc3
>>>>        then:
>>>>          properties:
>>>>            interrupts:
>>>>              minItems: 3
>>>>              maxItems: 4
>>>>            interrupt-names:
>>>> -          minItems: 3
>>>>              items:
>>>> +            - const: pwr_event
>>>>                - const: hs_phy_irq
>>>> -            - const: dp_hs_phy_irq
>>>> -            - const: dm_hs_phy_irq
>>>> +            - const: qusb2_phy
>>>
>>> Why qusb2_phy is after hs_phy_irq? In the earlier if:then: it is the
>>> second one.
>>>
>>
>> In v3 as well, the hs_phy_irq is before qusb2_phy interrupt:
>> https://lore.kernel.org/all/20231211121124.4194-2-quic_kriskura@quicinc.com/
> 
> ? How v3 matters?
> 
>>
>>>
>>>>                - const: ss_phy_irq
>>>>    
>>>>      - if:
>>>> @@ -460,11 +422,13 @@ allOf:
>>>>            compatible:
>>>>              contains:
>>>>                enum:
>>>> +              - qcom,ipq5332-dwc3
>>>>                  - qcom,sc8280xp-dwc3
>>>>                  - qcom,x1e80100-dwc3
>>>>        then:
>>>>          properties:
>>>>            interrupts:
>>>> +          minItems: 3
>>>
>>> Hm, why? This commit is unmanageable. Your commit msg is already huge
>>> but still does not explain this. Are you sure you are fixing only one
>>> logical thing per patch? Does not look like.
>>>
>>
>> This is reordering the targets based on interrupts they have. I put it
>> in one commit because splitting this into multiple patches breaks one
>> thing or other. Also once I am defining permutations, I have to group
>> targets into these combinations in the same patch. I know this is a big
>> commit but it solves the interrupt cleanup and defines a way for future
>> targets.
> 
> 
> This does not answer why, you sc8280xp and x1e80100 not get one optional
> interrupt. I asked "why" you are doing this change. Why do you need it?
> What is the rationale?
> 
> Then I grunted about unmanageable commit, because all my troubles to
> review it are the effect of it: it is very difficult to read. It is also
> difficult for you, because you keep making here mistakes. So if you
> cannot write this commit properly and I cannot review it, then it is way
> over-complicated, don't you think? But this is still second problem
> here, don't ignore the fist - "why?"

HI Krzysztof,

  Thanks for the review.
  To answer the question,

"why ?" : The interrupts have been mis-interpreted on many platforms or 
many interrupts are missing.

Now, if I am adding the missing interrupts, I need to segregate targets 
also into respective buckets in the same patch and that is what making 
this patch a little complicated. Is it possible / acceptable to split 
this into two patches if this is the case. Can you help with suggestions 
from your end ? Or may be I am understanding your question wrong ? 😅

Regards,
Krishna,