On Friday, September 17, 2010, Andrew Morton wrote:
> On Thu, 16 Sep 2010 20:29:33 -0500
> Nishanth Menon <nm@xxxxxx> wrote:
>
> > SOCs have a standard set of tuples consisting of frequency and
> > voltage pairs that the device will support per voltage domain. These
> > are called Operating Performance Points or OPPs. The actual
> > definitions of Operating Performance Points varies over silicon within the
> > same family of devices. For a specific domain, you can have a set of
> > {frequency, voltage} pairs. As the kernel boots and more information
> > is available, a set of these are activated based on the precise nature
> > of device the kernel boots up on. It is interesting to remember that
> > each IP which belongs to a voltage domain may define their own set of
> > OPPs on top of this.
> >
> > To implement an OPP, some sort of power management support is necessary
> > hence this library enablement depends on CONFIG_PM, however this does
> > not fit into the core power framework as it is an independent library.
> > This is hence introduced under lib allowing all architectures to
> > selectively enable the feature based on thier capabilities.
> >
> > Contributions include:
> > Sanjeev Premi for the initial concept:
> > http://patchwork.kernel.org/patch/50998/
> > Kevin Hilman for converting original design to device-based
> > Kevin Hilman and Paul Walmsey for cleaning up many of the function
> > abstractions, improvements and data structure handling
> > Romit Dasgupta for using enums instead of opp pointers
> > Thara Gopinath, Eduardo Valentin and Vishwanath BS for fixes and
> > cleanups.
> > Linus Walleij for recommending this layer be made generic for usage
> > in other architectures beyond OMAP and ARM.
> >
> > Discussions and comments from:
> > http://marc.info/?l=linux-omap&m=126033945313269&w=2
> > http://marc.info/?l=linux-omap&m=125482970102327&w=2
> > http://marc.info/?t=125809247500002&r=1&w=2
> > http://marc.info/?l=linux-omap&m=126025973426007&w=2
> > http://marc.info/?t=128152609200064&r=1&w=2
> > incorporated.
> >
> > ...
> >
> > Documentation/power/00-INDEX | 2 +
> > include/linux/opp.h | 136 +++++++++++++
> > kernel/power/Kconfig | 14 ++
> > lib/Makefile | 2 +
> > lib/opp.c | 440 ++++++++++++++++++++++++++++++++++++++++++
>
> ./lib/ is an unusual place to put a driver-like thing such as this.
> The lib/ directory is mainly for generic kernel-wide support things.
> I'd suggest that ./drivers/opp/ would be a better place.
Well, there are a few similar things in drivers/base/power already.
I agree with all of your comments below.
Thanks,
Rafael
> > ...
> >
> > +/*
> > + * Initialization wrapper used to define an OPP.
> > + * To point at the end of a terminator of a list of OPPs,
> > + * use OPP_DEF(0, 0, 0)
> > + */
> > +#define OPP_DEF(_enabled, _freq, _uv) \
> > +{ \
> > + .enabled = _enabled, \
> > + .freq = _freq, \
> > + .u_volt = _uv, \
> > +}
>
> OPP_DEF is a somewhat atypical name. OPP_INITIALIZER would be more
> conventional.
>
> However OPP_DEF has no usage in this patch so perhaps this can be
> removed?
>
> > +static LIST_HEAD(dev_opp_list);
>
> There's no locking for this list. That's OK under some circumstances,
> but I do think there should be a comment here explaining this apparent
> bug: why is no locking needed, what are the lifetime rules for entries
> on this list.
>
> Also, the _ordering_ of items on this list is significant. It should
> also be documented.
>
> >
> > ...
> >
> > +/**
> > + * opp_get_voltage() - Gets the voltage corresponding to an opp
>
> Usually the () is omitted from function names in kerneldoc comments.
> It might be OK, or it might produce strange output - I haven't
> checked.
>
> >
> > ...
> >
> > +/**
> > + * opp_find_freq_exact() - search for an exact frequency
> > + * @dev: device for which we do this operation
> > + * @freq: frequency to search for
> > + * @enabled: enabled/disabled OPP to search for
> > + *
> > + * Searches for exact match in the opp list and returns handle to the matching
>
> s/handle/pointer/
>
> > + * opp if found, else returns ERR_PTR in case of error and should be handled
> > + * using IS_ERR.
> > + *
> > + * Note: enabled is a modifier for the search. if enabled=true, then the match
> > + * is for exact matching frequency and is enabled. if false, the match is for
> > + * exact frequency which is disabled.
> > + */
> >
> > ...
> >
> > +int opp_add(struct device *dev, const struct opp_def *opp_def)
> > +{
> > + struct device_opp *tmp_dev_opp, *dev_opp = NULL;
> > + struct opp *opp, *new_opp;
> > + struct list_head *head;
> > +
> > + /* Check for existing list for 'dev' */
> > + list_for_each_entry(tmp_dev_opp, &dev_opp_list, node) {
> > + if (dev == tmp_dev_opp->dev) {
> > + dev_opp = tmp_dev_opp;
> > + break;
> > + }
> > + }
> > +
> > + if (!dev_opp) {
> > + /* Allocate a new device OPP table */
> > + dev_opp = kzalloc(sizeof(struct device_opp), GFP_KERNEL);
> > + if (!dev_opp) {
> > + pr_warning("%s: unable to allocate device struct\n",
> > + __func__);
> > + return -ENOMEM;
> > + }
> > +
> > + dev_opp->dev = dev;
> > + INIT_LIST_HEAD(&dev_opp->opp_list);
> > +
> > + list_add(&dev_opp->node, &dev_opp_list);
> > + }
> > +
> > + /* allocate new OPP node */
> > + new_opp = kzalloc(sizeof(struct opp), GFP_KERNEL);
> > + if (!new_opp) {
> > + if (list_empty(&dev_opp->opp_list)) {
> > + list_del(&dev_opp->node);
>
> It would be neater to move the list_add() down to after the allocation
> of new_opp and to remove this list_del().
>
> > + kfree(dev_opp);
> > + }
> > + pr_warning("%s: unable to allocate new opp node\n",
> > + __func__);
> > + return -ENOMEM;
> > + }
> > + opp_populate(new_opp, opp_def);
> > +
> > + /* Insert new OPP in order of increasing frequency */
> > + head = &dev_opp->opp_list;
> > + list_for_each_entry_reverse(opp, &dev_opp->opp_list, node) {
> > + if (new_opp->rate >= opp->rate) {
> > + head = &opp->node;
> > + break;
> > + }
> > + }
> > + list_add(&new_opp->node, head);
> > + dev_opp->opp_count++;
> > + if (new_opp->enabled)
> > + dev_opp->enabled_opp_count++;
>
> These non-atomic read-modify-write operations on *dev_opp have no
> locking. What prevents races here?
>
> > + return 0;
> > +}
> > +
> >
> > ...
> >
> > +void opp_init_cpufreq_table(struct device *dev,
> > + struct cpufreq_frequency_table **table)
> > +{
> > + struct device_opp *dev_opp;
> > + struct opp *opp;
> > + struct cpufreq_frequency_table *freq_table;
> > + int i = 0;
> > +
> > + dev_opp = find_device_opp(dev);
> > + if (IS_ERR(dev_opp)) {
> > + pr_warning("%s: unable to find device\n", __func__);
> > + return;
> > + }
> > +
> > + freq_table = kzalloc(sizeof(struct cpufreq_frequency_table) *
> > + (dev_opp->enabled_opp_count + 1), GFP_ATOMIC);
> > + if (!freq_table) {
> > + pr_warning("%s: failed to allocate frequency table\n",
> > + __func__);
> > + return;
> > + }
> > +
> > + list_for_each_entry(opp, &dev_opp->opp_list, node) {
> > + if (opp->enabled) {
> > + freq_table[i].index = i;
> > + freq_table[i].frequency = opp->rate / 1000;
> > + i++;
> > + }
> > + }
> > +
> > + freq_table[i].index = i;
> > + freq_table[i].frequency = CPUFREQ_TABLE_END;
> > +
> > + *table = &freq_table[0];
> > +}
>
> So we're playing with cpufreq internals here but there's no #ifdef
> CONFIG_CPUFREQ and there's no Kconfig dependency on cpufreq. That
> needs fixing I think, if only from a reduce-code-bloat perspective.
>
>
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
