* [PATCH v2 1/7] usb: dwc3: qcom: re-enable wakeup interrupts on failed resume
2026-09-03 15:38 [PATCH v2 0/7] usb: dwc3: qcom: Add firmware-managed resource support for SA8255P Faisal Hassan
@ 2026-09-03 15:38 ` Faisal Hassan
2026-09-03 15:53 ` sashiko-bot
2026-09-03 15:38 ` [PATCH v2 2/7] usb: dwc3: qcom: Distinguish PM and runtime suspend/resume paths Faisal Hassan
` (5 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Faisal Hassan @ 2026-09-03 15:38 UTC (permalink / raw)
To: Thinh Nguyen, Greg Kroah-Hartman, Mathias Nyman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Wesley Cheng
Cc: Sriram Dash, Jack Pham, Krishna Kurapati, Shazad Hussain,
linux-arm-msm, linux-usb, devicetree, linux-kernel
dwc3_qcom_resume() disables wakeup interrupts before re-enabling
clocks, then re-enables them again once resume completes. If
clk_bulk_prepare_enable() fails partway through, the function
returns early without re-enabling the interrupts that were just
disabled, leaving host-mode wakeup interrupts masked after a
failed resume.
Jump to a common cleanup path that re-enables the interrupts
before returning the error, mirroring the success path.
Signed-off-by: Faisal Hassan <faisal.hassan@oss.qualcomm.com>
---
drivers/usb/dwc3/dwc3-qcom.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index ac68b4218b56..49698a31b2f4 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -382,7 +382,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
if (ret < 0)
- return ret;
+ goto enable_irq;
ret = dwc3_qcom_interconnect_enable(qcom);
if (ret)
@@ -398,6 +398,16 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
qcom->is_suspended = false;
return 0;
+
+enable_irq:
+ /*
+ * Re-enable interrupts if they were disabled earlier, so a failed
+ * resume doesn't leave wakeup interrupts masked.
+ */
+ if (dwc3_qcom_is_host(qcom) && wakeup)
+ dwc3_qcom_enable_interrupts(qcom);
+
+ return ret;
}
static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 1/7] usb: dwc3: qcom: re-enable wakeup interrupts on failed resume
2026-09-03 15:38 ` [PATCH v2 1/7] usb: dwc3: qcom: re-enable wakeup interrupts on failed resume Faisal Hassan
@ 2026-09-03 15:53 ` sashiko-bot
0 siblings, 0 replies; 14+ messages in thread
From: sashiko-bot @ 2026-09-03 15:53 UTC (permalink / raw)
To: Faisal Hassan; +Cc: devicetree, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] Re-enabling wakeup interrupts on a failed resume path causes an unrecoverable interrupt storm.
Pre-existing issues:
- [High] Using a read-modify-write operation (`dwc3_qcom_setbits`) to clear a Write-1-to-Clear (W1C) interrupt status register unintentionally clears all other pending events.
--
commit 04b120125f0ae65d0e301d68751f219b8e9f86e0
Author: Faisal Hassan <faisal.hassan@oss.qualcomm.com>
usb: dwc3: qcom: re-enable wakeup interrupts on failed resume
This commit modifies dwc3_qcom_resume() to handle failures from
clk_bulk_prepare_enable(). On failure, it jumps to a new enable_irq label
that re-enables wakeup interrupts if they were disabled earlier.
> dwc3_qcom_resume() disables wakeup interrupts before re-enabling
> clocks, then re-enables them again once resume completes. If
> clk_bulk_prepare_enable() fails partway through, the function
> returns early without re-enabling the interrupts that were just
> disabled, leaving host-mode wakeup interrupts masked after a
> failed resume.
>
> Jump to a common cleanup path that re-enables the interrupts
> before returning the error, mirroring the success path.
[Severity: High]
Does the success path actually re-enable the interrupts? Looking at
dwc3_qcom_resume(), the success path just returns 0 without re-enabling them,
as wakeup interrupts are meant to be active only while the device is suspended.
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index ac68b4218b56b..49698a31b2f41 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -382,7 +382,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
>
> ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
> if (ret < 0)
> - return ret;
> + goto enable_irq;
[Severity: High]
If we jump to enable_irq here, we bypass clearing the hardware events in
pwr_evnt_irq_stat_reg (which happens later in this function). Since clearing
the register requires the clocks to be enabled, jumping to the error path means
the pending hardware event is never cleared.
Will re-enabling the interrupt line below without clearing this hardware event
cause the interrupt to remain asserted? Since qcom_dwc3_resume_irq() returns
IRQ_HANDLED, this could cause the kernel IRQ subsystem to unmask the line,
resulting in an immediate interrupt storm that hangs the CPU.
[ ... ]
> @@ -398,6 +398,16 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
> qcom->is_suspended = false;
>
> return 0;
> +
> +enable_irq:
> + /*
> + * Re-enable interrupts if they were disabled earlier, so a failed
> + * resume doesn't leave wakeup interrupts masked.
> + */
> + if (dwc3_qcom_is_host(qcom) && wakeup)
> + dwc3_qcom_enable_interrupts(qcom);
[Severity: High]
This is a pre-existing issue, but earlier in dwc3_qcom_resume() the code clears
L2-related events using dwc3_qcom_setbits():
for (i = 0; i < qcom->num_ports; i++) {
dwc3_qcom_setbits(qcom->qscratch_base,
pwr_evnt_irq_stat_reg[i],
PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);
}
Since pwr_evnt_irq_stat_reg is a Write-1-to-Clear (W1C) register, doesn't using
a read-modify-write operation like dwc3_qcom_setbits() read all currently
pending interrupts as '1's and write them back as '1's?
Could this unintentionally acknowledge and clear every other unrelated pending
event in the register, causing missed hardware events and stuck states?
> +
> + return ret;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260903153827.3463313-1-faisal.hassan@oss.qualcomm.com?part=1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/7] usb: dwc3: qcom: Distinguish PM and runtime suspend/resume paths
2026-09-03 15:38 [PATCH v2 0/7] usb: dwc3: qcom: Add firmware-managed resource support for SA8255P Faisal Hassan
2026-09-03 15:38 ` [PATCH v2 1/7] usb: dwc3: qcom: re-enable wakeup interrupts on failed resume Faisal Hassan
@ 2026-09-03 15:38 ` Faisal Hassan
2026-09-03 15:38 ` [PATCH v2 3/7] dt-bindings: usb: Add qcom,sa8255p-dwc3 for firmware-managed resources Faisal Hassan
` (4 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Faisal Hassan @ 2026-09-03 15:38 UTC (permalink / raw)
To: Thinh Nguyen, Greg Kroah-Hartman, Mathias Nyman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Wesley Cheng
Cc: Sriram Dash, Jack Pham, Krishna Kurapati, Shazad Hussain,
linux-arm-msm, linux-usb, devicetree, linux-kernel
The dwc3_qcom_suspend() and dwc3_qcom_resume() functions
handle both system PM and runtime PM paths but cannot
differentiate between them, preventing distinct power
management strategies.
Refactor suspend/resume functions to accept pm_message_t
instead of boolean wakeup flags. Use PMSG_IS_AUTO()
to identify runtime PM (PMSG_AUTO_SUSPEND/RESUME) versus
system PM (PMSG_SUSPEND/RESUME) paths.
Preserve existing wakeup behavior: always enable wakeup
for runtime suspend, check device_may_wakeup() for system
suspend.
Follow dwc3/core.c pattern to enable future per-path
strategies like differentiated power states.
Co-developed-by: Sriram Dash <sriram.dash@oss.qualcomm.com>
Signed-off-by: Sriram Dash <sriram.dash@oss.qualcomm.com>
Signed-off-by: Faisal Hassan <faisal.hassan@oss.qualcomm.com>
---
drivers/usb/dwc3/dwc3-qcom.c | 34 ++++++++++++++++++++++++++--------
1 file changed, 26 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 49698a31b2f4..6d25f81800a7 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -335,14 +335,24 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
dwc3_qcom_enable_port_interrupts(&qcom->ports[i]);
}
-static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
+static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, pm_message_t msg)
{
u32 val;
int i, ret;
+ bool wakeup;
if (qcom->is_suspended)
return 0;
+ /*
+ * For runtime suspend, always enable wakeup.
+ * For system suspend, check device wakeup capability.
+ */
+ if (PMSG_IS_AUTO(msg))
+ wakeup = true;
+ else
+ wakeup = device_may_wakeup(qcom->dev);
+
for (i = 0; i < qcom->num_ports; i++) {
val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg[i]);
if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
@@ -369,14 +379,24 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
return 0;
}
-static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
+static int dwc3_qcom_resume(struct dwc3_qcom *qcom, pm_message_t msg)
{
int ret;
int i;
+ bool wakeup;
if (!qcom->is_suspended)
return 0;
+ /*
+ * For runtime resume, always assume wakeup was enabled.
+ * For system resume, check device wakeup capability.
+ */
+ if (PMSG_IS_AUTO(msg))
+ wakeup = true;
+ else
+ wakeup = device_may_wakeup(qcom->dev);
+
if (dwc3_qcom_is_host(qcom) && wakeup)
dwc3_qcom_disable_interrupts(qcom);
@@ -759,14 +779,13 @@ static int dwc3_qcom_pm_suspend(struct device *dev)
{
struct dwc3 *dwc = dev_get_drvdata(dev);
struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
- bool wakeup = device_may_wakeup(dev);
int ret;
ret = dwc3_pm_suspend(&qcom->dwc);
if (ret)
return ret;
- ret = dwc3_qcom_suspend(qcom, wakeup);
+ ret = dwc3_qcom_suspend(qcom, PMSG_SUSPEND);
if (ret)
return ret;
@@ -779,10 +798,9 @@ static int dwc3_qcom_pm_resume(struct device *dev)
{
struct dwc3 *dwc = dev_get_drvdata(dev);
struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
- bool wakeup = device_may_wakeup(dev);
int ret;
- ret = dwc3_qcom_resume(qcom, wakeup);
+ ret = dwc3_qcom_resume(qcom, PMSG_RESUME);
if (ret)
return ret;
@@ -819,7 +837,7 @@ static int dwc3_qcom_runtime_suspend(struct device *dev)
if (ret)
return ret;
- return dwc3_qcom_suspend(qcom, true);
+ return dwc3_qcom_suspend(qcom, PMSG_AUTO_SUSPEND);
}
static int dwc3_qcom_runtime_resume(struct device *dev)
@@ -828,7 +846,7 @@ static int dwc3_qcom_runtime_resume(struct device *dev)
struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
int ret;
- ret = dwc3_qcom_resume(qcom, true);
+ ret = dwc3_qcom_resume(qcom, PMSG_AUTO_RESUME);
if (ret)
return ret;
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 3/7] dt-bindings: usb: Add qcom,sa8255p-dwc3 for firmware-managed resources
2026-09-03 15:38 [PATCH v2 0/7] usb: dwc3: qcom: Add firmware-managed resource support for SA8255P Faisal Hassan
2026-09-03 15:38 ` [PATCH v2 1/7] usb: dwc3: qcom: re-enable wakeup interrupts on failed resume Faisal Hassan
2026-09-03 15:38 ` [PATCH v2 2/7] usb: dwc3: qcom: Distinguish PM and runtime suspend/resume paths Faisal Hassan
@ 2026-09-03 15:38 ` Faisal Hassan
2026-09-03 15:56 ` sashiko-bot
2026-09-03 15:38 ` [PATCH v2 4/7] usb: host: xhci-plat: Support XHCI_RESET_ON_RESUME via device property Faisal Hassan
` (3 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Faisal Hassan @ 2026-09-03 15:38 UTC (permalink / raw)
To: Thinh Nguyen, Greg Kroah-Hartman, Mathias Nyman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Wesley Cheng
Cc: Sriram Dash, Jack Pham, Krishna Kurapati, Shazad Hussain,
linux-arm-msm, linux-usb, devicetree, linux-kernel
On Qualcomm automotive SoC sa8255p, platform resources like clocks,
interconnects, resets, regulators and GDSC are abstracted and managed
collectively by firmware.
Add a new binding file for firmware-managed USB controllers where
access to these resources is exposed as two power domains:
- Power domain: Controls power supply (GDSC) to the USB controller
- Bus domain: Controls clocks and interconnects for data transfer
The binding makes power-domains and power-domain-names properties mandatory
for firmware-managed platforms, while excluding the clocks property since
clocks are managed through the power domains.
Co-developed-by: Sriram Dash <sriram.dash@oss.qualcomm.com>
Signed-off-by: Sriram Dash <sriram.dash@oss.qualcomm.com>
Signed-off-by: Faisal Hassan <faisal.hassan@oss.qualcomm.com>
---
.../bindings/usb/qcom,sa8255p-dwc3.yaml | 123 ++++++++++++++++++
1 file changed, 123 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/qcom,sa8255p-dwc3.yaml
diff --git a/Documentation/devicetree/bindings/usb/qcom,sa8255p-dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,sa8255p-dwc3.yaml
new file mode 100644
index 000000000000..c6db7bcf4981
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/qcom,sa8255p-dwc3.yaml
@@ -0,0 +1,123 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/qcom,sa8255p-dwc3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm SA8255P USB3 Controller with Firmware-Managed Resources
+
+maintainers:
+ - Wesley Cheng <wesley.cheng@oss.qualcomm.com>
+ - Faisal Hassan <faisal.hassan@oss.qualcomm.com>
+
+description:
+ USB controller on Qualcomm SA8255P automotive SoC where platform resources
+ such as clocks, interconnects, resets, regulators and the GDSC are
+ abstracted and managed collectively by firmware through SCMI (System
+ Control and Management Interface).
+
+ Access to these firmware-managed resources is exposed as two power
+ domains, one gating the power supply (GDSC) to the controller, and one
+ gating the clocks and interconnects used for data transfer.
+
+properties:
+ compatible:
+ const: qcom,sa8255p-dwc3
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ minItems: 1
+ maxItems: 6
+
+ interrupt-names:
+ minItems: 1
+ maxItems: 6
+
+ power-domains:
+ description: |
+ Power domains are provided by SCMI (System Control and Management
+ Interface). See Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+ for details.
+
+ Exactly two power domains must be specified:
+ - "power": controls power supply (GDSC) to the USB controller
+ - "bus": controls clocks and interconnects used for data transfer
+ minItems: 2
+ maxItems: 2
+
+ power-domain-names:
+ description: Names for the power domains specified in the power-domains property.
+ items:
+ - const: power
+ - const: bus
+
+ resets:
+ description: |
+ Reset provided by SCMI firmware. See
+ Documentation/devicetree/bindings/firmware/arm,scmi.yaml for details.
+ maxItems: 1
+
+ iommus:
+ maxItems: 1
+
+ phys:
+ minItems: 1
+ maxItems: 2
+
+ phy-names:
+ minItems: 1
+ maxItems: 2
+
+ # Firmware-managed platforms do not use direct clock/interconnect control
+ clocks: false
+ clock-names: false
+ interconnects: false
+ interconnect-names: false
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - interrupt-names
+ - power-domains
+ - power-domain-names
+ - resets
+
+allOf:
+ - $ref: snps,dwc3-common.yaml#
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ usb@a800000 {
+ compatible = "qcom,sa8255p-dwc3";
+ reg = <0x0a800000 0x10000>;
+
+ interrupts-extended = <&intc GIC_SPI 349 IRQ_TYPE_LEVEL_HIGH>,
+ <&intc GIC_SPI 352 IRQ_TYPE_LEVEL_HIGH>,
+ <&intc GIC_SPI 351 IRQ_TYPE_LEVEL_HIGH>,
+ <&pdc 8 IRQ_TYPE_EDGE_BOTH>,
+ <&pdc 7 IRQ_TYPE_EDGE_BOTH>,
+ <&pdc 13 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "dwc_usb3",
+ "pwr_event",
+ "hs_phy_irq",
+ "dp_hs_phy_irq",
+ "dm_hs_phy_irq",
+ "ss_phy_irq";
+
+ power-domains = <&scmi_pd 1>, <&scmi_pd 2>;
+ power-domain-names = "power", "bus";
+ resets = <&scmi_rst 0>;
+
+ iommus = <&apps_smmu 0x0a0 0x0>;
+ phys = <&usb_1_hsphy>, <&usb_1_qmpphy>;
+ phy-names = "usb2-phy", "usb3-phy";
+ };
+...
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 3/7] dt-bindings: usb: Add qcom,sa8255p-dwc3 for firmware-managed resources
2026-09-03 15:38 ` [PATCH v2 3/7] dt-bindings: usb: Add qcom,sa8255p-dwc3 for firmware-managed resources Faisal Hassan
@ 2026-09-03 15:56 ` sashiko-bot
0 siblings, 0 replies; 14+ messages in thread
From: sashiko-bot @ 2026-09-03 15:56 UTC (permalink / raw)
To: Faisal Hassan; +Cc: conor+dt, devicetree, robh
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The 'dma-coherent' property is missing from the binding, preventing it from being used in device trees.
- [Low] The 'interrupt-names' property lacks item constraints (e.g., 'items' or 'enum' lists), allowing arbitrary strings (like typos) to pass validation.
--
commit 5d3262b4959573620da7ce43d432ab2890209041
Author: Faisal Hassan <faisal.hassan@oss.qualcomm.com>
dt-bindings: usb: Add qcom,sa8255p-dwc3 for firmware-managed resources
This commit adds a new device tree binding for the USB controller on the
Qualcomm SA8255P automotive SoC. It specifies that platform resources
such as clocks and interconnects are abstracted and managed collectively by
firmware through two power domains.
> diff --git a/Documentation/devicetree/bindings/usb/qcom,sa8255p-dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,sa8255p-dwc3.yaml
> new file mode 100644
> index 0000000000000..c6db7bcf4981e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/qcom,sa8255p-dwc3.yaml
[ ... ]
> + interrupts:
> + minItems: 1
> + maxItems: 6
> +
> + interrupt-names:
> + minItems: 1
> + maxItems: 6
[Severity: Low]
Does interrupt-names need an items list or enum to constrain the exact string
names?
Without an explicit list, the schema will accept any arbitrary strings, which
prevents dtbs_check from catching typos during validation.
[ ... ]
> +allOf:
> + - $ref: snps,dwc3-common.yaml#
> +
> +unevaluatedProperties: false
[Severity: Medium]
Since unevaluatedProperties is false, does dma-coherent need to be explicitly
defined in this binding?
Because dma-coherent is omitted here and in the referenced
snps,dwc3-common.yaml, adding it to a device tree will cause dtbs_check
validation to fail.
If device tree authors omit the property to appease the validator, could the
kernel fall back to non-coherent DMA mappings, resulting in unnecessary cache
maintenance overhead and a performance regression on coherent hardware?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260903153827.3463313-1-faisal.hassan@oss.qualcomm.com?part=3
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 4/7] usb: host: xhci-plat: Support XHCI_RESET_ON_RESUME via device property
2026-09-03 15:38 [PATCH v2 0/7] usb: dwc3: qcom: Add firmware-managed resource support for SA8255P Faisal Hassan
` (2 preceding siblings ...)
2026-09-03 15:38 ` [PATCH v2 3/7] dt-bindings: usb: Add qcom,sa8255p-dwc3 for firmware-managed resources Faisal Hassan
@ 2026-09-03 15:38 ` Faisal Hassan
2026-09-03 15:38 ` [PATCH v2 5/7] usb: dwc3: add xhci-reset-on-resume property for power-loss resume Faisal Hassan
` (2 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Faisal Hassan @ 2026-09-03 15:38 UTC (permalink / raw)
To: Thinh Nguyen, Greg Kroah-Hartman, Mathias Nyman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Wesley Cheng
Cc: Sriram Dash, Jack Pham, Krishna Kurapati, Shazad Hussain,
linux-arm-msm, linux-usb, devicetree, linux-kernel
When the USB controller loses power during PM suspend (as occurs
on targets like SA8255P that fully power off the controller),
it generates a Host Controller Error (HCE) on resume requiring
full reinitialization. This is handled by the XHCI_RESET_ON_RESUME
quirk (introduced upstream in 8b328f8002bc).
Read the "xhci-reset-on-resume-quirk" device property to set
XHCI_RESET_ON_RESUME. This allows parent drivers (e.g., dwc3)
to enable the quirk via software nodes.
Co-developed-by: Sriram Dash <sriram.dash@oss.qualcomm.com>
Signed-off-by: Sriram Dash <sriram.dash@oss.qualcomm.com>
Signed-off-by: Faisal Hassan <faisal.hassan@oss.qualcomm.com>
---
drivers/usb/host/xhci-plat.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 074d9c731639..7df2947d13dc 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -277,6 +277,9 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s
if (device_property_read_bool(tmpdev, "xhci-skip-phy-init-quirk"))
xhci->quirks |= XHCI_SKIP_PHY_INIT;
+ if (device_property_read_bool(tmpdev, "xhci-reset-on-resume-quirk"))
+ xhci->quirks |= XHCI_RESET_ON_RESUME;
+
device_property_read_u32(tmpdev, "imod-interval-ns",
&xhci->imod_interval);
device_property_read_u16(tmpdev, "num-hc-interrupters",
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 5/7] usb: dwc3: add xhci-reset-on-resume property for power-loss resume
2026-09-03 15:38 [PATCH v2 0/7] usb: dwc3: qcom: Add firmware-managed resource support for SA8255P Faisal Hassan
` (3 preceding siblings ...)
2026-09-03 15:38 ` [PATCH v2 4/7] usb: host: xhci-plat: Support XHCI_RESET_ON_RESUME via device property Faisal Hassan
@ 2026-09-03 15:38 ` Faisal Hassan
2026-09-03 16:08 ` sashiko-bot
2026-09-04 6:09 ` Krishna Kurapati
2026-09-03 15:38 ` [PATCH v2 6/7] usb: dwc3: qcom: Support broken suspend for SA8255P Faisal Hassan
2026-09-03 15:38 ` [PATCH v2 7/7] usb: dwc3: qcom: Support firmware-managed resource states for power management Faisal Hassan
6 siblings, 2 replies; 14+ messages in thread
From: Faisal Hassan @ 2026-09-03 15:38 UTC (permalink / raw)
To: Thinh Nguyen, Greg Kroah-Hartman, Mathias Nyman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Wesley Cheng
Cc: Sriram Dash, Jack Pham, Krishna Kurapati, Shazad Hussain,
linux-arm-msm, linux-usb, devicetree, linux-kernel
On platforms where the USB controller loses power during system
suspend, like SA8255P, resume triggers a Host Controller Error
requiring full reinitialization.
The xhci driver supports XHCI_RESET_ON_RESUME to handle this,
but dwc3 previously lacked platform-specific quirk configuration.
Add a device property "xhci-reset-on-resume" that enables this
quirk for controllers needing complete reinitialization after
power loss during suspend.
Let dwc3 manage the quirk for the usb controller using
software properties.
Co-developed-by: Sriram Dash <sriram.dash@oss.qualcomm.com>
Signed-off-by: Sriram Dash <sriram.dash@oss.qualcomm.com>
Signed-off-by: Faisal Hassan <faisal.hassan@oss.qualcomm.com>
---
drivers/usb/dwc3/core.c | 4 ++++
drivers/usb/dwc3/core.h | 5 +++++
drivers/usb/dwc3/host.c | 5 ++++-
3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index fd5c2cd36c59..eb6ffd5148a7 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1712,6 +1712,10 @@ static void dwc3_get_software_properties(struct dwc3 *dwc,
&gsbuscfg0_reqinfo);
if (!ret)
dwc->gsbuscfg0_reqinfo = gsbuscfg0_reqinfo;
+
+ if (!dwc->xhci_reset_on_resume &&
+ device_property_read_bool(tmpdev, "xhci-reset-on-resume"))
+ dwc->xhci_reset_on_resume = true;
}
}
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 608daeb7ef10..0ff23eb217bc 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1172,6 +1172,10 @@ struct dwc3_glue_ops {
* @suspended: set to track suspend event due to U3/L2.
* @susphy_state: state of DWC3_GUSB2PHYCFG_SUSPHY + DWC3_GUSB3PIPECTL_SUSPHY
* before PM suspend.
+ * @xhci_reset_on_resume: Enable XHCI_RESET_ON_RESUME quirk for the xHCI
+ * host controller. Set to true for platforms where
+ * the USB controller loses power during system suspend,
+ * requiring complete reinitialization on resume.
* @imod_interval: set the interrupt moderation interval in 250ns
* increments or 0 to disable.
* @max_cfg_eps: current max number of IN eps used across all USB configs.
@@ -1420,6 +1424,7 @@ struct dwc3 {
unsigned wakeup_configured:1;
unsigned suspended:1;
unsigned susphy_state:1;
+ unsigned xhci_reset_on_resume:1;
u16 imod_interval;
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index 96b588bd08cd..2170fcafc889 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -129,7 +129,7 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
int dwc3_host_init(struct dwc3 *dwc)
{
- struct property_entry props[6];
+ struct property_entry props[7];
struct platform_device *xhci;
int ret, irq;
int prop_idx = 0;
@@ -173,6 +173,9 @@ int dwc3_host_init(struct dwc3 *dwc)
if (dwc->usb2_lpm_disable)
props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");
+ if (dwc->xhci_reset_on_resume)
+ props[prop_idx++] = PROPERTY_ENTRY_BOOL("xhci-reset-on-resume-quirk");
+
/**
* WORKAROUND: dwc3 revisions <=3.00a have a limitation
* where Port Disable command doesn't work.
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 5/7] usb: dwc3: add xhci-reset-on-resume property for power-loss resume
2026-09-03 15:38 ` [PATCH v2 5/7] usb: dwc3: add xhci-reset-on-resume property for power-loss resume Faisal Hassan
@ 2026-09-03 16:08 ` sashiko-bot
2026-09-04 6:09 ` Krishna Kurapati
1 sibling, 0 replies; 14+ messages in thread
From: sashiko-bot @ 2026-09-03 16:08 UTC (permalink / raw)
To: Faisal Hassan; +Cc: conor+dt, robh, devicetree
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Out-of-bounds stack read in dwc3_host_init due to missing NULL terminator in props array
- [High] Use-After-Free due to dangling pointer when xHCI initialization fails
--
commit a0d50bed63a440d17232b616205b33080bdd0057
Author: Faisal Hassan <faisal.hassan@oss.qualcomm.com>
usb: dwc3: add xhci-reset-on-resume property for power-loss resume
This commit adds a new device property "xhci-reset-on-resume" that enables
the XHCI_RESET_ON_RESUME quirk for controllers needing complete
reinitialization after power loss during system suspend. It also updates
the dwc3 core to manage this quirk via software properties.
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index c5674161b2b08..1402520140862 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -130,7 +130,7 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
>
> int dwc3_host_init(struct dwc3 *dwc)
> {
> - struct property_entry props[6];
> + struct property_entry props[7];
[Severity: High]
This is a pre-existing issue, but does this array need an extra element for
a NULL terminator?
If all the conditional properties evaluate to true (usb3-lpm-capable,
usb2-lpm-disable, xhci-reset-on-resume-quirk, and quirk-broken-port-ped),
prop_idx reaches 7 and all 7 elements are populated.
When device_create_managed_software_node() copies the properties, it iterates
until it encounters a NULL name field.
Since there is no zero-initialized sentinel element at the end of the array,
could this lead to an out-of-bounds read on the stack?
> struct platform_device *xhci;
> int ret, irq;
> int prop_idx = 0;
[ ... ]
> @@ -174,6 +174,9 @@ int dwc3_host_init(struct dwc3 *dwc)
> if (dwc->usb2_lpm_disable)
> props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");
>
> + if (dwc->xhci_reset_on_resume)
> + props[prop_idx++] = PROPERTY_ENTRY_BOOL("xhci-reset-on-resume-quirk");
> +
[Severity: High]
This isn't a bug introduced by this patch, but does the error handling in
dwc3_host_init() leave a dangling pointer?
Earlier in dwc3_host_init(), the allocated device is assigned to dwc->xhci:
dwc->xhci = xhci;
If a subsequent initialization step fails, the error path frees the device:
err:
platform_device_put(xhci);
return ret;
However, dwc->xhci is never cleared to NULL. When a subsequent role switch or
teardown occurs, dwc3_host_exit() dereferences this freed pointer and calls
platform_device_unregister(dwc->xhci).
Could this lead to a use-after-free if memory allocation or initialization
fails during a USB role switch?
> /**
> * WORKAROUND: dwc3 revisions <=3.00a have a limitation
> * where Port Disable command doesn't work.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260903153827.3463313-1-faisal.hassan@oss.qualcomm.com?part=5
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2 5/7] usb: dwc3: add xhci-reset-on-resume property for power-loss resume
2026-09-03 15:38 ` [PATCH v2 5/7] usb: dwc3: add xhci-reset-on-resume property for power-loss resume Faisal Hassan
2026-09-03 16:08 ` sashiko-bot
@ 2026-09-04 6:09 ` Krishna Kurapati
1 sibling, 0 replies; 14+ messages in thread
From: Krishna Kurapati @ 2026-09-04 6:09 UTC (permalink / raw)
To: Faisal Hassan, Thinh Nguyen
Cc: Sriram Dash, Jack Pham, Shazad Hussain, linux-arm-msm, linux-usb,
devicetree, linux-kernel, Greg Kroah-Hartman, Mathias Nyman,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Philipp Zabel,
Wesley Cheng
On 9/3/2026 9:08 PM, Faisal Hassan wrote:
> On platforms where the USB controller loses power during system
> suspend, like SA8255P, resume triggers a Host Controller Error
> requiring full reinitialization.
>
> The xhci driver supports XHCI_RESET_ON_RESUME to handle this,
> but dwc3 previously lacked platform-specific quirk configuration.
> Add a device property "xhci-reset-on-resume" that enables this
> quirk for controllers needing complete reinitialization after
> power loss during suspend.
>
> Let dwc3 manage the quirk for the usb controller using
> software properties.
>
> Co-developed-by: Sriram Dash <sriram.dash@oss.qualcomm.com>
> Signed-off-by: Sriram Dash <sriram.dash@oss.qualcomm.com>
> Signed-off-by: Faisal Hassan <faisal.hassan@oss.qualcomm.com>
> ---
> drivers/usb/dwc3/core.c | 4 ++++
> drivers/usb/dwc3/core.h | 5 +++++
> drivers/usb/dwc3/host.c | 5 ++++-
> 3 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index fd5c2cd36c59..eb6ffd5148a7 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1712,6 +1712,10 @@ static void dwc3_get_software_properties(struct dwc3 *dwc,
> &gsbuscfg0_reqinfo);
> if (!ret)
> dwc->gsbuscfg0_reqinfo = gsbuscfg0_reqinfo;
> +
> + if (!dwc->xhci_reset_on_resume &&
> + device_property_read_bool(tmpdev, "xhci-reset-on-resume"))
> + dwc->xhci_reset_on_resume = true;
> }
xHCI plat does look for properties in xhci device node as well as the
ones above it in the parent chain:
"
/* Iterate over all parent nodes for finding quirks */
for (tmpdev = &pdev->dev; tmpdev; tmpdev = tmpdev->parent) {
"
So adding device_add_software_node in dwc3_qcom must be sufficient.
Regards,
Krishna,
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 6/7] usb: dwc3: qcom: Support broken suspend for SA8255P
2026-09-03 15:38 [PATCH v2 0/7] usb: dwc3: qcom: Add firmware-managed resource support for SA8255P Faisal Hassan
` (4 preceding siblings ...)
2026-09-03 15:38 ` [PATCH v2 5/7] usb: dwc3: add xhci-reset-on-resume property for power-loss resume Faisal Hassan
@ 2026-09-03 15:38 ` Faisal Hassan
2026-09-03 16:13 ` sashiko-bot
2026-09-03 15:38 ` [PATCH v2 7/7] usb: dwc3: qcom: Support firmware-managed resource states for power management Faisal Hassan
6 siblings, 1 reply; 14+ messages in thread
From: Faisal Hassan @ 2026-09-03 15:38 UTC (permalink / raw)
To: Thinh Nguyen, Greg Kroah-Hartman, Mathias Nyman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Wesley Cheng
Cc: Sriram Dash, Jack Pham, Krishna Kurapati, Shazad Hussain,
linux-arm-msm, linux-usb, devicetree, linux-kernel
During APPS Suspend, the clock controller unit disables all clocks
to the CPU and subsystems, causing the controller to enter the
Power-On Reset (POR) state. This results in the loss of
Qscratch programming.
Upon APPS Resume, the USB controller does not have the proper
PIPE_UTMI_CLK_SEL or VBUS_VALID. Hence, if only the High-Speed
PHY is used for a controller capable of SuperSpeed operations,
the controller will assume it should use the USB3 PHY and not
the HS PHY as configured before APPS Suspend. This behavior
disrupts the normal operation of the USB controller. Similarly,
if the controller was operating in device mode prior to
APPS suspend, and VBUS_VALID signal is not set after APPS resume,
this will cause the PHYs to transition into the suspend state.
This behavior disrupts the normal operation of the USB controller.
Address the issues by ensuring that the PIPE_UTMI_CLK_SEL and
VBUS_VALID are handled properly after the system resumes from
APPS Suspend. This is crucial for maintaining the correct
operational state of the USB controller and preventing
unintended suspensions of the PHYs.
Also, as the controller is starting from POR state, make
sure the controller Reset is performed.
Co-developed-by: Sriram Dash <sriram.dash@oss.qualcomm.com>
Signed-off-by: Sriram Dash <sriram.dash@oss.qualcomm.com>
Signed-off-by: Faisal Hassan <faisal.hassan@oss.qualcomm.com>
---
drivers/usb/dwc3/dwc3-qcom.c | 128 +++++++++++++++++++++++++++--------
1 file changed, 100 insertions(+), 28 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 6d25f81800a7..e58a9ca23b00 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -68,6 +68,10 @@ struct dwc3_qcom_port {
enum usb_device_speed usb2_speed;
};
+struct dwc3_qcom_priv_data {
+ bool broken_suspend;
+};
+
struct dwc3_qcom {
struct device *dev;
void __iomem *qscratch_base;
@@ -85,6 +89,13 @@ struct dwc3_qcom {
struct icc_path *icc_path_apps;
enum usb_role current_role;
+
+ bool broken_suspend;
+ bool ignore_pipe_clk;
+};
+
+static const struct dwc3_qcom_priv_data sa8255p_dwc3_qcom_priv_data = {
+ .broken_suspend = true,
};
#define to_dwc3_qcom(d) container_of((d), struct dwc3_qcom, dwc)
@@ -335,6 +346,23 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
dwc3_qcom_enable_port_interrupts(&qcom->ports[i]);
}
+static void dwc3_qcom_select_utmi_clk(struct dwc3_qcom *qcom)
+{
+ /* Configure dwc3 to use UTMI clock as PIPE clock not present */
+ dwc3_qcom_setbits(qcom->qscratch_base, QSCRATCH_GENERAL_CFG,
+ PIPE_UTMI_CLK_DIS);
+
+ usleep_range(100, 1000);
+
+ dwc3_qcom_setbits(qcom->qscratch_base, QSCRATCH_GENERAL_CFG,
+ PIPE_UTMI_CLK_SEL | PIPE3_PHYSTATUS_SW);
+
+ usleep_range(100, 1000);
+
+ dwc3_qcom_clrbits(qcom->qscratch_base, QSCRATCH_GENERAL_CFG,
+ PIPE_UTMI_CLK_DIS);
+}
+
static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, pm_message_t msg)
{
u32 val;
@@ -388,6 +416,28 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, pm_message_t msg)
if (!qcom->is_suspended)
return 0;
+ if (qcom->broken_suspend && !PMSG_IS_AUTO(msg)) {
+ /*
+ * Only system suspend fully powers off the controller and
+ * puts it into POR state. Runtime suspend does not, so skip
+ * the reset on runtime resume to avoid needlessly clobbering
+ * state that was never lost.
+ */
+ ret = reset_control_assert(qcom->resets);
+ if (ret) {
+ dev_err(qcom->dev, "failed to assert resets, err=%d\n", ret);
+ return ret;
+ }
+
+ usleep_range(10, 1000);
+
+ ret = reset_control_deassert(qcom->resets);
+ if (ret) {
+ dev_err(qcom->dev, "failed to deassert resets, err=%d\n", ret);
+ return ret;
+ }
+ }
+
/*
* For runtime resume, always assume wakeup was enabled.
* For system resume, check device wakeup capability.
@@ -415,6 +465,14 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, pm_message_t msg)
PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);
}
+ if (qcom->broken_suspend) {
+ if (!wakeup && qcom->ignore_pipe_clk)
+ dwc3_qcom_select_utmi_clk(qcom);
+ /* Make sure vbus valid is set for PHYs after PM resume */
+ if (!(dwc3_qcom_is_host(qcom) && wakeup))
+ dwc3_qcom_vbus_override_enable(qcom, true);
+ }
+
qcom->is_suspended = false;
return 0;
@@ -449,23 +507,6 @@ static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
return IRQ_HANDLED;
}
-static void dwc3_qcom_select_utmi_clk(struct dwc3_qcom *qcom)
-{
- /* Configure dwc3 to use UTMI clock as PIPE clock not present */
- dwc3_qcom_setbits(qcom->qscratch_base, QSCRATCH_GENERAL_CFG,
- PIPE_UTMI_CLK_DIS);
-
- usleep_range(100, 1000);
-
- dwc3_qcom_setbits(qcom->qscratch_base, QSCRATCH_GENERAL_CFG,
- PIPE_UTMI_CLK_SEL | PIPE3_PHYSTATUS_SW);
-
- usleep_range(100, 1000);
-
- dwc3_qcom_clrbits(qcom->qscratch_base, QSCRATCH_GENERAL_CFG,
- PIPE_UTMI_CLK_DIS);
-}
-
static int dwc3_qcom_request_irq(struct dwc3_qcom *qcom, int irq,
const char *name)
{
@@ -637,6 +678,15 @@ static struct dwc3_glue_ops dwc3_qcom_glue_ops = {
.pre_run_stop = dwc3_qcom_run_stop_notifier,
};
+static const struct property_entry dwc3_qcom_props_broken_suspend[] = {
+ PROPERTY_ENTRY_BOOL("xhci-reset-on-resume"),
+ { }
+};
+
+static const struct software_node dwc3_qcom_swnode_prop_broken_suspend = {
+ .properties = dwc3_qcom_props_broken_suspend,
+};
+
static int dwc3_qcom_probe(struct platform_device *pdev)
{
struct dwc3_probe_data probe_data = {};
@@ -644,8 +694,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
struct dwc3_qcom *qcom;
struct resource res;
struct resource *r;
+ const struct dwc3_qcom_priv_data *priv_data;
int ret;
- bool ignore_pipe_clk;
bool wakeup_source;
qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL);
@@ -654,21 +704,33 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
qcom->dev = &pdev->dev;
+ priv_data = of_device_get_match_data(dev);
+ if (priv_data && priv_data->broken_suspend) {
+ ret = device_add_software_node(&pdev->dev,
+ &dwc3_qcom_swnode_prop_broken_suspend);
+ if (ret)
+ return ret;
+ qcom->broken_suspend = priv_data->broken_suspend;
+ }
+
qcom->resets = devm_reset_control_array_get_optional_exclusive(dev);
if (IS_ERR(qcom->resets)) {
- return dev_err_probe(&pdev->dev, PTR_ERR(qcom->resets),
- "failed to get resets\n");
+ ret = dev_err_probe(&pdev->dev, PTR_ERR(qcom->resets),
+ "failed to get resets\n");
+ goto err_remove_swnode;
}
ret = devm_clk_bulk_get_all(&pdev->dev, &qcom->clks);
- if (ret < 0)
- return dev_err_probe(dev, ret, "failed to get clocks\n");
+ if (ret < 0) {
+ dev_err_probe(dev, ret, "failed to get clocks\n");
+ goto err_remove_swnode;
+ }
qcom->num_clocks = ret;
ret = reset_control_assert(qcom->resets);
if (ret) {
dev_err(&pdev->dev, "failed to assert resets, err=%d\n", ret);
- return ret;
+ goto err_remove_swnode;
}
usleep_range(10, 1000);
@@ -676,12 +738,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
ret = reset_control_deassert(qcom->resets);
if (ret) {
dev_err(&pdev->dev, "failed to deassert resets, err=%d\n", ret);
- return ret;
+ goto err_remove_swnode;
}
ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
if (ret < 0)
- return ret;
+ goto err_remove_swnode;
r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!r) {
@@ -708,9 +770,9 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
* Disable pipe_clk requirement if specified. Used when dwc3
* operates without SSPHY and only HS/FS/LS modes are supported.
*/
- ignore_pipe_clk = device_property_read_bool(dev,
- "qcom,select-utmi-as-pipe-clk");
- if (ignore_pipe_clk)
+ qcom->ignore_pipe_clk = device_property_read_bool(dev,
+ "qcom,select-utmi-as-pipe-clk");
+ if (qcom->ignore_pipe_clk)
dwc3_qcom_select_utmi_clk(qcom);
qcom->mode = usb_get_dr_mode(dev);
@@ -757,6 +819,9 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
clk_disable:
clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
+err_remove_swnode:
+ if (qcom->broken_suspend)
+ device_remove_software_node(&pdev->dev);
return ret;
}
@@ -773,6 +838,9 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
dwc3_qcom_interconnect_exit(qcom);
pm_runtime_put_noidle(qcom->dev);
+
+ if (qcom->broken_suspend)
+ device_remove_software_node(&pdev->dev);
}
static int dwc3_qcom_pm_suspend(struct device *dev)
@@ -867,6 +935,10 @@ static const struct dev_pm_ops dwc3_qcom_dev_pm_ops = {
};
static const struct of_device_id dwc3_qcom_of_match[] = {
+ {
+ .compatible = "qcom,sa8255p-dwc3",
+ .data = &sa8255p_dwc3_qcom_priv_data,
+ },
{ .compatible = "qcom,snps-dwc3" },
{ }
};
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 6/7] usb: dwc3: qcom: Support broken suspend for SA8255P
2026-09-03 15:38 ` [PATCH v2 6/7] usb: dwc3: qcom: Support broken suspend for SA8255P Faisal Hassan
@ 2026-09-03 16:13 ` sashiko-bot
0 siblings, 0 replies; 14+ messages in thread
From: sashiko-bot @ 2026-09-03 16:13 UTC (permalink / raw)
To: Faisal Hassan; +Cc: devicetree, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Qscratch programming is not restored during system resume if device wakeup is enabled, leaving the controller broken.
- [High] Flawed boolean logic enables device-mode VBUS override during host mode operations.
--
commit 62abc776630f8d66e2fc8e0f35ca6edc55d04a4d
Author: Faisal Hassan <faisal.hassan@oss.qualcomm.com>
usb: dwc3: qcom: Support broken suspend for SA8255P
This commit addresses Qscratch programming loss on SA8255P during APPS Suspend
by restoring UTMI clock selection and VBUS valid signals after system resume.
It also ensures the controller is properly reset from the POR state.
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 6d25f81800a7f..e58a9ca23b009 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
[ ... ]
> @@ -388,6 +416,28 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, pm_message_t msg)
> if (!qcom->is_suspended)
> return 0;
>
> + if (qcom->broken_suspend && !PMSG_IS_AUTO(msg)) {
> + /*
> + * Only system suspend fully powers off the controller and
> + * puts it into POR state. Runtime suspend does not, so skip
> + * the reset on runtime resume to avoid needlessly clobbering
> + * state that was never lost.
> + */
> + ret = reset_control_assert(qcom->resets);
[Severity: High]
Does this unconditionally reset the controller during system resume,
destroying Qscratch state regardless of whether device wakeup is enabled?
[ ... ]
> @@ -415,6 +465,14 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, pm_message_t msg)
> PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);
> }
>
> + if (qcom->broken_suspend) {
> + if (!wakeup && qcom->ignore_pipe_clk)
> + dwc3_qcom_select_utmi_clk(qcom);
[Severity: High]
Could this logic improperly skip required Qscratch reprogramming after the
controller reset if wakeup is enabled? Since the reset wiped the state above,
the clock configuration shouldn't be skipped when wakeup is true, as it
breaks USB operations on resume.
> + /* Make sure vbus valid is set for PHYs after PM resume */
> + if (!(dwc3_qcom_is_host(qcom) && wakeup))
> + dwc3_qcom_vbus_override_enable(qcom, true);
[Severity: High]
Will this boolean logic accidentally enable device-mode VBUS override during
host mode operations? By De Morgan's laws, if dwc3_qcom_is_host() is true
and wakeup is false, this evaluates to true. Putting the PHY into device
mode VBUS override disrupts normal host mode operations.
> + }
> +
> qcom->is_suspended = false;
>
> return 0;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260903153827.3463313-1-faisal.hassan@oss.qualcomm.com?part=6
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 7/7] usb: dwc3: qcom: Support firmware-managed resource states for power management
2026-09-03 15:38 [PATCH v2 0/7] usb: dwc3: qcom: Add firmware-managed resource support for SA8255P Faisal Hassan
` (5 preceding siblings ...)
2026-09-03 15:38 ` [PATCH v2 6/7] usb: dwc3: qcom: Support broken suspend for SA8255P Faisal Hassan
@ 2026-09-03 15:38 ` Faisal Hassan
2026-09-03 16:14 ` sashiko-bot
6 siblings, 1 reply; 14+ messages in thread
From: Faisal Hassan @ 2026-09-03 15:38 UTC (permalink / raw)
To: Thinh Nguyen, Greg Kroah-Hartman, Mathias Nyman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Wesley Cheng
Cc: Sriram Dash, Jack Pham, Krishna Kurapati, Shazad Hussain,
linux-arm-msm, linux-usb, devicetree, linux-kernel
Add support for firmware-managed resource handling in the Qualcomm
DWC3 USB controller driver. On platforms such as SA8255P, controller
resources are abstracted and managed collectively by firmware, and are
exposed to Linux through SCMI-backed power domains.
On SA8255P, resources such as clocks, regulators, resets, TLMM and
interconnects are controlled by firmware. The driver communicates with
firmware through two power domains:
- "power": controls power supply (GDSC) to the USB controller
- "bus": controls clocks and interconnects used for data transfer.
Register accesses through the controller require the "power"
domain to be enabled.
The two domains are voted on and off directly from the PM callbacks
through dwc3_qcom_domains_get() and dwc3_qcom_domains_put(). Runtime
PM usage counting keeps each domain enabled for as long as it is
required; the driver does not attempt to track or infer any aggregate
device power state, nor does it assume what state firmware or the
bootloader left the domains in before probe.
Runtime suspend/resume vote only the "bus" domain, since the "power"
domain must remain enabled for hardware accesses that can occur while
runtime suspended. System suspend/resume vote both domains.
The domains are attached with PD_FLAG_NO_DEV_LINK because runtime
suspend and system suspend require different supplier states. Runtime
suspend releases only the "bus" domain while retaining the "power"
domain, whereas system suspend releases both domains. Ordinary
device-links would couple both suppliers to the same consumer runtime
PM state and therefore cannot express the required policy. The driver
consequently manages the runtime PM references for the attached domains
explicitly.
These votes are taken during probe, remove, suspend and resume,
enabling coordinated management of grouped resources according to the
controller's operational requirements.
Enable firmware-managed resource handling for the
"qcom,sa8255p-dwc3" compatible while maintaining backward
compatibility with existing platforms that use direct resource
control.
While here, harden dwc3_qcom_remove() and the probe error-unwind path:
disable runtime PM before tearing down so a concurrent PM operation
can't race the teardown, assert the controller reset and mark the
device runtime-suspended once cleanup completes, and keep going with
best-effort cleanup if the initial pm_runtime_resume_and_get() in
remove() fails instead of leaking. These paths are shared by every
qcom,snps-dwc3 platform, not only the firmware-managed ones added
here: the added reset_control_assert() and pm_runtime_set_suspended()
calls are unconditional. They are safe for existing platforms because
remove() already fully powers the controller down and probe failure
already leaves it unclocked, so putting the (shared) reset line into
assert and marking the device suspended only makes the visible runtime
PM state match reality more precisely than before.
Co-developed-by: Shazad Hussain <shazad.hussain@oss.qualcomm.com>
Signed-off-by: Shazad Hussain <shazad.hussain@oss.qualcomm.com>
Co-developed-by: Sriram Dash <sriram.dash@oss.qualcomm.com>
Signed-off-by: Sriram Dash <sriram.dash@oss.qualcomm.com>
Signed-off-by: Faisal Hassan <faisal.hassan@oss.qualcomm.com>
---
drivers/usb/dwc3/dwc3-qcom.c | 309 +++++++++++++++++++++++++++++++----
1 file changed, 281 insertions(+), 28 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index e58a9ca23b00..7139eadbfb1e 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -13,6 +13,7 @@
#include <linux/kernel.h>
#include <linux/interconnect.h>
#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
#include <linux/phy/phy.h>
#include <linux/usb/of.h>
#include <linux/reset.h>
@@ -70,6 +71,7 @@ struct dwc3_qcom_port {
struct dwc3_qcom_priv_data {
bool broken_suspend;
+ bool fw_managed;
};
struct dwc3_qcom {
@@ -92,14 +94,147 @@ struct dwc3_qcom {
bool broken_suspend;
bool ignore_pipe_clk;
+
+ bool fw_managed;
+ struct dev_pm_domain_list *pd_list;
};
static const struct dwc3_qcom_priv_data sa8255p_dwc3_qcom_priv_data = {
.broken_suspend = true,
+ .fw_managed = true,
};
#define to_dwc3_qcom(d) container_of((d), struct dwc3_qcom, dwc)
+/*
+ * Firmware-managed resource handling
+ *
+ * On platforms where clocks, interconnects, resets and the GDSC for the USB
+ * controller are owned and sequenced by firmware, access to those resources
+ * is exposed to Linux as two power domains:
+ * - pd_list->pd_devs[0] ("power"): the GDSC supplying the controller
+ * - pd_list->pd_devs[1] ("bus"): the clocks and interconnects used for data
+ * transfer, which additionally require "power" to be on for register
+ * accesses to succeed
+ *
+ * Reset signals are controlled separately through the reset control framework
+ * during probe/remove.
+ *
+ * The two domains are voted on/off directly from each PM callback that needs
+ * them; there's no attempt to track or name an aggregate device power state,
+ * since the actual state lives in firmware and is queried by voting through
+ * runtime PM, not by mirroring it in the driver.
+ */
+
+/**
+ * dwc3_qcom_domain_detach() - Detach power domains
+ * @qcom: Pointer to the dwc3_qcom structure
+ *
+ * Detaches all power domains.
+ */
+static void dwc3_qcom_domain_detach(struct dwc3_qcom *qcom)
+{
+ if (qcom->pd_list)
+ dev_pm_domain_detach_list(qcom->pd_list);
+}
+
+/**
+ * dwc3_qcom_domain_attach() - Attach power domains
+ * @qcom: Pointer to the dwc3_qcom structure
+ *
+ * Attaches power domains for firmware managed resource handling.
+ * Returns 0 on success, negative error code on failure.
+ */
+static int dwc3_qcom_domain_attach(struct dwc3_qcom *qcom)
+{
+ struct dev_pm_domain_attach_data pd_data = {
+ .pd_flags = PD_FLAG_NO_DEV_LINK,
+ .pd_names = (const char*[]) { "power", "bus" },
+ .num_pd_names = 2,
+ };
+ struct device *dev = qcom->dev;
+ int ret;
+
+ ret = dev_pm_domain_attach_list(dev, &pd_data, &qcom->pd_list);
+ if (ret != pd_data.num_pd_names) {
+ dev_err(dev, "domain attach failed (%d)\n", ret);
+ return ret < 0 ? ret : -ENODEV;
+ }
+
+ return 0;
+}
+
+/**
+ * dwc3_qcom_domains_get() - Vote the firmware-managed domains on
+ * @qcom: Pointer to the dwc3_qcom structure
+ * @bus_only: If true, leave the power domain untouched and only vote bus
+ *
+ * Votes runtime PM "on" for the domains this call is responsible for. Each
+ * call has exactly one matching dwc3_qcom_domains_put() call with the same
+ * @bus_only value on the corresponding disable path; runtime PM's own usage
+ * counting is what keeps power on for as long as bus needs it, without the
+ * driver tracking or assuming any prior domain state itself.
+ * Returns 0 on success, negative error code on failure.
+ */
+static int dwc3_qcom_domains_get(struct dwc3_qcom *qcom, bool bus_only)
+{
+ struct device *power_dev = qcom->pd_list->pd_devs[0];
+ struct device *bus_dev = qcom->pd_list->pd_devs[1];
+ int ret;
+
+ if (!bus_only) {
+ ret = pm_runtime_resume_and_get(power_dev);
+ if (ret) {
+ dev_err(qcom->dev, "failed to enable power domain: %d\n", ret);
+ return ret;
+ }
+ }
+
+ ret = pm_runtime_resume_and_get(bus_dev);
+ if (ret) {
+ dev_err(qcom->dev, "failed to enable bus domain: %d\n", ret);
+ if (!bus_only)
+ pm_runtime_put_sync(power_dev);
+ return ret;
+ }
+
+ return 0;
+}
+
+/**
+ * dwc3_qcom_domains_put() - Vote the firmware-managed domains off
+ * @qcom: Pointer to the dwc3_qcom structure
+ * @bus_only: If true, leave the power domain untouched and only vote bus
+ *
+ * Releases the votes taken by the matching dwc3_qcom_domains_get() call.
+ * Bus is released before power since bus register accesses require power
+ * to still be on.
+ * Returns 0 on success, negative error code on failure.
+ */
+static int dwc3_qcom_domains_put(struct dwc3_qcom *qcom, bool bus_only)
+{
+ struct device *power_dev = qcom->pd_list->pd_devs[0];
+ struct device *bus_dev = qcom->pd_list->pd_devs[1];
+ int ret;
+
+ ret = pm_runtime_put_sync(bus_dev);
+ if (ret < 0) {
+ dev_err(qcom->dev, "failed to disable bus domain: %d\n", ret);
+ return ret;
+ }
+
+ if (bus_only)
+ return 0;
+
+ ret = pm_runtime_put_sync(power_dev);
+ if (ret < 0) {
+ dev_err(qcom->dev, "failed to disable power domain: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
{
u32 reg;
@@ -386,11 +521,23 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, pm_message_t msg)
if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
dev_err(qcom->dev, "port-%d HS-PHY not in L2\n", i + 1);
}
- clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
- ret = dwc3_qcom_interconnect_disable(qcom);
- if (ret)
- dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
+ if (!qcom->fw_managed) {
+ clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
+
+ ret = dwc3_qcom_interconnect_disable(qcom);
+ if (ret)
+ dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
+ } else {
+ /*
+ * Runtime suspend only needs to drop the bus domain; power
+ * stays voted on so register accesses remain possible for
+ * whatever briefly resumes it. System suspend drops both.
+ */
+ ret = dwc3_qcom_domains_put(qcom, PMSG_IS_AUTO(msg));
+ if (ret)
+ return ret;
+ }
/*
* The role is stable during suspend as role switching is done from a
@@ -450,13 +597,25 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, pm_message_t msg)
if (dwc3_qcom_is_host(qcom) && wakeup)
dwc3_qcom_disable_interrupts(qcom);
- ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
- if (ret < 0)
- goto enable_irq;
+ if (!qcom->fw_managed) {
+ ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
+ if (ret < 0)
+ goto enable_irq;
- ret = dwc3_qcom_interconnect_enable(qcom);
- if (ret)
- dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
+ ret = dwc3_qcom_interconnect_enable(qcom);
+ if (ret)
+ dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
+ } else {
+ /*
+ * Runtime resume only needs to re-vote bus (power was left
+ * on across runtime suspend). System resume votes both.
+ */
+ ret = dwc3_qcom_domains_get(qcom, PMSG_IS_AUTO(msg));
+ if (ret) {
+ dev_err(qcom->dev, "failed to enable power domains: %d\n", ret);
+ goto enable_irq;
+ }
+ }
/* Clear existing events from PHY related to L2 in/out */
for (i = 0; i < qcom->num_ports; i++) {
@@ -713,6 +872,9 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
qcom->broken_suspend = priv_data->broken_suspend;
}
+ if (priv_data && priv_data->fw_managed)
+ qcom->fw_managed = priv_data->fw_managed;
+
qcom->resets = devm_reset_control_array_get_optional_exclusive(dev);
if (IS_ERR(qcom->resets)) {
ret = dev_err_probe(&pdev->dev, PTR_ERR(qcom->resets),
@@ -720,12 +882,14 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
goto err_remove_swnode;
}
- ret = devm_clk_bulk_get_all(&pdev->dev, &qcom->clks);
- if (ret < 0) {
- dev_err_probe(dev, ret, "failed to get clocks\n");
- goto err_remove_swnode;
+ if (!qcom->fw_managed) {
+ ret = devm_clk_bulk_get_all(&pdev->dev, &qcom->clks);
+ if (ret < 0) {
+ dev_err_probe(dev, ret, "failed to get clocks\n");
+ goto err_remove_swnode;
+ }
+ qcom->num_clocks = ret;
}
- qcom->num_clocks = ret;
ret = reset_control_assert(qcom->resets);
if (ret) {
@@ -741,9 +905,31 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
goto err_remove_swnode;
}
- ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
- if (ret < 0)
- goto err_remove_swnode;
+ if (!qcom->fw_managed) {
+ ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
+ if (ret < 0)
+ goto err_remove_swnode;
+ } else {
+ ret = dwc3_qcom_domain_attach(qcom);
+ if (ret) {
+ dev_err(dev, "Failed to attach domains (%d).\n", ret);
+ goto err_remove_swnode;
+ }
+
+ /*
+ * Vote both domains on for the remainder of probe. This is a
+ * plain runtime PM get, not an assumption about what state
+ * firmware or the bootloader left the domains in beforehand;
+ * pm_runtime_resume_and_get() is correct regardless of that
+ * prior state.
+ */
+ ret = dwc3_qcom_domains_get(qcom, false /* bus_only */);
+ if (ret) {
+ dev_err(dev, "Failed to enable power domains (%d)\n", ret);
+ dwc3_qcom_domain_detach(qcom);
+ goto err_remove_swnode;
+ }
+ }
r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!r) {
@@ -803,9 +989,15 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
goto clk_disable;
}
- ret = dwc3_qcom_interconnect_init(qcom);
- if (ret)
- goto remove_core;
+ /*
+ * Initialize interconnect paths only for non-firmware managed resource handling.
+ * In firmware managed resource handling, interconnects are controlled by power domains.
+ */
+ if (!qcom->fw_managed) {
+ ret = dwc3_qcom_interconnect_init(qcom);
+ if (ret)
+ goto remove_core;
+ }
wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
device_init_wakeup(&pdev->dev, wakeup_source);
@@ -817,7 +1009,20 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
remove_core:
dwc3_core_remove(&qcom->dwc);
clk_disable:
- clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
+ if (!qcom->fw_managed) {
+ clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
+ } else {
+ int cleanup_ret;
+
+ cleanup_ret = dwc3_qcom_domains_put(qcom, false /* bus_only */);
+ if (cleanup_ret)
+ dev_err(dev, "Failed to disable power domains during cleanup: %d\n",
+ cleanup_ret);
+ dwc3_qcom_domain_detach(qcom);
+ }
+
+ /* Assert reset on error */
+ reset_control_assert(qcom->resets);
err_remove_swnode:
if (qcom->broken_suspend)
@@ -829,15 +1034,63 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
{
struct dwc3 *dwc = platform_get_drvdata(pdev);
struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
+ bool pm_resumed = false;
+ bool domains_suspended = true;
+ int ret;
- if (pm_runtime_resume_and_get(qcom->dev) < 0)
- return;
+ ret = pm_runtime_resume_and_get(qcom->dev);
+ if (ret < 0)
+ dev_warn(qcom->dev, "Failed to resume. Perform critical cleanups only.\n");
+ else
+ pm_resumed = true;
- dwc3_core_remove(&qcom->dwc);
- clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
- dwc3_qcom_interconnect_exit(qcom);
+ /*
+ * Disable runtime PM to prevent any automatic PM operations during removal.
+ * This prevents race conditions where interrupt handlers or other subsystems
+ * might trigger runtime PM operations while the device is being torn down.
+ */
+ pm_runtime_disable(qcom->dev);
+
+ if (pm_resumed) {
+ /* Only perform operations that require device to be active */
+ dwc3_core_remove(&qcom->dwc);
+
+ if (!qcom->fw_managed) {
+ dwc3_qcom_interconnect_exit(qcom);
+ clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
+ } else {
+ ret = dwc3_qcom_domains_put(qcom, false /* bus_only */);
+ if (ret) {
+ dev_err(qcom->dev, "Failed to disable power domains during remove: %d\n",
+ ret);
+ domains_suspended = false;
+ }
+ }
+ }
- pm_runtime_put_noidle(qcom->dev);
+ /*
+ * Critical cleanup operations that must be performed even if
+ * pm_runtime_resume_and_get failed to prevent resource leaks
+ * and anomalies during reboot cycles.
+ */
+ if (qcom->fw_managed) {
+ /* Always detach power domains for firmware-managed case */
+ dwc3_qcom_domain_detach(qcom);
+ }
+
+ /* Always assert reset on remove */
+ reset_control_assert(qcom->resets);
+
+ if (pm_resumed)
+ pm_runtime_put_noidle(qcom->dev);
+
+ /*
+ * Only report the device as suspended if the domains were actually
+ * brought down; otherwise leave the runtime PM status untouched so
+ * it doesn't misreport hardware that may still be powered.
+ */
+ if (domains_suspended)
+ pm_runtime_set_suspended(qcom->dev);
if (qcom->broken_suspend)
device_remove_software_node(&pdev->dev);
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 7/7] usb: dwc3: qcom: Support firmware-managed resource states for power management
2026-09-03 15:38 ` [PATCH v2 7/7] usb: dwc3: qcom: Support firmware-managed resource states for power management Faisal Hassan
@ 2026-09-03 16:14 ` sashiko-bot
0 siblings, 0 replies; 14+ messages in thread
From: sashiko-bot @ 2026-09-03 16:14 UTC (permalink / raw)
To: Faisal Hassan; +Cc: robh, conor+dt, devicetree
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
New issues:
- [High] System suspend fails to release the 'power' domain if the device is already runtime suspended, causing a power leak.
- [High] Early return in dwc3_qcom_domains_put() on -EAGAIN leaks the 'power' domain vote and corrupts PM state.
- [High] Double pm_runtime_disable() in remove causes a permanent PM state corruption.
- [Medium] Probe error paths bypass the newly added reset_control_assert() hardening.
- [Medium] PM domain runtime usage count leak in the remove() error path.
Pre-existing issues:
- [High] Skipping dwc3_core_remove() on resume failure leaves child devices active, causing a Use-After-Free.
- [High] Hardware registers are accessed while the power domain is disabled, causing a synchronous external abort.
--
commit 322ab32f12f7861ac5e4f76813db99fd65efad09
Author: Faisal Hassan <faisal.hassan@oss.qualcomm.com>
usb: dwc3: qcom: Support firmware-managed resource states for power management
This commit adds support for firmware-managed resource handling in the
Qualcomm DWC3 USB controller driver via SCMI-backed power domains ("power"
and "bus"). It coordinates runtime and system suspend state through domain
voting and attempts to harden the driver's probe error-unwind and remove
paths.
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index e58a9ca23b009..7139eadbfb1e3 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
[ ... ]
> +static int dwc3_qcom_domains_put(struct dwc3_qcom *qcom, bool bus_only)
> +{
> + struct device *power_dev = qcom->pd_list->pd_devs[0];
> + struct device *bus_dev = qcom->pd_list->pd_devs[1];
> + int ret;
> +
> + ret = pm_runtime_put_sync(bus_dev);
> + if (ret < 0) {
> + dev_err(qcom->dev, "failed to disable bus domain: %d\n", ret);
> + return ret;
> + }
[Severity: High]
When pm_runtime_put_sync() is called for the bus domain, could it return
-EAGAIN if the device is kept active by another thread? If it does, this
early return treats it as a failure, skipping the power domain put and
propagating the error back to the PM core. The PM core aborts the suspend
and marks the device as active, but the bus domain usage counter was
already decremented. If the bus physically suspends later, does this lead
to accessing unclocked hardware and causing a synchronous external abort?
[ ... ]
> @@ -386,11 +521,23 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, pm_message_t msg)
> if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> dev_err(qcom->dev, "port-%d HS-PHY not in L2\n", i + 1);
> }
> - clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
>
> - ret = dwc3_qcom_interconnect_disable(qcom);
> - if (ret)
> - dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
> + if (!qcom->fw_managed) {
> + clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
> +
> + ret = dwc3_qcom_interconnect_disable(qcom);
> + if (ret)
> + dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
> + } else {
> + /*
> + * Runtime suspend only needs to drop the bus domain; power
> + * stays voted on so register accesses remain possible for
> + * whatever briefly resumes it. System suspend drops both.
> + */
> + ret = dwc3_qcom_domains_put(qcom, PMSG_IS_AUTO(msg));
> + if (ret)
> + return ret;
> + }
>
> /*
> * The role is stable during suspend as role switching is done from a
> @@ -450,13 +597,25 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, pm_message_t msg)
> if (dwc3_qcom_is_host(qcom) && wakeup) {
> for (i = 0; i < qcom->num_ports; i++)
> qcom->ports[i].usb2_speed = dwc3_qcom_read_usb2_speed(qcom, i);
> dwc3_qcom_enable_interrupts(qcom);
> }
[Severity: High]
This is a pre-existing issue, but by the time we reach this point during
system suspend, dwc3_qcom_domains_put() has already disabled both the bus
and power domains. If the device is a wakeup source, this code accesses
hardware registers by calling dwc3_qcom_read_usb2_speed() and
dwc3_qcom_enable_interrupts(). Because the power domain (GDSC) is off,
will accessing these registers cause a hardware exception or bus fault?
[ ... ]
> static int dwc3_qcom_resume(struct dwc3_qcom *qcom, pm_message_t msg)
> {
> int ret;
> int i;
> bool wakeup;
>
> if (!qcom->is_suspended)
> return 0;
[Severity: High]
If the device is already runtime suspended, qcom->is_suspended is true and
this returns early during system suspend. However, during runtime suspend,
only the bus domain vote is dropped and the power domain vote is kept. By
returning early here, does system suspend fail to release the power domain,
causing a permanent power leak of the PM domain usage counter?
[ ... ]
> /*
> * For runtime resume, always assume wakeup was enabled.
> * For system resume, check device wakeup capability.
> */
> if (PMSG_IS_AUTO(msg))
> wakeup = true;
> else
> wakeup = device_may_wakeup(qcom->dev);
>
> if (dwc3_qcom_is_host(qcom) && wakeup)
> dwc3_qcom_disable_interrupts(qcom);
[Severity: High]
This is a pre-existing issue, but this code accesses hardware registers
by calling dwc3_qcom_disable_interrupts() before the power and bus domains
are enabled by dwc3_qcom_domains_get() below. Does accessing these registers
while the power domain is disabled cause a synchronous external abort?
> - ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
> - if (ret < 0)
> - goto enable_irq;
> + if (!qcom->fw_managed) {
> + ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
> + if (ret < 0)
> + goto enable_irq;
>
> - ret = dwc3_qcom_interconnect_enable(qcom);
> - if (ret)
> - dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
> + ret = dwc3_qcom_interconnect_enable(qcom);
> + if (ret)
> + dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
> + } else {
> + /*
> + * Runtime resume only needs to re-vote bus (power was left
> + * on across runtime suspend). System resume votes both.
> + */
> + ret = dwc3_qcom_domains_get(qcom, PMSG_IS_AUTO(msg));
[Severity: High]
If the device was runtime suspended prior to system suspend,
dwc3_qcom_suspend() returned early and skipped dropping the power domain
vote. During system resume, dwc3_qcom_resume() does not return early, and
this unconditionally calls dwc3_qcom_domains_get() for both domains. Doesn't
this increment the power domain usage counter a second time, leading to a
permanent resource leak where the power domain never turns off?
[ ... ]
> @@ -741,9 +905,31 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> goto err_remove_swnode;
> }
>
> - ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
> - if (ret < 0)
> - goto err_remove_swnode;
> + if (!qcom->fw_managed) {
> + ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
> + if (ret < 0)
> + goto err_remove_swnode;
[Severity: Medium]
If early probe functions like clk_bulk_prepare_enable() fail, this jumps
directly to err_remove_swnode. Since err_remove_swnode is located after
the newly added reset_control_assert() block, doesn't this bypass the
intended hardware cleanup and leave the hardware controller out of reset?
[ ... ]
> @@ -829,15 +1034,63 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
> {
> struct dwc3 *dwc = platform_get_drvdata(pdev);
> struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
> + bool pm_resumed = false;
> + bool domains_suspended = true;
> + int ret;
>
> - if (pm_runtime_resume_and_get(qcom->dev) < 0)
> - return;
> + ret = pm_runtime_resume_and_get(qcom->dev);
> + if (ret < 0)
> + dev_warn(qcom->dev, "Failed to resume. Perform critical cleanups only.\n");
> + else
> + pm_resumed = true;
>
> - dwc3_core_remove(&qcom->dwc);
> - clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
> - dwc3_qcom_interconnect_exit(qcom);
> + /*
> + * Disable runtime PM to prevent any automatic PM operations during removal.
> + * This prevents race conditions where interrupt handlers or other subsystems
> + * might trigger runtime PM operations while the device is being torn down.
> + */
> + pm_runtime_disable(qcom->dev);
[Severity: High]
This adds an initial pm_runtime_disable() on the shared device. However,
when dwc3_core_remove() is called immediately after, the core unconditionally
calls pm_runtime_disable() on the exact same device. Doesn't this double
disable increment the disable_depth twice, permanently corrupting the PM
state and preventing the device from ever runtime suspending or resuming
again after a driver unbind/rebind cycle?
> + if (pm_resumed) {
> + /* Only perform operations that require device to be active */
> + dwc3_core_remove(&qcom->dwc);
[Severity: High]
This isn't a bug introduced by this patch, but if pm_runtime_resume_and_get()
fails, pm_resumed evaluates to false and dwc3_core_remove() is skipped.
Because the qcom structure is devres-managed, the memory backing it (including
the dwc3 struct) is unconditionally freed upon return. Does skipping core
removal leave the USB gadget, xHCI platform device, and notifiers active,
causing a use-after-free when these subsystems attempt to access the freed
memory?
> + if (!qcom->fw_managed) {
> + dwc3_qcom_interconnect_exit(qcom);
> + clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
> + } else {
> + ret = dwc3_qcom_domains_put(qcom, false /* bus_only */);
[Severity: Medium]
If the device fails to resume, pm_resumed is false and this skips calling
dwc3_qcom_domains_put(). However, a few lines down, dwc3_qcom_domain_detach()
is called unconditionally. Does this destroy the virtual domain devices while
the underlying genpd provider still holds an active PM vote from probe,
stranding the domain in the powered-on state indefinitely?
> + if (ret) {
> + dev_err(qcom->dev, "Failed to disable power domains during remove: %d\n",
> + ret);
> + domains_suspended = false;
> + }
> + }
> + }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260903153827.3463313-1-faisal.hassan@oss.qualcomm.com?part=7
^ permalink raw reply [flat|nested] 14+ messages in thread