[PATCH 0/2] power: sequencing: pcie-m2: Add 3.3Vaux supply support

[PATCH 0/2] power: sequencing: pcie-m2: Add 3.3Vaux supply support

[Sherry Sun (OSS)]

From: Sherry Sun <sherry.sun@nxp.com>

3.3Vaux supply has different power lifecycle requirements compared to other
supplies like vpcie3v3. It must remain enabled during system suspend to
support PCIe L2 link state and wake-up mechanisms. This is a requirement
for devices that need to maintain PCIe link presence and support remote
wakeup while the system is suspended.

This patchset adds support for the 3.3Vaux supply by:

  1. Extending the PCIe M.2 E-key connector DT binding to include the
     optional vpcie3v3aux-supply property.

  2. Updating the PCIe M.2 power sequencing driver to handle vpcie3v3aux
     separately from other supplies, keeping it enabled across system
     suspend/resume cycles when present.

This has been tested on i.MX95 & i.MX943 EVK board with a NXP Wi-Fi/BT M.2
AW693 module that requires 3.3Vaux to remain active during suspend for
wake-up support.

Sherry Sun (2):
  dt-bindings: connector: pcie-m2-e: Add 3.3Vaux supply support
  power: sequencing: pcie-m2: Add 3.3Vaux supply support

 .../connector/pcie-m2-e-connector.yaml        |  3 ++
 drivers/power/sequencing/pwrseq-pcie-m2.c     | 42 ++++++++++++++++++-
 2 files changed, 44 insertions(+), 1 deletion(-)

-- 
2.50.1

[PATCH 1/2] dt-bindings: connector: pcie-m2-e: Add 3.3Vaux supply support

[Sherry Sun (OSS)]

From: Sherry Sun <sherry.sun@nxp.com>

Add 3.3Vaux separately for special handling. This supply has a different
lifecycle - it must remain enabled during system suspend to support PCIe
L2 link state and wake-up mechanisms.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
 .../devicetree/bindings/connector/pcie-m2-e-connector.yaml     | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/connector/pcie-m2-e-connector.yaml b/Documentation/devicetree/bindings/connector/pcie-m2-e-connector.yaml
index f7859aa9b634..381b2ba8c80d 100644
--- a/Documentation/devicetree/bindings/connector/pcie-m2-e-connector.yaml
+++ b/Documentation/devicetree/bindings/connector/pcie-m2-e-connector.yaml
@@ -25,6 +25,9 @@ properties:
   vpcie1v8-supply:
     description: A phandle to the regulator for VIO 1.8v supply.
 
+  vpcie3v3aux-supply:
+    description: A phandle to the regulator for 3.3v AUX supply.
+
   i2c-parent:
     $ref: /schemas/types.yaml#/definitions/phandle
     description: I2C interface
-- 
2.50.1

Re: [PATCH 1/2] dt-bindings: connector: pcie-m2-e: Add 3.3Vaux supply support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The `vpcie3v3aux-supply` property describes a non-existent hardware feature on the M.2 Key E connector to work around a software policy.
--

commit 0268cb00caa35acacc055d760f1155866bebdf1d
Author: Sherry Sun <sherry.sun@nxp.com>

dt-bindings: connector: pcie-m2-e: Add 3.3Vaux supply support

This commit adds a vpcie3v3aux-supply property to the M.2 Key E connector
binding. The commit message indicates this is added to support special
lifecycle handling for the regulator during system suspend.

> diff --git a/Documentation/devicetree/bindings/connector/pcie-m2-e-connector.yaml b/Documentation/devicetree/bindings/connector/pcie-m2-e-connector.yaml
> --- a/Documentation/devicetree/bindings/connector/pcie-m2-e-connector.yaml
> +++ b/Documentation/devicetree/bindings/connector/pcie-m2-e-connector.yaml
> @@ -25,6 +25,9 @@ properties:
>    vpcie1v8-supply:
>      description: A phandle to the regulator for VIO 1.8v supply.
>  
> +  vpcie3v3aux-supply:
> +    description: A phandle to the regulator for 3.3v AUX supply.
> +

