Re: [PATCH v12 4/7] 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] Concurrent regulator operations race on the shared device-level protection register, leading to silent hardware configuration failures.
- [Medium] The driver module will not auto-load because it lacks a MODULE_ALIAS or platform_device_id table.
- [Medium] If regmap_read or the unlock sequence fails in the guard constructor, the driver proceeds with the regulator operation on locked registers.
- [Low] Typo in regulator config assignment assigns dev->of_node to itself instead of rcfg.of_node.
--

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

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.

> --- /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: Medium]
If regmap_read() fails here, the function returns early and swallows the
error since the guard constructor returns void. Will this cause the caller
to proceed with regulator operations on locked hardware, potentially
resulting in silent hardware configuration failures? 

If the device ACKs the transaction despite ignoring the write, wouldn't
the driver incorrectly report success to the regulator core?

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

[Severity: High]
Does this guard provide mutual exclusion across the four different regulators
on the same device? 

The regmap lock protects individual I/O operations, but since there is no
device-level lock held across the unlock/modify/lock sequence, can concurrent
operations on different regulators interleave?

For example, could one thread re-lock the device in p3h2x4x_reg_guard_exit()
while another thread is just about to write its configuration, causing the
hardware write to be silently ignored? Is a shared device-level mutex needed
to serialize these sequences?

[ ... ]

> +static int p3h2x4x_regulator_probe(struct platform_device *pdev)
> +{
> +	struct p3h2x4x_dev *p3h2x4x = dev_get_drvdata(pdev->dev.parent);
> +	struct p3h2x4x_regulator_dev *p3h2x4x_regulator;
> +	struct regulator_config rcfg = { };
> +	struct device *dev = &pdev->dev;
> +	struct regulator_dev *rdev;
> +	int i;
> +
> +	p3h2x4x_regulator = devm_kzalloc(dev, sizeof(*p3h2x4x_regulator), GFP_KERNEL);
> +	if (!p3h2x4x_regulator)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, p3h2x4x_regulator);
> +
> +	p3h2x4x_regulator->regmap = p3h2x4x->regmap;
> +	device_set_of_node_from_dev(dev, dev->parent);
> +
> +	rcfg.dev = dev;
> +	rcfg.dev->of_node = dev->of_node;

[Severity: Low]
Should this be rcfg.of_node = dev->of_node? Since rcfg.dev is assigned to dev
on the previous line, this just assigns dev->of_node to itself.

> +	rcfg.regmap = p3h2x4x_regulator->regmap;
> +	rcfg.driver_data = p3h2x4x_regulator;

[ ... ]

> +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]
Since the driver is registered as a platform driver with the name
"p3h2x4x-regulator" but the module is compiled as
"p3h2840_i3c_hub_regulator.ko", will this module fail to auto-load? 

Without a MODULE_ALIAS("platform:p3h2x4x-regulator") or an exported
platform_device_id table, udev may not be able to load the module when the
MFD core creates the child platform device.

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

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c