[PATCH 0/4] phy: qcom: qmp-combo fixes + x1-dell-thena DT maintenance

[PATCH 0/4] phy: qcom: qmp-combo fixes + x1-dell-thena DT maintenance

[Michael Scott]

Four patches:

  * Two pre-existing bug fixes in phy-qcom-qmp-combo that are reachable
    today on any board which registers a usb_role_switch on this PHY,
    and will become reachable on more X1E boards as their Type-C
    support matures (patches 1-2).

  * Two dell-thena DT maintenance items: one mirrors a regulator
    always-on change Hovold applied to the rest of the X1E80100
    family but that dell-thena missed, and one bumps the linux,cma
    reserved-memory pool so the camera pipeline can actually
    allocate buffers alongside a normal desktop (patches 3-4).

== phy-qcom-qmp-combo fixes (patches 1-2) ==

Both bugs were found by exercising the typec_mux + role-switch code
paths on Dell Latitude 7455 (X1E80100, dell-thena). In mainline
today the bugs are reachable on x1e001de-devkit, which registers a
usb_role_switch on one USB-C port; they would also fire on any
future board that opts into the same DT pattern.

Patch 1: qmp_combo_usb_power_off() / qmp_combo_usb_exit() can be
re-entered as ->exit from an external consumer (dwc3 phy_exit during
driver unbind) after this device's backing devm resources have
already been released along a separate teardown chain. The
dereference of qmp->pcs (whose ioremap has been freed) then oopses
with a level-3 translation fault. The patch adds a usb_init_count
guard so the re-entry is a no-op. The proper long-term fix is a
teardown-ordering rework so the QMP PHY outlives any consumer that
may still call its phy_ops; until then, this guard prevents the
oops.

Patch 2: qmp_combo_typec_mux_set() updates the cached qmpphy_mode
unconditionally, but only reprograms hardware when init_count is
non-zero. So a typec_mux_set arriving before phy_init updates the
cache without programming hardware; subsequent calls then see a
"match" against the cached mode and bail out early, leaving the
lane mux in whatever state it powered up in. The patch tracks
separately whether the cache has been committed to hardware, so
the fast-path bail only happens when the cache truly reflects the
hardware.

== DT maintenance (patches 3-4) ==

Patch 3 marks vreg_l12b_1p2 and vreg_l15b_1p8 always-on. Hovold did
this for every other X1E80100 board in March 2025; dell-thena landed
four months later (commit e7733b42111c) and missed the change, which
leaves the kernel free to disable those LDOs even though several
board-level fixed regulators have no described vin-supply link back
to them.

Patch 4 raises linux,cma from 128 MiB to 256 MiB. The 128 MiB pool
is too small to support libcamera's buffer set in parallel with the
normal desktop: msm DRM framebuffers, qcom_iris codec buffers, and
qcom_camss VFE pre-allocations occupy ~100 MiB at GNOME idle,
leaving ~25 MiB free. libcamera's "simple" pipeline asks for four
8.35 MiB ABGR8888 frames (32 MiB total) and the fourth allocation
fails with "dma-heap allocation failure". At 256 MiB, ~150 MiB is
free at idle -- comfortable headroom.

Note for other X1E maintainers: every other X1E80100 / X1E78100 /
X1P42100 board in mainline is still on the 128 MiB default, and
several of them carry camera nodes (Dell XPS 13 9345, Medion
Sprchrgd-14, ASUS Zenbook A14, Microsoft Romulus, Microsoft Denali,
Lenovo ThinkBook 16). Those boards are likely to hit the same
allocation failure once libcamera enablement lands on them, and
should probably take a similar bump. I limited this patch to
dell-thena because I do not have the other boards on hand to
verify the resulting CmaFree numbers under a real workload --
applying the same change blindly across boards I cannot test would
just shift the guesswork.

