Re: [PATCH v4 2/2] pmdomain: spacemit: Add power domain driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] pmdomain: spacemit: devres frees active domains when
  pm_genpd_remove() fails
- [Low] pmdomain: spacemit: log ret instead of pm_index on power
  transition error
--

commit 1bedd4d63c29dbe88ab766b2b331bdd5d01ee76c
Author: Yixun Lan <dlan@kernel.org>

pmdomain: spacemit: Add power domain driver

This commit adds a generic power domain driver for SpacemiT K1 and K3 SoCs
as an auxiliary bus driver supporting both software and hardware power
sequencing modes.

> diff --git a/drivers/pmdomain/spacemit/spacemit-pm-domains.c b/drivers/pmdomain/spacemit/spacemit-pm-domains.c
> new file mode 100644
> index 0000000000000..dd0613c1912a0
> --- /dev/null
> +++ b/drivers/pmdomain/spacemit/spacemit-pm-domains.c
> @@ -0,0 +1,424 @@

[ ... ]

> +static int spacemit_pd_power_off(struct generic_pm_domain *domain)
> +{
> +	struct spacemit_pm_domain *pd = to_spacemit_pd(domain);
> +	struct spacemit_pmu *pmu = pd->pmu;
> +	const struct spacemit_pm_domain_param *p = pd->param;
> +	unsigned int val;
> +	int ret;
> +
> +	if (!pd->param->use_hw) {
> +		regmap_clear_bits(pmu->regmap, p->reg_pwr_ctrl, BIT(p->bit_isolation));
> +		fsleep(15);
> +
> +		val = BIT(p->bit_sleep1) | BIT(p->bit_sleep2);
> +		regmap_clear_bits(pmu->regmap, p->reg_pwr_ctrl, val);
> +		fsleep(15);
> +
> +		ret = regmap_read_poll_timeout(pmu->regmap,
> +					       APMU_POWER_STATUS_REG,
> +					       val,
> +					       !(val & BIT(p->bit_pwr_stat)),
> +					       5,
> +					       APMU_POWER_TIMEOUT_US);
> +	} else {
> +		val = BIT(p->bit_auto_pwr_on) | BIT(p->bit_hw_mode);
> +		regmap_clear_bits(pmu->regmap, p->reg_pwr_ctrl, val);
> +		fsleep(15);
> +
> +		ret = regmap_read_poll_timeout(pmu->regmap,
> +					       APMU_POWER_STATUS_REG,
> +					       val,
> +					       !(val & BIT(p->bit_hw_pwr_stat)),
> +					       5,
> +					       APMU_POWER_TIMEOUT_US);
> +	}
> +
> +	if (ret)
> +		dev_err(&domain->dev, "failed to power off domain %s: %d\n",
> +			p->name, pd->pm_index);
> +
> +	return ret;
> +}
> +
> +static int spacemit_pd_power_on(struct generic_pm_domain *domain)
> +{
> +	struct spacemit_pm_domain *pd = to_spacemit_pd(domain);
> +	struct spacemit_pmu *pmu = pd->pmu;
> +	const struct spacemit_pm_domain_param *p = pd->param;
> +	unsigned int val;
> +	int ret = 0;
> +
> +	if (!p->use_hw) {
> +		regmap_set_bits(pmu->regmap, p->reg_pwr_ctrl, BIT(p->bit_sleep1));
> +		fsleep(20);
> +
> +		val = BIT(p->bit_sleep2) | BIT(p->bit_sleep1);
> +		regmap_set_bits(pmu->regmap, p->reg_pwr_ctrl, val);
> +		fsleep(20);
> +
> +		regmap_set_bits(pmu->regmap, p->reg_pwr_ctrl, BIT(p->bit_isolation));
> +		fsleep(15);
> +
> +		ret = regmap_read_poll_timeout(pmu->regmap,
> +					       APMU_POWER_STATUS_REG,
> +					       val,
> +					       (val & BIT(p->bit_pwr_stat)),
> +					       5,
> +					       APMU_POWER_TIMEOUT_US);
> +	} else {
> +		val = BIT(p->bit_auto_pwr_on) | BIT(p->bit_hw_mode);
> +		regmap_set_bits(pmu->regmap, p->reg_pwr_ctrl, val);
> +		fsleep(300);
> +
> +		ret = regmap_read_poll_timeout(pmu->regmap,
> +					       APMU_POWER_STATUS_REG,
> +					       val,
> +					       (val & BIT(p->bit_hw_pwr_stat)),
> +					       5,
> +					       APMU_POWER_TIMEOUT_US);
> +	}
> +
> +	if (ret)
> +		dev_err(&domain->dev, "failed to power on domain %s: %d\n",
> +			p->name, pd->pm_index);
> +
> +	return ret;
> +}

