Re: [PATCH 1/2] PM: EM: Add min/max available performance state limits

[Lukasz Luba <lukasz.luba@xxxxxxx>]

On 4/9/24 15:47, Hongyan Xia wrote:
> On 03/04/2024 17:23, Lukasz Luba wrote:
> > [...]
> >
> > diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> > index 927cc55ba0b3d..1a8b394251cb2 100644
> > --- a/kernel/power/energy_model.c
> > +++ b/kernel/power/energy_model.c
> > @@ -628,6 +628,8 @@ int em_dev_register_perf_domain(struct device 
> > *dev, unsigned int nr_states,
> >           goto unlock;
> >       dev->em_pd->flags |= flags;
> > +    dev->em_pd->min_ps = 0;
> > +    dev->em_pd->max_ps = nr_states - 1;
> >       em_cpufreq_update_efficiencies(dev, dev->em_pd->em_table->state);
> > @@ -856,3 +858,49 @@ int em_dev_update_chip_binning(struct device *dev)
> >       return em_recalc_and_update(dev, pd, em_table);
> >   }
> >   EXPORT_SYMBOL_GPL(em_dev_update_chip_binning);
> > +
> > +
> > +/**
> > + * em_update_performance_limits() - Update Energy Model with performance
> > + *                limits information.
> > + * @pd            : Performance Domain with EM that has to be updated.
> > + * @freq_min_khz    : New minimum allowed frequency for this device.
> > + * @freq_max_khz    : New maximum allowed frequency for this device.
> > + *
> > + * This function allows to update the EM with information about 
> > available
> > + * performance levels. It takes the minimum and maximum frequency in kHz
> > + * and does internal translation to performance levels.
> > + * Returns 0 on success or -EINVAL when failed.
> > + */
> > +int em_update_performance_limits(struct em_perf_domain *pd,
> > +        unsigned long freq_min_khz, unsigned long freq_max_khz)
> > +{
> > +    struct em_perf_state *table;
> > +    int min_ps = -1;
> > +    int max_ps = -1;
> > +    int i;
> > +
> > +    if (!pd)
> > +        return -EINVAL;
> > +
> > +    rcu_read_lock();
> > +    table = em_perf_state_from_pd(pd);
> > +
> > +    for (i = 0; i < pd->nr_perf_states; i++) {
> > +        if (freq_min_khz == table[i].frequency)
> > +            min_ps = i;
> > +        if (freq_max_khz == table[i].frequency)
> > +            max_ps = i;
> > +    }
> > +    rcu_read_unlock();
> > +
> > +    /* Only update when both are found and sane */
> > +    if (min_ps < 0 || max_ps < 0 || max_ps < min_ps)
> > +        return -EINVAL;
> > +
> > +    pd->min_ps = min_ps;
> > +    pd->max_ps = max_ps;
>
> Are we sure we are protected against multiple simultaneous updates? Or 
> is this a concern for the caller?
>
> The rest of the patch LGTM.

I've tried to make it running fast for only one caller. Although,
if someone would like to use it from many places then locking should be
handled under in function (and I will use the existing mutex for it).

I'll change it. Thanks for the review.