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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Race condition in hardware unlock/lock sequence across regulators
- [Medium] Silent hardware state mismatch due to unhandled errors in lock guard
- [Medium] Missing MODULE_ALIAS prevents automatic module loading
- [Low] Incorrect use of of_match_ptr() on string literals
--

commit 4a0c2a87632650e322585272d36928a08ab2888d
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>

regulator: p3h2x4x: Add driver for on-die regulators in NXP P3H2x4x i3c hub

This commit adds a platform driver for the on-die voltage regulators of the
NXP P3H2x4x multi-function I3C Hub device, registering them through the
regulator framework via MFD.

[ ... ]

> 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;
> +
> +	if (state->orig != P3H2X4X_REGISTERS_UNLOCK_CODE) {
> +		regmap_write(rdev->regmap,
> +			     P3H2X4X_DEV_REG_PROTECTION_CODE,
> +			     P3H2X4X_REGISTERS_UNLOCK_CODE);

[Severity: Medium]
Does this silently ignore hardware I/O errors?

If the I3C/I2C write to unlock the hardware fails, the hardware remains
locked. Because guard constructors cannot propagate errors, the caller will
proceed to modify the regulator configuration. The regmap_update_bits() call
will succeed in software but fail in hardware, causing the kernel's tracking
of the regulator's state to diverge from the physical hardware.

> +		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);

[Severity: High]
Can this cause a race condition between different regulators on the same chip?

The p3h2x4x_reg guard is instantiated per-regulator, and the regulator
subsystem only holds a per-regulator lock. If Thread A unlocks the hardware
for ldo-cp0, and Thread B concurrently reads the protection register for
ldo-cp1, Thread B may observe the hardware as already unlocked and skip its
own unlock/restore sequence.

When Thread A completes, it relocks the device. Thread B's subsequent
register update will then be written to a locked hardware state, causing
the write to be discarded and leading to silent state mismatch. Should a
shared lock from the MFD parent be used to ensure atomicity?

> +
> +static int p3h2x4x_regulator_enable(struct regulator_dev *rdev)
> +{
> +	guard(p3h2x4x_reg)(rdev);
> +	return regulator_enable_regmap(rdev);
> +}

[ ... ]

> +static struct regulator_desc p3h2x4x_regulators[] = {
> +	{
> +		.name = "ldo-cp0",
> +		.of_match = of_match_ptr("ldo-cp0"),
> +		.regulators_node = of_match_ptr("regulators"),

[Severity: Low]
Is it safe to use of_match_ptr() on string literals here?

On non-OF builds, of_match_ptr() evaluates to NULL. This breaks regulator
matching for systems that use non-OF firmware interfaces (like ACPI PRP0001
or swnodes) which still rely on these strings to be present. Applying it
to string literals doesn't save any memory since the strings are already
compiled in for the name field.

> +		.volt_table = p3h2x4x_voltage_table,
> +		.n_voltages = ARRAY_SIZE(p3h2x4x_voltage_table),
> +		.ops = &p3h2x4x_ldo_ops,

[ ... ]

> +static struct platform_driver p3h2x4x_regulator_driver = {
> +	.driver = {
> +		.name = "p3h2x4x-regulator",
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +	},
> +	.probe = p3h2x4x_regulator_probe,
> +};
> +module_platform_driver(p3h2x4x_regulator_driver);
> +
> +MODULE_AUTHOR("Aman Kumar Pandey <aman.kumarpandey@nxp.com>");
> +MODULE_AUTHOR("Vikash Bansal <vikash.bansal@nxp.com>");
> +MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@nxp.com>");
> +MODULE_DESCRIPTION("NXP P3H2X4X I3C HUB Regulator driver");
> +MODULE_LICENSE("GPL");

[Severity: Medium]
Does this prevent automatic module loading?

The driver registers as an MFD child device using a platform driver named
"p3h2x4x-regulator". Because it relies on the platform bus matching by name
and does not provide an id_table or of_match_table, the MFD core will emit
a uevent with MODALIAS=platform:p3h2x4x-regulator. Without a corresponding
MODULE_ALIAS declaration, udev cannot map the uevent to the kernel module.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525064209.2263045-1-lakshay.piplani@nxp.com?part=6