== Patch summary ==

  1/4  phy: qcom: qmp-combo: skip USB power_off/exit after device
       teardown
  2/4  phy: qcom: qmp-combo: track whether the cached typec_mux mode
       was committed to hardware
  3/4  arm64: dts: qcom: x1-dell-thena: mark l12b and l15b always-on
  4/4  arm64: dts: qcom: x1-dell-thena: bump linux,cma to 256 MiB

== Testing ==

Hardware: Dell Latitude 7455 (X1E80100), running Ubuntu 26.04.

Test kernel: a local build of Ubuntu's 7.0.0-15-generic source with
this series applied on top. The series has additionally been verified
to apply cleanly on mainline v7.1-rc4 via `git am` with zero conflicts,
which is the base used to generate the patches in this submission.

  - Without this series:
    * Writing "none" to a manually-bound usb_role_switch reliably
      oopses the kernel. vmcore captured via kdump-tools; crash(1)
      analysis confirms qmp->pcs UAF in qmp_combo_usb_power_off,
      reached via dwc3_remove -> dwc3_phy_exit -> phy_exit ->
      qmp_combo_usb_exit.
    * Without patch 2, the first typec_mux_set arriving before
      phy_init updates the cache but not the hardware; the next
      call hits "same qmpphy mode, bail out" and the lane mux
      stays in its default configuration.
    * libcamera-mediated camera apps (gnome-snapshot, etc.) fail
      to start with "dma-heap allocation failure for frame-3".

  - With this series:
    * Role-switch teardown no longer oopses (patch 1's guard).
    * QMP PHY is reprogrammed on first altmode notification after
      phy_init (patch 2's committed-state tracking).
    * CmaFree at GNOME idle is ~150 MiB (was ~25 MiB).
    * gnome-snapshot opens with a live preview from the OV02E10
      sensor.

Patches 1-2 were exercised by manually wiring up a usb_role_switch
on dell-thena and driving the role-switch path; the DT change that
makes that wiring permanent is not part of this series.

Michael Scott (4):
  phy: qcom: qmp-combo: skip USB power_off/exit after device teardown
  phy: qcom: qmp-combo: track whether the cached typec_mux mode was
    committed to hardware
  arm64: dts: qcom: x1-dell-thena: mark l12b and l15b always-on
  arm64: dts: qcom: x1-dell-thena: bump linux,cma to 256 MiB

 arch/arm64/boot/dts/qcom/x1-dell-thena.dtsi |  4 ++-
 drivers/phy/qualcomm/phy-qcom-qmp-combo.c   | 47 +++++++++++++++++++++++++++--
 2 files changed, 48 insertions(+), 3 deletions(-)

base-commit: aa61612ab641d7d62b0b6889f2c7c9251489f6e3

--
2.53.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH 1/4] phy: qcom: qmp-combo: skip USB power_off/exit after device teardown

[Michael Scott]

qmp_combo_usb_power_off() is reachable from an external consumer
(notably dwc3 via phy_exit() during driver unbind) after this device's
backing resources have already been released along a separate teardown
chain. The dereference of qmp->pcs (whose ioremap mapping has been
freed by devm cleanup) then takes a level-3 translation fault and
oopses.

Easily reproducible during testing of USB-C role-switch enablement on
Dell Latitude 7455 (X1E80100), by writing "none" to a USB-C DWC3's
usb_role_switch role attribute, e.g.

  echo none > /sys/class/usb_role/a800000.usb-role-switch/role

which triggers the chain:

  Unable to handle kernel paging request at virtual address ffff8000876c5400
  pc : qmp_combo_usb_power_off.isra.0+0x58/0x470 [phy_qcom_qmp_combo]
  Call trace:
    qmp_combo_usb_power_off+0x58/0x470 [phy_qcom_qmp_combo]
    qmp_combo_usb_exit+0x38/0x90 [phy_qcom_qmp_combo]
    phy_exit
    dwc3_phy_exit [dwc3]
    dwc3_core_remove [dwc3]
    dwc3_remove [dwc3]
    platform_remove
    device_release_driver_internal
    device_driver_detach
    unbind_store
    sysfs_kf_write
    vfs_write
    ksys_write
    __arm64_sys_write
    el0_svc

Two WARNs precede the oops from the same teardown chain, confirming
the resource ordering:

  WARNING: drivers/clk/clk.c:4494 at clk_nodrv_disable_unprepare+0x8/0x18
  WARNING: drivers/regulator/core.c:2657 at _regulator_put+0x84/0x98

i.e. the pipe clock provider has been unregistered and the regulators
released before qmp_combo_usb_power_off() runs.

The proper long-term fix is a teardown-ordering rework so the QMP
PHY's backing resources outlive any consumer that may still call its
phy_ops. Pending that, guard the power_off/exit paths with the
existing usb_init_count balance so re-entry after teardown does not
oops. usb_init_count tracks the balance of usb_power_on/off; if it
is zero we have either never powered on or have already powered off,
and there is nothing to do.

The same guard is added to qmp_combo_usb_exit() since it is the entry
point used by external consumers via phy_exit().

Signed-off-by: Michael Scott <mike.scott@oss.qualcomm.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index 93f1aa10d400..4057583c0cfe 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
@@ -3922,6 +3922,17 @@ static int qmp_combo_usb_power_off(struct phy *phy)
 	struct qmp_combo *qmp = phy_get_drvdata(phy);
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
 
+	/*
+	 * Reachable as ->exit from external consumers (notably dwc3) after
+	 * this device's backing resources have already been released along
+	 * a teardown chain. Refuse to touch registers in that case.
+	 */
+	if (!qmp->usb_init_count) {
+		dev_dbg(qmp->dev, "%s: PHY not powered on, skipping\n",
+			__func__);
+		return 0;
+	}
+
 	clk_disable_unprepare(qmp->pipe_clk);
 
 	/* PHY reset */
@@ -3966,6 +3977,17 @@ static int qmp_combo_usb_exit(struct phy *phy)
 	struct qmp_combo *qmp = phy_get_drvdata(phy);
 	int ret;
 
+	/*
+	 * See qmp_combo_usb_power_off(): an external consumer may call
+	 * phy_exit() after the QMP device's resources have been torn
+	 * down. usb_init_count tracks usb_init/usb_exit balance.
+	 */
+	if (!qmp->usb_init_count) {
+		dev_dbg(qmp->dev, "%s: PHY not initialised, skipping\n",
+			__func__);
+		return 0;
+	}
+
 	mutex_lock(&qmp->phy_mutex);
 	ret = qmp_combo_usb_power_off(phy);
 	if (ret)
-- 
2.53.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH 2/4] phy: qcom: qmp-combo: track whether the cached typec_mux mode was committed to hardware

