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

[Yixun Lan <dlan@kernel.org>]

Hi Ulf,

I'm seeking help for last review comment from sashiko, see my reply
at end of this mail..


On 08:22 Thu 12 Nov     , sashiko-bot@kernel.org wrote:
> 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_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.
> 
Ok, I see the point..
I was initially trying to log the id (pm_index) of power domain, but
with p->name, it's not necessary, I will fix it in next version.

I would completely remove member pm_index from struct which not useful..

> [ ... ]
> 
> > +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?
> 
This is an alter solution.. but just workaround the problem

> 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
> 

I've attempted to submit a patch to fix this issue, Ulf, can
you give an early comment on this? I'd like to see if it's a
right direction before I go further.. see below
https://lore.kernel.org/all/20261112-01-pm-device-link-v1-1-8932e0862378@kernel.org/

-- 
Yixun Lan (dlan)