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: <55c478c7-abcc-4487-b81c-479df47d5666@linaro.org>
Date: Tue, 26 Dec 2023 10:33:07 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>,
 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 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?"

Best regards,
Krzysztof