[Michael Scott]

qmp_combo_typec_mux_set() updates qmp->qmpphy_mode (the cached state)
unconditionally, but only reprograms hardware when qmp->init_count is
non-zero. If pmic_glink_altmode (or any other typec_mux consumer)
calls into the PHY before DWC3 has performed phy_init() -- a real
ordering observed during testing of USB-C role-switch enablement on
Snapdragon X (X1E80100) -- the cache transitions away from the
probe default QMPPHY_MODE_USB3DP but the hardware is never touched.

Subsequent calls (for example on partner detach, where TYPEC_STATE_SAFE
also resolves to QMPPHY_MODE_USB3_ONLY in the !DP-SVID branch) then
match the cached mode and the function bails out early with:

  qcom-qmp-combo-phy faXX000.phy: typec_mux_set: same qmpphy mode, bail out

leaving the lane mux in whatever configuration it powered up in. On
the Dell Latitude 7455 this manifests as the SS lanes being left in
the default state when the first altmode notification arrives during
DWC3 probe, with the function bailing out on every subsequent attach.

Track separately whether the cached mode has actually been committed
to hardware. The bail-out optimization is only safe when the cache
truly reflects the hardware:

  - qmp_combo_typec_mux_set(): bail only when the cached mode matches
    and was committed; clear the committed flag whenever the cache is
    updated, set it again after a successful reprogram inside the
    init_count-guarded block.

  - qmp_combo_com_init(): set the committed flag at the end of a
    successful init, since com_init() programs registers from the
    cached qmpphy_mode.

