linux-kernel - Re: [PATCH v2 2/6] usb: dwc3: qcom: Rename hs_phy_irq to qusb2_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: <516aeb55-69ad-460c-9757-6ad8a203b693@quicinc.com>
Date:   Fri, 8 Dec 2023 17:44:41 +0530
From:   Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
To:     Johan Hovold <johan@...nel.org>
CC:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Wesley Cheng <quic_wcheng@...cinc.com>,
        Conor Dooley <conor+dt@...nel.org>,
        <cros-qcom-dts-watchers@...omium.org>, <linux-usb@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <quic_ppratap@...cinc.com>,
        <quic_jackp@...cinc.com>
Subject: Re: [PATCH v2 2/6] usb: dwc3: qcom: Rename hs_phy_irq to
 qusb2_phy_irq



On 12/7/2023 10:04 PM, Johan Hovold wrote:
> On Thu, Dec 07, 2023 at 09:17:32PM +0530, Krishna Kurapati PSSNV wrote:
>> On 12/7/2023 8:58 PM, Johan Hovold wrote:
> 
>>> Here too you should say something about why this won't break any systems
>>> booting using an older devicetree. Specifically, the QUSB2 PHY interrupt
>>> has never been armed on any system running mainline as those bits never
>>> made it upstream.
>>>
>>> So an alternative to this could also be to just drop the QUSB2 PHY
>>> interrupt handling from this driver for now. >
> 
>> So, are you suggesting that we drop the whole patch ?
> 
> No, I meant that an alternative could be to drop the current hs_phy_irq
> handling from the driver.
> 
>> I assume if the older kernels are using old DT, they would be using an
>> old driver version too right ?
> 
> No, and this is part of the devicetree ABI as we discussed the other
> week.
> 
> You should generally be able to continue booting with an older devicetree
> on a newer kernel (even if newer functionality may not be enabled then).
> 
>> Is there a case where DT is not updated but driver is ? Because if we
>> drop this patch from series, targets with updated DT's would break.
> 
> Actually they would not due to the fact that the QUSB2 PHY interrupt is
> currently never armed in the PHY (and the interrupts are looked up by
> name and are considered optional by the driver).
> 
> But simply dropping this patch is not an option here. I'm fine with this
> patch as it is, but the reason we can merge it is that those interrupts
> are currently not actually used. Otherwise, this would break older
> devicetrees.
> 
> But this also means, we could consider dropping the current hs_phy_irq
> handling altogether.
> 
> Hmm. Looking at the qusb2_phy_runtime_suspend() again now I see that the
> interrupt is actually armed on runtime suspend, it's just that it is
> configured incorrectly and would wakeup immediately if someone ever
> exercised this path.
> 
> Specifically, the bits that would set those PHY_MODE_USB_HOST_HS modes
> (that should never have been merged) never made it upstream so this code
> is just dead code currently. I said before I'll look into ripping this
> out, but yeah, I'm swamped with work as usual (and it has been sitting
> there dead for years so there's no rush).
> 
> So to summarise, the QUSB2 wakeup handling is incomplete and broken, so
> we won't actually make things worse by renaming the interrupts. If this
> was working, we would need to continue supporting the old names, though.
> 

Thanks for the review. Since renaming the interrupts won't be an issue, 
I will keep this patch as is in that case in v3.

Regards,
Krishna,