[Severity: Medium]
Does this property model a physical hardware pin that actually exists in the
M.2 specification?

The PCI Express M.2 Specification does not define a separate 3.3Vaux power
rail; it only defines a single primary 3.3V supply alongside an optional 1.8V
VIO supply. The concept of 3.3Vaux existed in older standards like PCIe Mini
Card and standard PCIe CEM slots, but was removed in the M.2 standard.

Device tree bindings are expected to strictly model physical hardware rather
than software lifecycle policies. Adding a non-existent hardware pin to the
standard M.2 connector binding purely to instruct the driver to keep the
regulator enabled during system suspend pollutes the permanent device tree ABI.

Could the existing vpcie3v3-supply regulator instead be properly configured in
the board device tree using standard regulator suspend states (such as
regulator-state-mem), or could the host driver be made aware of the wake
requirements to leave the primary supply on?

>    i2c-parent:
>      $ref: /schemas/types.yaml#/definitions/phandle
>      description: I2C interface

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609033840.2006078-1-sherry.sun@oss.nxp.com?part=1

[PATCH 2/2] power: sequencing: pcie-m2: Add 3.3Vaux supply support

[Sherry Sun (OSS)]

From: Sherry Sun <sherry.sun@nxp.com>

Add 3.3Vaux separately for special handling. This supply has a different
lifecycle - it must remain enabled during system suspend to support PCIe
L2 link state and wake-up mechanisms.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
 drivers/power/sequencing/pwrseq-pcie-m2.c | 42 ++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/power/sequencing/pwrseq-pcie-m2.c b/drivers/power/sequencing/pwrseq-pcie-m2.c
index e82821655fc4..6b8c77cf20a9 100644
--- a/drivers/power/sequencing/pwrseq-pcie-m2.c
+++ b/drivers/power/sequencing/pwrseq-pcie-m2.c
@@ -37,6 +37,8 @@ struct pwrseq_pcie_m2_ctx {
 	const struct pwrseq_pcie_m2_pdata *pdata;
 	struct regulator_bulk_data *regs;
 	size_t num_vregs;
+	struct regulator *vaux_reg;
+	bool vaux_enabled;
 	struct notifier_block nb;
 	struct gpio_desc *w_disable1_gpio;
 	struct gpio_desc *w_disable2_gpio;
@@ -48,8 +50,23 @@ struct pwrseq_pcie_m2_ctx {
 static int pwrseq_pcie_m2_vregs_enable(struct pwrseq_device *pwrseq)
 {
 	struct pwrseq_pcie_m2_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
+	int ret;
+
+	/* Enable 3.3Vaux if present and not already enabled. */
+	if (ctx->vaux_reg && !ctx->vaux_enabled) {
+		ret = regulator_enable(ctx->vaux_reg);
+		if (ret)
+			return ret;
+		ctx->vaux_enabled = true;
+	}
 
-	return regulator_bulk_enable(ctx->num_vregs, ctx->regs);
+	ret = regulator_bulk_enable(ctx->num_vregs, ctx->regs);
+	if (ret && ctx->vaux_reg && ctx->vaux_enabled) {
+		regulator_disable(ctx->vaux_reg);
+		ctx->vaux_enabled = false;
+	}
+
+	return ret;
 }
 
 static int pwrseq_pcie_m2_vregs_disable(struct pwrseq_device *pwrseq)
@@ -497,10 +514,28 @@ static int pwrseq_pcie_m2_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, -ENODEV,
 				     "Failed to obtain platform data\n");
 
+	/*
+	 * Get 3.3Vaux separately for special handling. This supply has a
+	 * different lifecycle - it must remain enabled during system suspend
+	 * to support PCIe L2 link state and wake-up mechanisms.
+	 */
+	ctx->vaux_reg = devm_regulator_get_optional(dev, "vpcie3v3aux");
+	if (IS_ERR(ctx->vaux_reg)) {
+		if (PTR_ERR(ctx->vaux_reg) != -ENODEV)
+			return PTR_ERR(ctx->vaux_reg);
+		ctx->vaux_reg = NULL;
+	}
+	ctx->vaux_enabled = false;
+
 	/*
 	 * Currently, of_regulator_bulk_get_all() is the only regulator API that
 	 * allows to get all supplies in the devicetree node without manually
 	 * specifying them.
+	 *
+	 * This will include vaux again, but we'll manage it separately via
+	 * vaux_reg. The regulator framework handles multiple gets of the same
+	 * regulator correctly via refcounting, so having vaux in both places is
+	 * safe.
 	 */
 	ret = of_regulator_bulk_get_all(dev, dev_of_node(dev), &ctx->regs);
 	if (ret < 0)
@@ -573,6 +608,11 @@ static void pwrseq_pcie_m2_remove(struct platform_device *pdev)
 	pwrseq_pcie_m2_remove_serdev(ctx, NULL);
 	mutex_destroy(&ctx->list_lock);
 
+	if (ctx->vaux_reg && ctx->vaux_enabled) {
+		regulator_disable(ctx->vaux_reg);
+		ctx->vaux_enabled = false;
+	}
+
 	regulator_bulk_free(ctx->num_vregs, ctx->regs);
 }
 