No behavioural change on platforms where typec_mux_set never fires
before phy_init -- committed remains true through normal operation.

Signed-off-by: Michael Scott <mike.scott@oss.qualcomm.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 25 +++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index 4057583c0cfe..9b6da783ce60 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
@@ -2295,6 +2295,7 @@ struct qmp_combo {
 	struct mutex phy_mutex;
 	int init_count;
 	enum qmpphy_mode qmpphy_mode;
+	bool qmpphy_mode_committed;
 
 	struct phy *usb_phy;
 	enum phy_mode phy_mode;
@@ -3747,6 +3748,9 @@ static int qmp_combo_com_init(struct qmp_combo *qmp, bool force)
 	qphy_setbits(qmp->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
 			SW_PWRDN);
 
+	/* com_init() just programmed registers from qmp->qmpphy_mode. */
+	qmp->qmpphy_mode_committed = true;
+
 	return 0;
 
 err_assert_reset:
@@ -4507,9 +4511,22 @@ static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_s
 		new_mode = QMPPHY_MODE_USB3_ONLY;
 	}
 
+	/*
+	 * Fast-path bail only when the cached mode is also known to be
+	 * committed to hardware. The cache may be ahead of the hardware
+	 * if a typec_mux_set arrived while the PHY had not yet been
+	 * initialised (init_count == 0); in that case the cache update
+	 * below was the only thing that ran, and we still need to drive
+	 * the registers when the PHY does come up.
+	 */
 	if (new_mode == qmp->qmpphy_mode) {
-		dev_dbg(qmp->dev, "typec_mux_set: same qmpphy mode, bail out\n");
-		return 0;
+		if (qmp->qmpphy_mode_committed) {
+			dev_dbg(qmp->dev,
+				"typec_mux_set: same qmpphy mode (committed), bail out\n");
+			return 0;
+		}
+		dev_dbg(qmp->dev,
+			"typec_mux_set: same qmpphy mode but uncommitted; reprogramming\n");
 	}
 
 	if (qmp->qmpphy_mode != QMPPHY_MODE_USB3_ONLY && qmp->dp_powered_on) {
@@ -4521,6 +4538,7 @@ static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_s
 		qmp->qmpphy_mode, new_mode);
 
 	qmp->qmpphy_mode = new_mode;
+	qmp->qmpphy_mode_committed = false;
 
 	if (qmp->init_count) {
 		if (qmp->usb_init_count)
@@ -4549,6 +4567,9 @@ static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_s
 			if (qmp->dp_init_count)
 				cfg->dp_aux_init(qmp);
 		}
+
+		/* Reprogram complete; cache now reflects hardware. */
+		qmp->qmpphy_mode_committed = true;
 	}
 
 	return 0;
-- 
2.53.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH 3/4] arm64: dts: qcom: x1-dell-thena: mark l12b and l15b always-on

[Michael Scott]

The l12b and l15b supplies are used by components that are not (fully)
described (and some never will be) and must never be disabled.

Mark the regulators as always-on to prevent them from being disabled,
for example, when consumers probe defer or suspend.

Note that these supplies currently have no consumers described in
mainline for dell-thena beyond the audio codec (vdd-buck/vdd-rxtx/
vdd-io on wcd938x), which can release them when the codec goes idle.
The board-level gpio-fixed regulators that feed the Type-C retimer's
VDDIO and other rails are not described with a vin-supply link, so
the kernel cannot keep their parent LDOs alive on its own.

