Message ID | 20231120161607.7405-3-johan+linaro@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:2b07:b0:403:3b70:6f57 with SMTP id io7csp43258vqb; Mon, 20 Nov 2023 08:16:54 -0800 (PST) X-Google-Smtp-Source: AGHT+IFnG++Pw3qqEIrrYNgLzOqycAvbtb8CFxpzbGuzbU9UidBBy+XyXOh4DE6O0PCIx1uyc8KJ X-Received: by 2002:a05:6870:c1c6:b0:1d0:d9e2:985f with SMTP id i6-20020a056870c1c600b001d0d9e2985fmr8570683oad.57.1700497014341; Mon, 20 Nov 2023 08:16:54 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1700497014; cv=none; d=google.com; s=arc-20160816; b=yn7TpyJ42TN2Aw9zG+5L3f5yAo7pYKO/VaOAQPaZTVip48Fr9LtkiZAOocX07/po9n bS9VqBxGlXDoNjiKT21mVAkPNO0eQx4/9QXRoijpu2fMlW5aYT0/pWC0Dbx7R0bFf+J4 2irXITRA9wP5Yw4RxSz/VPqIYXqb3jgvqBQuApBoCxa05zerP1YWEYhQohAlS2fbJZWc RI1mKAg1L69R2TiJXNb1ULvJRb7ALZmNVOJO7pmpRnMomqKg4EHWlo2FvYjD0GnGc5Cs hUtmrZGq24mFTe6IZRIo4fgIu7gphJkjThA5/+MWeV1Q/DR1qr+45c1mZ4ciMMptfJYE WrHw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=gggzIurzExw3FOP18r5l1fwzf4oFLxkFldeW697AT+4=; fh=DosjPweW6NClkqpOhkofDlSjjyWuV2NroeWkw4lGFFA=; b=bjtDgMKPcZHvDTDWnKp//PIgQbudHCcWsOYy1gr9+AmVhS5UyJWPmVEBUSwsCYgGfQ qHDV73Eb7bhKSoZs0rcQCq5v5K/YFBuVYNAucXN6HQQ9w9CWPc7Me97wQnh4xH7wXEuM EJfAMDuUm1GcVzyQ3/tw8EmcQokHJNXxsH0mAw1qBV3Uad97qVUax5SmARXgKd8OHZU4 drDknC2+R7ht3R4IegHmlfXroA1VIiwK3HPbiGXNiaPphaEdwOOg1WwqbWE6VBq7ejdU xcUbwawYRvztMKu6/0++DSovJ1cYtoFmcU2CKxEY5mrgeT513W6pqJd5zAOeqRinSMir kBkg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=f+M7JHDM; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from lipwig.vger.email (lipwig.vger.email. [23.128.96.33]) by mx.google.com with ESMTPS id en22-20020a056870079600b001daf105cfbbsi2928065oab.49.2023.11.20.08.16.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 20 Nov 2023 08:16:54 -0800 (PST) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=f+M7JHDM; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by lipwig.vger.email (Postfix) with ESMTP id 261C9803215B; Mon, 20 Nov 2023 08:16:52 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at lipwig.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233752AbjKTQQb (ORCPT <rfc822;heyuhang3455@gmail.com> + 27 others); Mon, 20 Nov 2023 11:16:31 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33026 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231996AbjKTQQX (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 20 Nov 2023 11:16:23 -0500 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BC09BF4; Mon, 20 Nov 2023 08:16:19 -0800 (PST) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 31264C433C7; Mon, 20 Nov 2023 16:16:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1700496979; bh=0MaPFqn+cznucJKrIbyiZQIJbG+WLSHKNg43LW6FYzo=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=f+M7JHDMHn8ZVIT2JxDub7nG01OiwBdMwf6+zh466edwZ60K1AAqZ1ZksTXL3hxiZ MLK4KeLxSkakqe6D/mrFe48elNTGrewsWACuaJ0lazdJ+mEzQUbqbCTL+uGj3RsN3m HecodNDE8QVfthebB1KO9Gz7/wvHdqbTksKGf1+sGZgbpWngA+D2HcGpy1+3xGvCfR fHsmBoa7Sv2deofSjvPqwZZ84mF4G3MC2a2lM+OH0E5uuuXpqN9TA594gFju+HuDMV cbkqKZgIWtUeCmKK94wgRU+N8EvCNgoGfQWkLixLSSt/qvzVsOU88JpkZpz+76nVEf r94+ZqlFDwTNg== Received: from johan by xi.lan with local (Exim 4.96.2) (envelope-from <johan+linaro@kernel.org>) id 1r56wm-0001vo-1x; Mon, 20 Nov 2023 17:16:28 +0100 From: Johan Hovold <johan+linaro@kernel.org> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: 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>, Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>, linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Johan Hovold <johan+linaro@kernel.org>, stable@vger.kernel.org Subject: [PATCH 2/3] USB: dwc3: qcom: fix wakeup after probe deferral Date: Mon, 20 Nov 2023 17:16:06 +0100 Message-ID: <20231120161607.7405-3-johan+linaro@kernel.org> X-Mailer: git-send-email 2.41.0 In-Reply-To: <20231120161607.7405-1-johan+linaro@kernel.org> References: <20231120161607.7405-1-johan+linaro@kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-1.2 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Mon, 20 Nov 2023 08:16:52 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1783100353137525653 X-GMAIL-MSGID: 1783100357481478011 |
Series |
USB: dwc3: qcom: fix wakeup after probe deferral
|
|
Commit Message
Johan Hovold
Nov. 20, 2023, 4:16 p.m. UTC
The Qualcomm glue driver is overriding the interrupt trigger types
defined by firmware when requesting the wakeup interrupts during probe.
This can lead to a failure to map the DP/DM wakeup interrupts after a
probe deferral as the firmware defined trigger types do not match the
type used for the initial mapping:
irq: type mismatch, failed to map hwirq-14 for interrupt-controller@b220000!
irq: type mismatch, failed to map hwirq-15 for interrupt-controller@b220000!
Fix this by not overriding the firmware provided trigger types when
requesting the wakeup interrupts.
Fixes: a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver")
Cc: stable@vger.kernel.org # 4.18
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
drivers/usb/dwc3/dwc3-qcom.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
Comments
On Mon, Nov 20, 2023 at 05:16:06PM +0100, Johan Hovold wrote: > The Qualcomm glue driver is overriding the interrupt trigger types > defined by firmware when requesting the wakeup interrupts during probe. > > This can lead to a failure to map the DP/DM wakeup interrupts after a > probe deferral as the firmware defined trigger types do not match the > type used for the initial mapping: > > irq: type mismatch, failed to map hwirq-14 for interrupt-controller@b220000! > irq: type mismatch, failed to map hwirq-15 for interrupt-controller@b220000! > > Fix this by not overriding the firmware provided trigger types when > requesting the wakeup interrupts. This series looks good to me and makes sense except for one point that I'm struggling to understand. What exactly is the relationship with this failure and probe deferral? Thanks, Andrew > > Fixes: a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver") > Cc: stable@vger.kernel.org # 4.18 > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/usb/dwc3/dwc3-qcom.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 10fb481d943b..82544374110b 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -549,7 +549,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) > irq_set_status_flags(irq, IRQ_NOAUTOEN); > ret = devm_request_threaded_irq(qcom->dev, irq, NULL, > qcom_dwc3_resume_irq, > - IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > + IRQF_ONESHOT, > "qcom_dwc3 HS", qcom); > if (ret) { > dev_err(qcom->dev, "hs_phy_irq failed: %d\n", ret); > @@ -564,7 +564,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) > irq_set_status_flags(irq, IRQ_NOAUTOEN); > ret = devm_request_threaded_irq(qcom->dev, irq, NULL, > qcom_dwc3_resume_irq, > - IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > + IRQF_ONESHOT, > "qcom_dwc3 DP_HS", qcom); > if (ret) { > dev_err(qcom->dev, "dp_hs_phy_irq failed: %d\n", ret); > @@ -579,7 +579,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) > irq_set_status_flags(irq, IRQ_NOAUTOEN); > ret = devm_request_threaded_irq(qcom->dev, irq, NULL, > qcom_dwc3_resume_irq, > - IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > + IRQF_ONESHOT, > "qcom_dwc3 DM_HS", qcom); > if (ret) { > dev_err(qcom->dev, "dm_hs_phy_irq failed: %d\n", ret); > @@ -594,7 +594,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) > irq_set_status_flags(irq, IRQ_NOAUTOEN); > ret = devm_request_threaded_irq(qcom->dev, irq, NULL, > qcom_dwc3_resume_irq, > - IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > + IRQF_ONESHOT, > "qcom_dwc3 SS", qcom); > if (ret) { > dev_err(qcom->dev, "ss_phy_irq failed: %d\n", ret); > -- > 2.41.0 > >
On Mon, Nov 20, 2023 at 11:39:07AM -0600, Andrew Halaney wrote: > On Mon, Nov 20, 2023 at 05:16:06PM +0100, Johan Hovold wrote: > > The Qualcomm glue driver is overriding the interrupt trigger types > > defined by firmware when requesting the wakeup interrupts during probe. > > > > This can lead to a failure to map the DP/DM wakeup interrupts after a > > probe deferral as the firmware defined trigger types do not match the > > type used for the initial mapping: > > > > irq: type mismatch, failed to map hwirq-14 for interrupt-controller@b220000! > > irq: type mismatch, failed to map hwirq-15 for interrupt-controller@b220000! > > > > Fix this by not overriding the firmware provided trigger types when > > requesting the wakeup interrupts. > > This series looks good to me and makes sense except for one point that > I'm struggling to understand. What exactly is the relationship with this > failure and probe deferral? Eric Chanudet pointed out to me (thanks!) offlist that if you: 1. Probe 2. Grab the IRQ 3. Request it (and muck with the trigger from the firmware default) 4. Defer out 5. Reprobe 6. Grab the IRQ again You get that error, which I played with some this afternoon... and can confirm. It really seems like maybe we should consider reworking messing with the trigger type at all (which is done later for runtime/system suspend) in a follow-up series? As far as I can tell if you were to remove the driver and reprobe after a suspend you'd hit similar. I've been sitting here scratching my head a bit trying to reason out why keeping it as IRQ_TYPE_EDGE_BOTH isn't acceptable in dwc3_qcom_enable_interrupts()... Correct me if you think that playing with the trigger there is really ok, but it seems like you run the same risks if you do that and then modprobe -r dwc3-qcom. 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. */ > > Thanks, > Andrew > > > > > Fixes: a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver") > > Cc: stable@vger.kernel.org # 4.18 > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > --- > > drivers/usb/dwc3/dwc3-qcom.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > > index 10fb481d943b..82544374110b 100644 > > --- a/drivers/usb/dwc3/dwc3-qcom.c > > +++ b/drivers/usb/dwc3/dwc3-qcom.c > > @@ -549,7 +549,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) > > irq_set_status_flags(irq, IRQ_NOAUTOEN); > > ret = devm_request_threaded_irq(qcom->dev, irq, NULL, > > qcom_dwc3_resume_irq, > > - IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > > + IRQF_ONESHOT, > > "qcom_dwc3 HS", qcom); > > if (ret) { > > dev_err(qcom->dev, "hs_phy_irq failed: %d\n", ret); > > @@ -564,7 +564,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) > > irq_set_status_flags(irq, IRQ_NOAUTOEN); > > ret = devm_request_threaded_irq(qcom->dev, irq, NULL, > > qcom_dwc3_resume_irq, > > - IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > > + IRQF_ONESHOT, > > "qcom_dwc3 DP_HS", qcom); > > if (ret) { > > dev_err(qcom->dev, "dp_hs_phy_irq failed: %d\n", ret); > > @@ -579,7 +579,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) > > irq_set_status_flags(irq, IRQ_NOAUTOEN); > > ret = devm_request_threaded_irq(qcom->dev, irq, NULL, > > qcom_dwc3_resume_irq, > > - IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > > + IRQF_ONESHOT, > > "qcom_dwc3 DM_HS", qcom); > > if (ret) { > > dev_err(qcom->dev, "dm_hs_phy_irq failed: %d\n", ret); > > @@ -594,7 +594,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) > > irq_set_status_flags(irq, IRQ_NOAUTOEN); > > ret = devm_request_threaded_irq(qcom->dev, irq, NULL, > > qcom_dwc3_resume_irq, > > - IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > > + IRQF_ONESHOT, > > "qcom_dwc3 SS", qcom); > > if (ret) { > > dev_err(qcom->dev, "ss_phy_irq failed: %d\n", ret); > > -- > > 2.41.0 > > > >
On Mon, Nov 20, 2023 at 02:50:52PM -0600, Andrew Halaney wrote: > On Mon, Nov 20, 2023 at 11:39:07AM -0600, Andrew Halaney wrote: > > On Mon, Nov 20, 2023 at 05:16:06PM +0100, Johan Hovold wrote: > > > The Qualcomm glue driver is overriding the interrupt trigger types > > > defined by firmware when requesting the wakeup interrupts during probe. > > > > > > This can lead to a failure to map the DP/DM wakeup interrupts after a > > > probe deferral as the firmware defined trigger types do not match the > > > type used for the initial mapping: > > > > > > irq: type mismatch, failed to map hwirq-14 for interrupt-controller@b220000! > > > irq: type mismatch, failed to map hwirq-15 for interrupt-controller@b220000! > > > > > > Fix this by not overriding the firmware provided trigger types when > > > requesting the wakeup interrupts. > > > > This series looks good to me and makes sense except for one point that > > I'm struggling to understand. What exactly is the relationship with this > > failure and probe deferral? > > Eric Chanudet pointed out to me (thanks!) offlist that if you: > > 1. Probe > 2. Grab the IRQ > 3. Request it (and muck with the trigger from the firmware default) > 4. Defer out > 5. Reprobe > 6. Grab the IRQ again > > You get that error, which I played with some this afternoon... > and can confirm. > > It really seems like maybe we should consider reworking messing with the > trigger type at all (which is done later for runtime/system suspend) > in a follow-up series? > > As far as I can tell if you were to remove the driver and reprobe after > a suspend you'd hit similar. Correct, but people don't go around unloading modules (unlike probe deferral which anyone can hit). It's a development (debugging) feature so there being some corner cases are not that big of a deal. > I've been sitting here scratching my head a > bit trying to reason out why keeping it as IRQ_TYPE_EDGE_BOTH isn't > acceptable in dwc3_qcom_enable_interrupts()... Correct me if you think > that playing with the trigger there is really ok, but it seems like you > run the same risks if you do that and then modprobe -r dwc3-qcom. Changing the trigger type during runtime depending on use-case should be fine. It just doesn't play well with the kernel's interrupt mapping code, which assumes that if an interrupt already has a mapping then it is a shared interrupt. I considered addressing that in the core code, but yeah, I don't want too much time since the remaining issue only affects module unload and there are other ways to avoid that issue too. > 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. Johan
> >> 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,
On Tue, Nov 21, 2023 at 06:25:37PM +0530, Krishna Kurapati PSSNV wrote: > > 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. > 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. No, we generally don't use opportunistic suspend (e.g. unlike android) so the laptop will not suspend again. So this is an actual bug affecting, for example, the Lenovo ThinkPad X13s. > 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) ? No, the "firmware" in this case is just the devicetree which has the DP/DM interrupts defined as edge-triggered while the driver requests them as level triggered. (It would look similar with ACPI firmware which also has these declared as edge triggered.) 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. > > No, we generally don't use opportunistic suspend (e.g. unlike android) > so the laptop will not suspend again. > > So this is an actual bug affecting, for example, the Lenovo ThinkPad > X13s. > Thanks for the clarification. I was thinking in android perspective only. But if we don't wake up the system upon disconnect, wouldn't the controller still be under the assumption that device is connected when it is actually not and only realise this when we resume later ? >> 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) ? > > No, the "firmware" in this case is just the devicetree which has the > DP/DM interrupts defined as edge-triggered while the driver requests > them as level triggered. > > (It would look similar with ACPI firmware which also has these declared > as edge triggered.) > Got it. Thanks for the clarification. Regards, Krishna,
On Tue, Nov 21, 2023 at 07:38:18PM +0530, Krishna Kurapati PSSNV wrote: > >> 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. > > > > No, we generally don't use opportunistic suspend (e.g. unlike android) > > so the laptop will not suspend again. > > > > So this is an actual bug affecting, for example, the Lenovo ThinkPad > > X13s. > Thanks for the clarification. I was thinking in android perspective > only. But if we don't wake up the system upon disconnect, wouldn't the > controller still be under the assumption that device is connected when > it is actually not and only realise this when we resume later ? Correct. Johan
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index 10fb481d943b..82544374110b 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -549,7 +549,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) irq_set_status_flags(irq, IRQ_NOAUTOEN); ret = devm_request_threaded_irq(qcom->dev, irq, NULL, qcom_dwc3_resume_irq, - IRQF_TRIGGER_HIGH | IRQF_ONESHOT, + IRQF_ONESHOT, "qcom_dwc3 HS", qcom); if (ret) { dev_err(qcom->dev, "hs_phy_irq failed: %d\n", ret); @@ -564,7 +564,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) irq_set_status_flags(irq, IRQ_NOAUTOEN); ret = devm_request_threaded_irq(qcom->dev, irq, NULL, qcom_dwc3_resume_irq, - IRQF_TRIGGER_HIGH | IRQF_ONESHOT, + IRQF_ONESHOT, "qcom_dwc3 DP_HS", qcom); if (ret) { dev_err(qcom->dev, "dp_hs_phy_irq failed: %d\n", ret); @@ -579,7 +579,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) irq_set_status_flags(irq, IRQ_NOAUTOEN); ret = devm_request_threaded_irq(qcom->dev, irq, NULL, qcom_dwc3_resume_irq, - IRQF_TRIGGER_HIGH | IRQF_ONESHOT, + IRQF_ONESHOT, "qcom_dwc3 DM_HS", qcom); if (ret) { dev_err(qcom->dev, "dm_hs_phy_irq failed: %d\n", ret); @@ -594,7 +594,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) irq_set_status_flags(irq, IRQ_NOAUTOEN); ret = devm_request_threaded_irq(qcom->dev, irq, NULL, qcom_dwc3_resume_irq, - IRQF_TRIGGER_HIGH | IRQF_ONESHOT, + IRQF_ONESHOT, "qcom_dwc3 SS", qcom); if (ret) { dev_err(qcom->dev, "ss_phy_irq failed: %d\n", ret);