-- 
2.50.1

Re: [PATCH 2/2] power: sequencing: pcie-m2: Add 3.3Vaux supply support

[sashiko-bot]

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

New issues:
- [High] Data race on `ctx->vaux_enabled` and unprotected access to `ctx->vaux_reg` between `pwrseq_pcie_m2_remove()` and concurrent power sequencer callbacks.
- [Medium] If `probe` fails after the power sequencer has been registered and a consumer has already utilized it, the `vaux_reg` regulator is never disabled. This leaks the enable count and causes a `WARN_ON` during `devres` cleanup.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the `regulator_bulk_enable()` error path will dereference an uninitialized `.supply` pointer, leading to a kernel crash. This occurs because `of_regulator_bulk_get_all()` allocates the consumers array without zeroing memory.
- [High] This problem wasn't introduced by this patch, but there is a NULL pointer dereference of `ctx->regs[i].consumer` causing a Use-After-Free kernel crash if a consumer unbinds concurrently with or immediately after the provider's `remove()` function.
--

commit ecd4292236752225aeb9fc0b3242e75d18056e96
Author: Sherry Sun <sherry.sun@nxp.com>

power: sequencing: pcie-m2: Add 3.3Vaux supply support

This commit adds support for the 3.3Vaux supply to the PCIe M.2 power
sequencer. The supply is handled separately because it has a different
lifecycle and must remain enabled during system suspend for L2 link states.

> diff --git a/drivers/power/sequencing/pwrseq-pcie-m2.c b/drivers/power/sequencing/pwrseq-pcie-m2.c
> index e82821655fc4b..6b8c77cf20a9c 100644
> --- a/drivers/power/sequencing/pwrseq-pcie-m2.c
> +++ b/drivers/power/sequencing/pwrseq-pcie-m2.c
[ ... ]
> @@ -48,8 +50,23 @@ struct pwrseq_pcie_m2_ctx {
>  static int pwrseq_pcie_m2_vregs_enable(struct pwrseq_device *pwrseq)
>  {
>  	struct pwrseq_pcie_m2_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
> +	int ret;
> +
> +	/* Enable 3.3Vaux if present and not already enabled. */
> +	if (ctx->vaux_reg && !ctx->vaux_enabled) {
> +		ret = regulator_enable(ctx->vaux_reg);
> +		if (ret)
> +			return ret;
> +		ctx->vaux_enabled = true;
> +	}
>  
> -	return regulator_bulk_enable(ctx->num_vregs, ctx->regs);
> +	ret = regulator_bulk_enable(ctx->num_vregs, ctx->regs);

[Severity: High]
This is a pre-existing issue, but does this regulator_bulk_enable() error
path handle uninitialized pointers correctly?

If regulator_bulk_enable() fails, its error unwind path attempts to print
the name of the failing supply. However, of_regulator_bulk_get_all()
allocates the consumers array using kmalloc_objs() which does not zero
memory, leaving the .supply field uninitialized.

Could this lead to an out-of-bounds read and kernel panic when the error
message dereferences the uninitialized string pointer?

> +	if (ret && ctx->vaux_reg && ctx->vaux_enabled) {
> +		regulator_disable(ctx->vaux_reg);
> +		ctx->vaux_enabled = false;
> +	}
> +
> +	return ret;
>  }
[ ... ]
> @@ -497,10 +514,28 @@ static int pwrseq_pcie_m2_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, -ENODEV,
>  				     "Failed to obtain platform data\n");
>  
> +	/*
> +	 * Get 3.3Vaux separately for special handling. This supply has a
> +	 * different lifecycle - it must remain enabled during system suspend
> +	 * to support PCIe L2 link state and wake-up mechanisms.
> +	 */
> +	ctx->vaux_reg = devm_regulator_get_optional(dev, "vpcie3v3aux");
> +	if (IS_ERR(ctx->vaux_reg)) {
> +		if (PTR_ERR(ctx->vaux_reg) != -ENODEV)
> +			return PTR_ERR(ctx->vaux_reg);
> +		ctx->vaux_reg = NULL;
> +	}
> +	ctx->vaux_enabled = false;
> +