This mirrors the same change Johan Hovold applied to every other
X1E80100 board in a March 2025 series; commit 63169c07d740
("arm64: dts: qcom: x1e80100-dell-xps13-9345: mark l12b and l15b always-on")
is representative. The dell-thena board file was introduced four months
later and did not inherit that change; this patch closes the gap.

Fixes: e7733b42111c ("arm64: dts: qcom: Add support for Dell Inspiron 7441 / Latitude 7455")
Cc: stable@vger.kernel.org
Signed-off-by: Michael Scott <mike.scott@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/x1-dell-thena.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/x1-dell-thena.dtsi b/arch/arm64/boot/dts/qcom/x1-dell-thena.dtsi
index 96e7a18366dc..d93b704872b5 100644
--- a/arch/arm64/boot/dts/qcom/x1-dell-thena.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1-dell-thena.dtsi
@@ -589,6 +589,7 @@ vreg_l12b_1p2: ldo12 {
 			regulator-min-microvolt = <1200000>;
 			regulator-max-microvolt = <1200000>;
 			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+			regulator-always-on;
 		};
 
 		vreg_l13b_3p0: ldo13 {
@@ -610,6 +611,7 @@ vreg_l15b_1p8: ldo15 {
 			regulator-min-microvolt = <1800000>;
 			regulator-max-microvolt = <1800000>;
 			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+			regulator-always-on;
 		};
 	};
 
-- 
2.53.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH 4/4] arm64: dts: qcom: x1-dell-thena: bump linux,cma to 256 MiB

[Michael Scott]

The 128 MiB linux,cma reserved-memory pool on dell-thena is too small
to support the camera pipeline in parallel with the normal Linux
desktop. On a freshly-booted system with GNOME running, the typical
runtime consumers — msm DRM framebuffers (Wayland triple buffering on
the eDP panel), qcom_iris video codec buffers, qcom_camss VFE
pre-allocated buffers — already occupy ~100 MiB of the pool, leaving
only ~25 MiB free.

The libcamera "simple" pipeline handler used by /dev/media0 on
dell-thena allocates four ABGR8888 frames at 1920×1088 = 32 MiB total.
That request fails on the fourth frame:

    ERROR DmaBufAllocator: dma-heap allocation failure for frame-3
    ERROR Allocator: Stream is not part of /base/.../camera@10 active configuration
    Can't allocate buffers
    Failed to start camera session

resulting in gnome-snapshot's "Could not play camera stream" and any
other libcamera-mediated app being unable to actually stream.

Bumping linux,cma to 256 MiB (a 0.9% reservation on these laptops'
typical 27 GiB RAM) leaves ~150 MiB free at runtime — sufficient for
the libcamera buffer set plus headroom for video playback or other
CMA-hungry workloads in parallel.

Tested on Dell Latitude 7455: with the 256 MiB pool, CmaFree at
GNOME-desktop idle is ~150 MiB, gnome-snapshot streams the OV02E10
camera cleanly, and `cam -c 1 --capture=2` succeeds.

The companion board files dell-inspiron-14-plus-7441 and the upstream
.dts variants inherit from x1-dell-thena.dtsi, so this changes the
pool size for every dell-thena-based laptop in one place.

