Re: [PATCH v11 6/9] regulator: p3h2x4x: Add driver for on-die regulators in NXP P3H2x4x i3c hub

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Concurrent hardware operations on different regulators share the same protection lock register without a cross-device shared lock, leading to a race condition where regulator writes fail silently.
- [High] I/O errors during hardware unlock are silently ignored, leading to false success returns for regulator operations.
--

commit 8fb846835511c487dd04df576ca53fdbc089839a
Author: Aman Kumar Pandey <[email protected]>
Subject: regulator: p3h2x4x: Add driver for on-die regulators in NXP P3H2x4x i3c hub

The NXP P3H2x4x family integrates on-die regulators alongside I3C hub
functionality. This driver registers the regulators using the MFD
framework and exposes them via the regulator subsystem.

> diff --git a/drivers/regulator/p3h2840_i3c_hub_regulator.c b/drivers/regulator/p3h2840_i3c_hub_regulator.c
> new file mode 100644
> index 0000000000000..4f2514d4d928e
> --- /dev/null
> +++ b/drivers/regulator/p3h2840_i3c_hub_regulator.c

[ ... ]

> +static void p3h2x4x_reg_guard_enter(struct regulator_dev *rdev,
> +				    struct p3h2x4x_reg_state *state)
> +{
> +	state->restore = false;
> +
> +	if (regmap_read(rdev->regmap,
> +			P3H2X4X_DEV_REG_PROTECTION_CODE,
> +			&state->orig))
> +		return;

[Severity: High]
If this regmap_read() fails, the function returns silently since it
has a void return type. Will this cause subsequent regulator operations
like regulator_enable_regmap() to execute blindly on locked hardware, 
ignoring the configuration write but returning a false success to the caller?

> +
> +	if (state->orig != P3H2X4X_REGISTERS_UNLOCK_CODE) {
> +		regmap_write(rdev->regmap,
> +			     P3H2X4X_DEV_REG_PROTECTION_CODE,
> +			     P3H2X4X_REGISTERS_UNLOCK_CODE);
> +		state->restore = true;
> +	}
> +}
> +
> +static void p3h2x4x_reg_guard_exit(struct regulator_dev *rdev,
> +				   struct p3h2x4x_reg_state *state)
> +{
> +	if (state->restore)
> +		regmap_write(rdev->regmap,
> +			     P3H2X4X_DEV_REG_PROTECTION_CODE,
> +			     state->orig);
> +}
> +
> +DEFINE_LOCK_GUARD_1(p3h2x4x_reg, struct regulator_dev,
> +		    p3h2x4x_reg_guard_enter(_T->lock, &_T->state),
> +		    p3h2x4x_reg_guard_exit(_T->lock, &_T->state),
> +		    struct p3h2x4x_reg_state state);
> +
> +static int p3h2x4x_regulator_enable(struct regulator_dev *rdev)
> +{
> +	guard(p3h2x4x_reg)(rdev);

[Severity: High]
Could there be a race condition here between different regulator instances?

The P3H2X4X_DEV_REG_PROTECTION_CODE register appears to be a shared
device-wide resource, but guard(p3h2x4x_reg) relies on rdev->mutex, which
only serializes operations for a single regulator instance.

If two threads access different regulators (e.g., ldo-cp0 and ldo-cp1) 
concurrently, could they interleave like this:

Thread A reads state->orig, sees locked, writes unlock, sets restore=true
Thread B reads state->orig, sees unlocked, sets restore=false
Thread A completes its update, runs guard_exit, and relocks the device
Thread B attempts its regulator update, but the hardware is now locked

Would it be safer to use a shared device-level lock in the parent MFD
device to protect the hardware protection register?

> +	return regulator_enable_regmap(rdev);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c