On Tue, Mar 14, 2017 at 08:46:52PM +0000, Jon Hunter wrote:
> On 14/03/17 19:15, Thierry Reding wrote:
[...]
> > diff --git a/drivers/soc/tegra/powergate-bpmp.c b/drivers/soc/tegra/powergate-bpmp.c
[...]
> > +static struct tegra_powergate *
> > +tegra_powergate_add(struct tegra_bpmp *bpmp,
> > + const struct tegra_powergate_info *info)
> > +{
> > + struct tegra_powergate *powergate;
> > + bool off;
> > +
> > + off = !tegra_bpmp_powergate_is_powered(bpmp, info->id);
> > +
> > + powergate = devm_kzalloc(bpmp->dev, sizeof(*powergate), GFP_KERNEL);
> > + if (!powergate)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + powergate->id = info->id;
> > + powergate->bpmp = bpmp;
> > +
> > + powergate->genpd.name = kstrdup(info->name, GFP_KERNEL);
> > + powergate->genpd.power_on = tegra_powergate_power_on;
> > + powergate->genpd.power_off = tegra_powergate_power_off;
> > +
> > + pm_genpd_init(&powergate->genpd, NULL, off);
>
> There was a recent change made so that pm_genpd_init() now returns
> success/failure so we should check it.
Okay, will do.
> > +static void tegra_powergate_remove(struct tegra_powergate *powergate)
> > +{
> > + struct generic_pm_domain *genpd = &powergate->genpd;
> > + struct tegra_bpmp *bpmp = powergate->bpmp;
> > + int err;
> > +
> > + err = pm_genpd_remove(genpd);
> > + if (err < 0)
> > + dev_err(bpmp->dev, "failed to remove power domain %s: %d\n",
> > + genpd->name, err);
> > +
> > + kfree(genpd->name);
>
> Should we still free the memory here on failure?
I think at this point we loose either way. In all current cases this
should not fail, since we only use this to remove domains on probe
failure, so before the provider is registered and before any devices
actually start using the domains.
In the case where we want to remove upon driver unload, it's fairly
unlikely that freeing the memory or not would be relevant. Not freeing
it would mean leaking the memory because the driver will go away and
hence there won't be a chance to clean up afterwards. And given that
the driver will disappear anyway, further access to the genpd is very
likely to cause a crash, due to some other resources being cleaned up
as part of the driver removal.
[...]
> > +int tegra_bpmp_init_powergates(struct tegra_bpmp *bpmp)
> > +{
> > + struct device_node *np = bpmp->dev->of_node;
> > + struct tegra_powergate_info *powergates;
> > + struct device *dev = bpmp->dev;
> > + unsigned int count, i;
> > + int err;
> > +
> > + err = tegra_bpmp_probe_powergates(bpmp, &powergates);
> > + if (err < 0)
> > + return err;
> > +
> > + count = err;
> > +
> > + dev_dbg(dev, "%u power domains probed\n", count);
> > +
> > + err = tegra_bpmp_add_powergates(bpmp, powergates, count);
> > + if (err < 0)
> > + goto free;
> > +
> > + bpmp->genpd.xlate = tegra_powergate_xlate;
> > +
> > + err = of_genpd_add_provider_onecell(np, &bpmp->genpd);
> > + if (err < 0) {
> > + dev_err(dev, "failed to add power domain provider: %d\n", err);
> > + goto remove;
> > + }
> > +
> > +free:
> > + for (i = 0; i < count; i++)
> > + kfree(powergates[i].name);
> > +
> > + kfree(powergates);
> > + return err;
> > +
> > +remove:
> > + tegra_bpmp_remove_powergates(bpmp);
> > + goto free;
>
> Nit ... why not move the call to tegra_bpmp_remove_powergates() to where
> we have the 'goto remove' and avoid this extra jump?
Yeah, I can do that. The original motivation for doing it this way was
to make it less error prone to leak the domains if new code was added,
but that seems fairly unlikely and we can catch that during review.
Thierry
Attachment:
signature.asc
Description: PGP signature