Signed-off-by: Michael Scott <mike.scott@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/x1-dell-thena.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/x1-dell-thena.dtsi b/arch/arm64/boot/dts/qcom/x1-dell-thena.dtsi
index d93b704872b5..7894d96d34ee 100644
--- a/arch/arm64/boot/dts/qcom/x1-dell-thena.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1-dell-thena.dtsi
@@ -167,7 +167,7 @@ led-camera-indicator {
 	reserved-memory {
 		linux,cma {
 			compatible = "shared-dma-pool";
-			size = <0x0 0x8000000>;
+			size = <0x0 0x10000000>;
 			reusable;
 			linux,cma-default;
 		};
-- 
2.53.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 1/4] phy: qcom: qmp-combo: skip USB power_off/exit after device teardown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Bailing out early in `qmp_combo_usb_exit()` completely bypasses the release of shared common resources, causing a permanent reference leak when a Type-C switch to `DP_ONLY` occurs.
- [High] A lockless Time-of-Check to Time-of-Use (TOCTOU) race condition on `qmp->usb_init_count` in `qmp_combo_usb_exit()` leads to integer underflow and state corruption.

Pre-existing issues:
- [High] The patch attempts to fix an MMIO crash by introducing a Use-After-Free read on the `qmp` driver data structure.
--

commit 35c4e56f462fac540e8354624e2048c6bd3ecd9e
Author: Michael Scott <mike.scott@oss.qualcomm.com>

phy: qcom: qmp-combo: skip USB power_off/exit after device teardown

This commit adds checks to qmp_combo_usb_power_off() and qmp_combo_usb_exit()
to guard against being called by an external consumer after the device's
backing resources have been released. It uses the usb_init_count variable
to determine if the PHY is powered on, skipping the operations to prevent
an oops.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 93f1aa10d4008..4057583c0cfeb 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -3922,6 +3922,17 @@ static int qmp_combo_usb_power_off(struct phy *phy)
>  	struct qmp_combo *qmp = phy_get_drvdata(phy);
>  	const struct qmp_phy_cfg *cfg = qmp->cfg;
>  
> +	/*
> +	 * Reachable as ->exit from external consumers (notably dwc3) after
> +	 * this device's backing resources have already been released along
> +	 * a teardown chain. Refuse to touch registers in that case.
> +	 */
> +	if (!qmp->usb_init_count) {

[Severity: High]
This isn't a bug introduced by this patch, but does this check introduce
a use-after-free read on the qmp driver data structure?

The commit message notes that this path runs after the device's backing
resources are released. Since qmp is allocated using devm_kzalloc(), it is
also freed during devres teardown.

When dwc3 calls phy_exit() later, phy_get_drvdata() returns a freed pointer.

Dereferencing qmp->usb_init_count accesses this freed memory. If the memory
was reallocated and non-zero, or if KASAN is enabled, wouldn't this still
trigger a use-after-free and crash instead of masking the teardown order bug?

> +		dev_dbg(qmp->dev, "%s: PHY not powered on, skipping\n",
> +			__func__);
> +		return 0;
> +	}
> +
>  	clk_disable_unprepare(qmp->pipe_clk);

[ ... ]

> @@ -3966,6 +3977,17 @@ static int qmp_combo_usb_exit(struct phy *phy)
>  	struct qmp_combo *qmp = phy_get_drvdata(phy);
>  	int ret;
>  
> +	/*
> +	 * See qmp_combo_usb_power_off(): an external consumer may call
> +	 * phy_exit() after the QMP device's resources have been torn
> +	 * down. usb_init_count tracks usb_init/usb_exit balance.
> +	 */
> +	if (!qmp->usb_init_count) {

[Severity: High]
Does bailing out early here bypass the release of shared common resources,
causing a reference leak when a Type-C switch to DP_ONLY occurs?

When the Type-C mux switches to QMPPHY_MODE_DP_ONLY,
qmp_combo_typec_mux_set() decrements usb_init_count to 0 but leaves
init_count unchanged. If the consumer unbinds and calls phy_exit() in
this state, this early return bypasses the qmp_combo_com_exit(qmp, false)
call.

Doesn't this mean qmp->init_count is never decremented, permanently
leaking shared resources like clocks and regulators?


[Severity: High]
Is there a lockless Time-of-Check to Time-of-Use race condition here?

This check is performed before acquiring qmp->phy_mutex. If another path
like qmp_combo_typec_mux_set() holds the mutex and decrements
usb_init_count to 0:

CPU1
qmp_combo_usb_exit()
   if (!qmp->usb_init_count) // observes > 0
   mutex_lock(&qmp->phy_mutex); // blocks

CPU2
qmp_combo_typec_mux_set()
   mutex_lock(&qmp->phy_mutex);
   qmp->usb_init_count--; // becomes 0
   mutex_unlock(&qmp->phy_mutex);

When CPU1 acquires the lock, it proceeds to decrement qmp->usb_init_count
without re-verifying the state. Doesn't decrementing 0 cause an integer
underflow to UINT_MAX, breaking the PHY's power management balance?

> +		dev_dbg(qmp->dev, "%s: PHY not initialised, skipping\n",
> +			__func__);
> +		return 0;
> +	}
> +
>  	mutex_lock(&qmp->phy_mutex);
>  	ret = qmp_combo_usb_power_off(phy);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521003615.1260844-1-mike.scott@oss.qualcomm.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 2/4] phy: qcom: qmp-combo: track whether the cached typec_mux mode was committed to hardware

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] The patch is a complete logical no-op and fails to alter the hardware programming flow in the described failure scenario.
- [Medium] The patch unconditionally marks `qmpphy_mode_committed = true` in `qmp_combo_typec_mux_set()`, ignoring hardware reprogramming failures.

