Re: [PATCH V2 9/9] cpufreq: scmi: Use .register_em() callback

[Viresh Kumar <viresh.kumar@linaro.org>]

On 11-08-21, 17:32, Lukasz Luba wrote:
> 
> 
> On 8/11/21 12:58 PM, Viresh Kumar wrote:
> > Set the newly added .register_em() callback to register with the EM
> > after the cpufreq policy is properly initialized.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >   drivers/cpufreq/scmi-cpufreq.c | 55 ++++++++++++++++++++--------------
> >   1 file changed, 32 insertions(+), 23 deletions(-)
> 
> > +static void scmi_cpufreq_register_em(struct cpufreq_policy *policy)
> > +{
> > +	struct em_data_callback em_cb = EM_DATA_CB(scmi_get_cpu_power);
> > +	bool power_scale_mw = perf_ops->power_scale_mw_get(ph);
> > +	struct scmi_data *priv = policy->driver_data;
> > +
> > +	em_dev_register_perf_domain(get_cpu_device(policy->cpu), priv->nr_opp,
> > +				    &em_cb, priv->opp_shared_cpus,
> > +				    power_scale_mw);
> 
> I would free the priv->opp_shared_cpus mask here, since we don't
> need it anymore and memory can be reclaimed.

Yes, we don't need it anymore, but this isn't a good place to undo
what init() has done. Moreover, it is possible that register_em() may
not get called at all, if some error has occurred after init() has
successfully returned. It is always better to use exit() for such
things. It won't hurt a lot to keep this around anyway.

> Don't forget this
> setup would be called N CPUs times, on this per-CPU policy platform.

Yes, but EM will just ignore this call. Though I have made a change
here now to check for non-zero nr_opp to avoid the unnecessary call.

> If freed here, then also there wouldn't be a need to free it in
> scmi_cpufreq_exit() so you can remove it from there.

-- 
viresh