Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Message-ID: <0b627853-78fb-4320-84e4-f88695ac6a9e@quicinc.com>
Date:   Tue, 21 Nov 2023 18:25:37 +0530
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/3] USB: dwc3: qcom: fix wakeup after probe deferral
To:     Johan Hovold <johan@kernel.org>,
        Andrew Halaney <ahalaney@redhat.com>
CC:     Johan Hovold <johan+linaro@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
        Wesley Cheng <quic_wcheng@quicinc.com>,
        <linux-arm-msm@vger.kernel.org>, <linux-usb@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <stable@vger.kernel.org>
References: <20231120161607.7405-1-johan+linaro@kernel.org>
 <20231120161607.7405-3-johan+linaro@kernel.org>
 <pgmtla6j3dshuq5zdxstszbkkssxcthtzelv2etcbrlstdw4nu@wixz6v5dfpum>
 <3ff65t36p6n3k7faw2z75t2vfi6rb5p64x7wqosetsksbhhwli@5xaxnm7zz4tu>
 <ZVx1wRefjNaN0byk@hovoldconsulting.com>
Content-Language: en-US
From:   Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
In-Reply-To: <ZVx1wRefjNaN0byk@hovoldconsulting.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

> 
>> 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,