Pre-existing issues:
- [High] `qmp_combo_com_init()` incorrectly decrements `init_count` on failure when `force == true`, corrupting reference counts.
- [High] TOCTOU race condition in `qmp_combo_usb_exit()` causes `usb_init_count` integer underflow.
- [High] Delayed PHY mode switches are permanently dropped if the DP PHY is actively powered on.
--

commit 6892dbad473d7036463b8a81f539f3ea5bbf55d4
Author: Michael Scott <mike.scott@oss.qualcomm.com>
Subject: phy: qcom: qmp-combo: track whether the cached typec_mux mode was committed to hardware

This patch attempts to fix an issue where the typec_mux_set function
bails out early and fails to reprogram hardware if called before
phy_init. It adds a committed flag to track if the cached mode matches
the actual hardware state.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 4057583c0cfeb..9b6da783ce607 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -3747,6 +3748,9 @@ static int qmp_combo_com_init(struct qmp_combo *qmp, bool force)
>  	qphy_setbits(qmp->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
>  			SW_PWRDN);
>  
> +	/* com_init() just programmed registers from qmp->qmpphy_mode. */
> +	qmp->qmpphy_mode_committed = true;
> +
>  	return 0;
>  
>  err_assert_reset:
>  	reset_control_bulk_assert(cfg->num_resets, qmp->resets);
>  err_disable_regulators:
>  	regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
>  err_decrement_count:
>  	qmp->init_count--;
>  
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but does this error path corrupt the reference
count when called with force == true?

If qmp_combo_com_init() is called with force = true, the short-circuit
evaluation bypasses incrementing qmp->init_count:

    if (!force && qmp->init_count++)
        return 0;

However, if a subsequent operation like regulator_bulk_enable() fails, the
error path jumps to err_decrement_count and unconditionally decrements it.

Could this underflow init_count to -1, permanently breaking the initialization
state machine since subsequent checks like `if (qmp->init_count++)` will see
truthy values?

[ ... ]

> @@ -4507,9 +4511,22 @@ static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_s
>  		new_mode = QMPPHY_MODE_USB3_ONLY;
>  	}
>  
> +	/*
> +	 * Fast-path bail only when the cached mode is also known to be
> +	 * committed to hardware. The cache may be ahead of the hardware
> +	 * if a typec_mux_set arrived while the PHY had not yet been
> +	 * initialised (init_count == 0); in that case the cache update
> +	 * below was the only thing that ran, and we still need to drive
> +	 * the registers when the PHY does come up.
> +	 */
>  	if (new_mode == qmp->qmpphy_mode) {
> -		dev_dbg(qmp->dev, "typec_mux_set: same qmpphy mode, bail out\n");
> -		return 0;
> +		if (qmp->qmpphy_mode_committed) {
> +			dev_dbg(qmp->dev,
> +				"typec_mux_set: same qmpphy mode (committed), bail out\n");
> +			return 0;
> +		}
> +		dev_dbg(qmp->dev,
> +			"typec_mux_set: same qmpphy mode but uncommitted; reprogramming\n");
>  	}
>  
>  	if (qmp->qmpphy_mode != QMPPHY_MODE_USB3_ONLY && qmp->dp_powered_on) {
>  		dev_err(qmp->dev, "DP PHY is still in use, delaying switch\n");
>  		return 0;
>  	}

