Re: [PATCH] opp: introduce library for device-specific OPPs

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Friday, September 17, 2010, Andrew Morton wrote:
> On Thu, 16 Sep 2010 20:29:33 -0500
> Nishanth Menon <nm@ti.com> 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.
> 
>