[Severity: Medium]
If pwrseq_pcie_m2_probe() fails later on (for example, in
pwrseq_pcie_m2_create_serdev()), is the vaux_reg properly disabled?

When devm_pwrseq_device_register() succeeds, the power sequencer becomes
active and a consumer can synchronously bind and call pwrseq_power_on(),
setting ctx->vaux_enabled to true.

If the probe subsequently fails, the err_free_regulators error path calls
regulator_bulk_free() without explicitly disabling ctx->vaux_reg. When the
consumer is unbound by devres and calls pwrseq_power_off(), it skips
disabling vaux_reg because the driver relies on explicit cleanup.

Could this leak the enable count and cause a kernel warning during devres
cleanup?

>  	/*
>  	 * Currently, of_regulator_bulk_get_all() is the only regulator API that
>  	 * allows to get all supplies in the devicetree node without manually
>  	 * specifying them.
[ ... ]
> @@ -573,6 +608,11 @@ static void pwrseq_pcie_m2_remove(struct platform_device *pdev)
>  	pwrseq_pcie_m2_remove_serdev(ctx, NULL);
>  	mutex_destroy(&ctx->list_lock);
>  
> +	if (ctx->vaux_reg && ctx->vaux_enabled) {
> +		regulator_disable(ctx->vaux_reg);
> +		ctx->vaux_enabled = false;
> +	}
> +

[Severity: High]
Are these accesses to ctx->vaux_enabled and ctx->vaux_reg protected against
concurrent modification?

This check and regulator_disable() are called without holding the
pwrseq->state_lock. Since devm_pwrseq_device_register() was used, the power
sequencer remains available to consumers until devres cleanup runs strictly
after pwrseq_pcie_m2_remove() completes.

If a consumer concurrently invokes pwrseq_power_on(),
pwrseq_pcie_m2_vregs_enable() executes and modifies ctx->vaux_enabled under
the lock. 

Could this race lead to an unbalanced disable warning or leave the regulator
disabled prematurely while a consumer thinks the power sequence completed
successfully?

>  	regulator_bulk_free(ctx->num_vregs, ctx->regs);

[Severity: High]
This is a pre-existing issue, but does this regulator_bulk_free() call
introduce a risk of dereferencing a NULL pointer?

The regulator_bulk_free() call explicitly sets consumers[i].consumer to
NULL. However, because the device was registered with
devm_pwrseq_device_register(), it is still active in the pwrseq core until
devres callbacks run.

If a consumer unbinds concurrently and invokes pwrseq_power_off(),
pwrseq_pcie_m2_vregs_disable() securely calls regulator_bulk_disable().

Could this cause a kernel crash when the bulk disable function blindly
dereferences the consumers[i].consumer pointers that were just set to NULL?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609033840.2006078-1-sherry.sun@oss.nxp.com?part=2