Re: [PATCH v8 4/8] PM / EM: add support for other devices than CPUs in Energy Model

[Lukasz Luba]

Hi Daniel,

On 6/1/20 10:44 PM, Daniel Lezcano wrote:
> On 27/05/2020 11:58, Lukasz Luba wrote:
> > Add support for other devices than CPUs. The registration function
> > does not require a valid cpumask pointer and is ready to handle new
> > devices. Some of the internal structures has been reorganized in order to
> > keep consistent view (like removing per_cpu pd pointers).
> >
> > Signed-off-by: Lukasz Luba <lukasz.luba@xxxxxxx>
> > ---
>
> [ ... ]
>
> >   }
> >   EXPORT_SYMBOL_GPL(em_register_perf_domain);
> > +
> > +/**
> > + * em_dev_unregister_perf_domain() - Unregister Energy Model (EM) for a device
> > + * @dev		: Device for which the EM is registered
> > + *
> > + * Try to unregister the EM for the specified device (but not a CPU).
> > + */
> > +void em_dev_unregister_perf_domain(struct device *dev)
> > +{
> > +	if (IS_ERR_OR_NULL(dev) || !dev->em_pd)
> > +		return;
> > +
> > +	if (_is_cpu_device(dev))
> > +		return;
> > +
> > +	mutex_lock(&em_pd_mutex);
>
> Is the mutex really needed?

I just wanted to align this unregister code with register. Since there
is debugfs dir lookup and the device's EM existence checks I thought it
wouldn't harm just to lock for a while and make sure the registration
path is not used. These two paths shouldn't affect each other, but with
modules loading/unloading I wanted to play safe.
I can change it maybe to just dmb() and the end of the function if it's
a big performance problem in this unloading path. What do you think?

> If this function is called that means there is no more user of the
> em_pd, no?

True, that EM users should already be unregistered i.e. thermal cooling.

> > +	em_debug_remove_pd(dev);
> > +
> > +	kfree(dev->em_pd->table);
> > +	kfree(dev->em_pd);
> > +	dev->em_pd = NULL;
> > +	mutex_unlock(&em_pd_mutex);
> > +}
> > +EXPORT_SYMBOL_GPL(em_dev_unregister_perf_domain);

Thank you for reviewing this.

Regards,
Lukasz