[Severity: High]
This is a pre-existing issue, but does this early return permanently drop
delayed PHY mode switches?

If a switch to QMPPHY_MODE_USB3_ONLY is requested but the DP PHY is actively
powered on, this branch bails out early, logs a message, and returns 0.

Because it returns success without updating qmp->qmpphy_mode or scheduling a
delayed work to apply the switch later, the Type-C framework assumes the mode
was successfully applied.

Does this cause a permanent desynchronization between software state and
hardware, keeping the DP PHY powered when it should be disabled?

> @@ -4521,6 +4538,7 @@ static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_s
>  		qmp->qmpphy_mode, new_mode);
>  
>  	qmp->qmpphy_mode = new_mode;
> +	qmp->qmpphy_mode_committed = false;
>  
>  	if (qmp->init_count) {

[Severity: High]
Is this patch effectively a logical no-op in the failure scenario it
describes?

If typec_mux_set() is called before phy_init() (when init_count is 0), the
early bail-out is bypassed because qmpphy_mode_committed is false. However,
the code then reaches the `if (qmp->init_count)` check here, which evaluates
to false, causing the function to return without touching the hardware anyway.

When phy_init() eventually executes, qmp_combo_com_init() correctly syncs
hardware to the cached mode and unconditionally sets
qmpphy_mode_committed = true.

Subsequent calls to typec_mux_set() with the same mode will see
qmpphy_mode_committed == true and bail out early, exactly as they did before
this patch. Does this mean the hardware state remains unchanged from the
pre-patch behavior?

>  		if (qmp->usb_init_count)
[ ... ]
>  			if (qmp->dp_init_count)
>  				cfg->dp_aux_init(qmp);
>  		}
> +
> +		/* Reprogram complete; cache now reflects hardware. */
> +		qmp->qmpphy_mode_committed = true;
>  	}

[Severity: Medium]
Is it safe to unconditionally mark the hardware state as committed here?

The code calls qmp_combo_com_init(qmp, true) earlier in this block to apply
hardware changes, but ignores its return value. If qmp_combo_com_init() fails,
the hardware state remains unconfigured, but this flag is blindly set to true.

Would this prevent any subsequent attempts to correct the hardware state,
since future calls will trigger the early bail-out on
qmp->qmpphy_mode_committed?


[Severity: High]
This isn't a bug introduced by this patch, but is there a time-of-check to
time-of-use race condition in qmp_combo_usb_exit() that can underflow
usb_init_count?

If qmp->usb_init_count is checked outside the phy_mutex in
qmp_combo_usb_exit(), a concurrent Type-C mode switch could occur:

Thread 1: qmp_combo_usb_exit()
    if (qmp->usb_init_count == 0)
        return;
    // Preempted before acquiring phy_mutex

Thread 2: qmp_combo_typec_mux_set()
    mutex_lock(&qmp->phy_mutex);
    // Switches to QMPPHY_MODE_DP_ONLY
    qmp->usb_init_count--; // Now 0
    mutex_unlock(&qmp->phy_mutex);

Thread 1 resumes:
    mutex_lock(&qmp->phy_mutex);
    ...
    qmp->usb_init_count--; // Underflows to UINT_MAX

Could this permanently break subsequent power management logic and allow
unclocked hardware accesses?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521003615.1260844-1-mike.scott@oss.qualcomm.com?part=2

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy