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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Unclocked MMIO access in `is_enabled` causes synchronous external abort.
- [High] Clock reference count underflow due to unbalanced disable.
- [Medium] Missing validation for zero clocks returned by `devm_clk_bulk_get_all()`.
--

commit 0bec5912036c4b180d8ea012798dcb554afa8119
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 clock handling since this SoC requires explicitly
enabling clocks to bring up the PHYs properly.

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

[ ... ]

> @@ -62,6 +77,42 @@ static int qcom_sdm845_refgen_is_enabled(struct regulator_dev *rdev)
>  	return 1;
>  }
>  
> +static int qcom_ipq9650_refgen_enable(struct regulator_dev *rdev)
> +{
> +	struct qcom_refgen_drvdata *drvdata = rdev_get_drvdata(rdev);
> +	int ret;
> +
> +	ret = clk_bulk_prepare_enable(drvdata->num_clks, drvdata->clks);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +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);

[Severity: High]
If the bootloader leaves the REFGEN block enabled, the regulator framework
will treat this as boot-on and might skip calling the enable callback when
the first consumer enables it. When a consumer later disables it (or when
regulator_disable_unused() runs), this disable callback will execute
clk_bulk_disable_unprepare().

Since the driver never called clk_bulk_prepare_enable() itself, will this
cause the Linux clock framework's internal reference counts to underflow
and trigger warnings?

> +
> +	return 0;
> +}
> +
> +static const struct regulator_desc ipq9650_refgen_desc = {
> +	.enable_reg = REFGEN_REG_REFGEN_STATUS,
> +	.enable_mask = REFGEN_STATUS_OUT_MASK,
> +	.enable_val = REFGEN_STATUS_OUT_ENABLE,
> +	.enable_time = 5,
> +	.name = "refgen",
> +	.owner = THIS_MODULE,
> +	.type = REGULATOR_CURRENT,
> +	.ops = &(const struct regulator_ops) {
> +		.enable		= qcom_ipq9650_refgen_enable,
> +		.disable	= qcom_ipq9650_refgen_disable,
> +		.is_enabled	= regulator_is_enabled_regmap,

[Severity: Critical]
During devm_regulator_register(), the regulator framework invokes the
is_enabled callback to determine the initial hardware state. Because
regulator_is_enabled_regmap() directly performs a regmap_read(), and
qcom_refgen_probe() only retrieves the clocks without enabling them,
will this read occur while the clocks are gated?

On Qualcomm platforms, does accessing MMIO registers of a clock-gated
peripheral stall the bus and trigger a synchronous external abort?

> +	},
> +};
> +
>  static const struct regulator_desc sdm845_refgen_desc = {

[ ... ]

> @@ -106,10 +172,23 @@ static int qcom_refgen_probe(struct platform_device *pdev)
>  	struct regmap *regmap;
>  	void __iomem *base;
>  
> -	rdesc = of_device_get_match_data(dev);
> -	if (!rdesc)
> +	data = of_device_get_match_data(dev);
> +	if (!data)
>  		return -ENODATA;
>  
> +	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]
If the device tree node improperly omits the clocks property,
devm_clk_bulk_get_all() will return 0. Does this allow the probe
function to silently succeed?

Later calls to clk_bulk_prepare_enable() will also succeed without
enabling any hardware. Since clocks are mandatory on IPQ9650, should
this treat a return value of 0 as an error?

> +			return dev_err_probe(dev, drvdata->num_clks,
> +					     "failed to get clocks\n");
> +	}
> +
> +	rdesc = data->rdesc;
> +
>  	base = devm_platform_ioremap_resource(pdev, 0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611-ipq9650_refgen-v2-0-d96a91d5b99e@oss.qualcomm.com?part=3