[Severity: Low]
As reported by Troy Mitchell on v3 [1], should these dev_err() calls in
spacemit_pd_power_off() and spacemit_pd_power_on() pass ret instead of
pd->pm_index so that the error code is logged rather than the 0-based
domain index?  With that changed, pd->pm_index can also be dropped.

[ ... ]

> +static int spacemit_pm_add_one_domain(struct spacemit_pmu *pmu, int id,
> +				      const struct spacemit_pm_domain_param *param)
> +{
> +	struct spacemit_pm_domain *pd;
> +	int ret;
> +
> +	pd = devm_kzalloc(pmu->dev, sizeof(*pd), GFP_KERNEL);
> +	if (!pd)
> +		return -ENOMEM;
> +
> +	pd->pm_index        = id;
> +	pd->param           = param;
> +	pd->pmu             = pmu;
> +	pd->genpd.name      = param->name;
> +	pd->genpd.flags     = param->flags;
> +	pd->genpd.power_off = spacemit_pd_power_off;
> +	pd->genpd.power_on  = spacemit_pd_power_on;
> +
> +	ret = pm_genpd_init(&pd->genpd, NULL, spacemit_pm_is_off_state(pmu, pd));
> +	if (ret)
> +		return ret;
> +
> +	pmu->domains[id] = &pd->genpd;
> +
> +	return 0;
> +}
> +
> +static void spacemit_pm_domain_cleanup(struct spacemit_pmu *pmu)
> +{
> +	struct spacemit_pm_domain *pd;
> +	int i, ret;
> +
> +	for (i = 0; i < pmu->genpd_data.num_domains; i++) {
> +		if (!pmu->domains[i])
> +			continue;
> +
> +		pd = to_spacemit_pd(pmu->domains[i]);
> +		ret = pm_genpd_remove(pmu->domains[i]);
> +		if (ret)
> +			dev_err(pmu->dev, "failed to remove domain %s: %d - state may be inconsistent\n",
> +				pd->param->name, ret);
> +	}
> +}

[ ... ]

> +static void spacemit_pm_domain_remove(struct auxiliary_device *adev)
> +{
> +	struct spacemit_pmu *pmu = auxiliary_get_drvdata(adev);
> +	struct device *dev = &adev->dev;
> +
> +	of_genpd_del_provider(dev->of_node);
> +	spacemit_pm_domain_cleanup(pmu);
> +}

[Severity: High]
As reported by Troy Mitchell on v3 [1], can unbinding this driver or
unloading the module while a consumer device is attached cause a
use-after-free on pd->genpd?

Because the auxiliary device shares its parent CCU platform device's
of_node, fw_devlink links consumer devices to the parent CCU device rather
than to adev->dev, so consumer devices are not detached when
spacemit_pm_domain_remove() runs.  pm_genpd_remove() then fails with
-EBUSY because genpd->device_count > 0, leaving pd->genpd linked in the
global gpd_list and referenced by consumers' dev->pm_domain pointers right
before devres frees the devm_kzalloc()-allocated struct
spacemit_pm_domain.

[1] https://lore.kernel.org/r/DMQHSDTR177I.1HWHAF4FWTFQ5@linux.spacemit.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-04-k3-pm-support-v4-0-de3931aa5153@kernel.org?part=2