linux-kernel - Re: [PATCH 2/3] USB: dwc3: qcom: fix wakeup after probe deferral

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, 21 Nov 2023 18:25:37 +0530
From:   Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
To:     Johan Hovold <johan@...nel.org>,
        Andrew Halaney <ahalaney@...hat.com>
CC:     Johan Hovold <johan+linaro@...nel.org>,
        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>,
        Conor Dooley <conor+dt@...nel.org>,
        Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Wesley Cheng <quic_wcheng@...cinc.com>,
        <linux-arm-msm@...r.kernel.org>, <linux-usb@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <stable@...r.kernel.org>
Subject: Re: [PATCH 2/3] USB: dwc3: qcom: fix wakeup after probe deferral

> 
>> I get that dwc3_qcom_enable_interrupts() limits the scope of what wakes us
>> up to what we expect given the current device (or lack thereof), but it
>> doesn't seem like you're really meant to play with the IRQ triggers,
>> or at least the warning you shared makes me think it is not a great idea
>> if you plan to probe the device ever again in the future.
>>
>> I'll post the current comment in dwc3_qcom_enable_interrupts() to
>> explain the "limits the scope of what wakes us up" a bit more clearly:
>>
>> 	/*
>> 	 * Configure DP/DM line interrupts based on the USB2 device attached to
>> 	 * the root hub port. When HS/FS device is connected, configure the DP line
>> 	 * as falling edge to detect both disconnect and remote wakeup scenarios. When
>> 	 * LS device is connected, configure DM line as falling edge to detect both
>> 	 * disconnect and remote wakeup. When no device is connected, configure both
>> 	 * DP and DM lines as rising edge to detect HS/HS/LS device connect scenario.
>> 	 */
> 
> Yes, that is how it is currently implemented and I intend to change that
> shortly. I just wanted to get the fixes out first.
> 
> Specifically, I consider the current implementation to be broken in that
> it generates wakeup events on disconnect which is generally not want you
> want. Consider closing the lid of your laptop and disconnecting a USB
> mouse before putting it in your backpack. Now it's no longer suspended
> as you would expect it to be.
> 
> With the devictrees soon fixed, we could also do away with changing the
> trigger type, but since this is how it was implemented initially we now
> need to consider backward compatibility with the broken DTs. We've dealt
> with that before, but yeah, getting things right from the start would
> have been so much better.
> 

Hi Johan,

  Just one query. Even if it wakes up after closing the lid and removing 
the mouse, wouldn't pm suspend be triggered again later by the system 
once it sees that usb is also good to be suspended again ? I presume a 
laptop form factor would be having this facility of re-trigerring 
suspend. Let me know if this is not the case.

Also, the warning you are mentioning in [1] comes because this is a 
laptop form factor and we have some firmware running (I don't know much 
about ACPI and stuff) ?

[1]: 
https://lore.kernel.org/all/20231120161607.7405-3-johan+linaro@kernel.org/

Regards,
Krishna,