Re: [PATCH v4 3/3] regulator: qcom-refgen: add support for the IPQ9650 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Unclocked MMIO read in get_status callback causes system crash.
- [Medium] Silent failure on missing clocks due to inadequate DT validation.
--

commit 90356084cf49412e26be88f69da7b99d50c27462
Author: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>

regulator: qcom-refgen: add support for the IPQ9650 SoC

This commit adds support for the IPQ9650 SoC to the qcom-refgen regulator
driver. It introduces explicit clock handling for this block and a get_status
callback to report the regulator status.

> diff --git a/drivers/regulator/qcom-refgen-regulator.c b/drivers/regulator/qcom-refgen-regulator.c
> index 6a3795469927e..cc72e5a43ba62 100644
> --- a/drivers/regulator/qcom-refgen-regulator.c
> +++ b/drivers/regulator/qcom-refgen-regulator.c

[ ... ]

> +static int qcom_ipq9650_refgen_disable(struct regulator_dev *rdev)
> +{
> +	struct qcom_refgen_drvdata *drvdata = rdev_get_drvdata(rdev);
> +
> +	clk_bulk_disable_unprepare(drvdata->num_clks, drvdata->clks);
> +
> +	drvdata->is_enabled = false;

[Severity: Critical]
Does this disable the clocks before updating the internal state?

If an is_enabled check were added to get_status() to prevent unclocked reads,
could a concurrent call to get_status() still crash the system if it
interleaves after clk_bulk_disable_unprepare() but before is_enabled is
set to false?

> +
> +	return 0;
> +}

[ ... ]

> +static int qcom_ipq9650_refgen_get_status(struct regulator_dev *rdev)
> +{
> +	u32 val;
> +
> +	regmap_read(rdev->regmap, REFGEN_REG_REFGEN_STATUS, &val);

[Severity: Critical]
Can this unclocked MMIO read cause a system crash?

The get_status() callback reads the REFGEN_REG_REFGEN_STATUS register without
ensuring the regulator's clocks are enabled. Since user space can trigger a
read of /sys/class/regulator/regulator.X/status at any time, which invokes
get_status() locklessly, a disabled regulator could cause this regmap_read()
to trigger a synchronous external abort and hang the bus.

> +	if (FIELD_GET(REFGEN_STATUS_OUT_MASK, val))
> +		return REGULATOR_STATUS_ON;
> +
> +	return REGULATOR_STATUS_OFF;
> +}

[ ... ]

> @@ -106,10 +192,23 @@ static int qcom_refgen_probe(struct platform_device *pdev)

[ ... ]

> +	if (data->has_clocks) {
> +		drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +		if (!drvdata)
> +			return -ENOMEM;
> +
> +		drvdata->num_clks = devm_clk_bulk_get_all(dev, &drvdata->clks);
> +		if (drvdata->num_clks < 0)

[Severity: Medium]
Will this silently fail if the clocks are missing from the device tree?

If the clocks property is omitted, devm_clk_bulk_get_all() returns 0.
Since this code only checks for drvdata->num_clks < 0, it treats the missing
clocks as a success, and subsequent calls to clk_bulk_prepare_enable(0, ...)
will silently succeed without enabling any clocks.

Should this explicitly verify drvdata->num_clks > 0 since clocks are mandatory
for the IPQ9650 PHY to function properly?

> +			return dev_err_probe(dev, drvdata->num_clks,
> +					     "failed to get clocks\n");
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-ipq9650_refgen-v4-0-c505ea6c6661@oss.qualcomm.com?part=3