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

[sashiko-bot@kernel.org]

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