On Wed, Aug 31, 2011 at 12:29 AM, MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx> wrote:
[snip]
> +/**
> + * get_devfreq() - find devfreq struct. a wrapped find_device_devfreq()
> + * with mutex protection. exported for governors
> + * @dev: device pointer used to lookup device devfreq.
> + */
> +struct devfreq *get_devfreq(struct device *dev)
> +{
> + struct devfreq *ret;
> +
> + mutex_lock(&devfreq_list_lock);
> + ret = find_device_devfreq(dev);
> + mutex_unlock(&devfreq_list_lock);
You prevent changes to the devfreq list while searching (good) but
after returning the pointer there is no protection from that item
being removed from the list. Generally "get" and "put" functions do
more than just return a pointer: get functions often increment a
refcount, or hold a lock. The put function will decrement the
refcount or release the lock. Maybe you want something like the
following:
mutex_lock(&devfreq_list_lock);
ret = find_device_devfreq(dev);
mutex_lock(&devfreq->lock);
mutex_unlock(&devfreq_list_lock);
Then you need a corresponding put which does the mutex_unlock(&devfreq->lock).
It looks like the only consumers of get_devfreq are the sysfs
show/store interfaces, which immediately hold devfreq->lock, so the
above proposal certainly makes fits the existing use case.
Also CPUfreq's "cpufreq_get" function does a nice job of protecting
the object from getting freed with a rw_semaphore. It is a bit more
"complicated" but makes for good reading.
> +
> + return ret;
> +}
> +
> +/**
> + * devfreq_do() - Check the usage profile of a given device and configure
> + * frequency and voltage accordingly
> + * @devfreq: devfreq info of the given device
> + */
> +static int devfreq_do(struct devfreq *devfreq)
> +{
> + struct opp *opp;
> + unsigned long freq;
> + int err;
> +
Put the mutex_is_locked check here? See more below.
> + err = devfreq->governor->get_target_freq(devfreq, &freq);
> + if (err)
> + return err;
> +
> + opp = opp_find_freq_ceil(devfreq->dev, &freq);
> + if (opp == ERR_PTR(-ENODEV))
> + opp = opp_find_freq_floor(devfreq->dev, &freq);
> +
> + if (IS_ERR(opp))
> + return PTR_ERR(opp);
> +
> + if (devfreq->previous_freq == freq)
> + return 0;
> +
> + err = devfreq->profile->target(devfreq->dev, opp);
> + if (err)
> + return err;
> +
> + devfreq->previous_freq = freq;
> + return 0;
> +}
> +
> +/**
> + * update_devfreq() - Notify that the device OPP or frequency requirement
> + * has been changed. This function is exported for governors.
> + * @devfreq: the devfreq instance.
> + *
> + * Note: lock devfreq->lock before calling update_devfreq
> + */
> +int update_devfreq(struct devfreq *devfreq)
> +{
> + int err = 0;
> +
> + if (!mutex_is_locked(&devfreq->lock)) {
> + WARN(true, "devfreq->lock must be locked by the caller.\n");
> + return -EINVAL;
> + }
> +
> + /* Reevaluate the proper frequency */
> + err = devfreq_do(devfreq);
> + return err;
> +}
> +
> +/**
> + * devfreq_update() - Notify that the device OPP has been changed.
> + * @dev: the device whose OPP has been changed.
> + *
> + * Called by OPP notifier.
> + */
> +static int devfreq_update(struct notifier_block *nb, unsigned long type,
> + void *devp)
> +{
> + struct devfreq *devfreq = container_of(nb, struct devfreq, nb);
> + int ret;
> +
> + mutex_lock(&devfreq->lock);
> + ret = update_devfreq(devfreq);
> + mutex_unlock(&devfreq->lock);
The whole devfreq_update/update_devfreq pairing is redundant.
update_devfreq's purpose is to make sure the lock is held before going
further, and the only caller of update_devfreq is devfreq_update which
always holds the lock.
This still doesn't stop a bad driver writer from just calling
devfreq_do with an extern. Perhaps the lock detection should be moved
into devfreq_do and update_devfreq should go away?
> +
> + return ret;
> +}
> +
> +/**
> + * devfreq_monitor() - Periodically run devfreq_do()
> + * @work: the work struct used to run devfreq_monitor periodically.
> + *
> + */
> +static void devfreq_monitor(struct work_struct *work)
> +{
> + static unsigned long last_polled_at;
> + struct devfreq *devfreq, *tmp;
> + int error;
> + unsigned long jiffies_passed;
> + unsigned long next_jiffies = ULONG_MAX, now = jiffies;
> +
> + /* Initially last_polled_at = 0, polling every device at bootup */
> + jiffies_passed = now - last_polled_at;
> + last_polled_at = now;
> + if (jiffies_passed == 0)
> + jiffies_passed = 1;
> +
> + mutex_lock(&devfreq_list_lock);
Should not lock the list here. If we lock the list for all major
operations, it nullifies the performance benefit of having a mutex in
struct devfreq.
> +
> + list_for_each_entry_safe(devfreq, tmp, &devfreq_list, node) {
> + mutex_lock(&devfreq->lock);
> +
> + if (devfreq->next_polling == 0) {
> + mutex_unlock(&devfreq->lock);
> + continue;
> + }
> +
> + /*
> + * Reduce more next_polling if devfreq_wq took an extra
> + * delay. (i.e., CPU has been idled.)
> + */
> + if (devfreq->next_polling <= jiffies_passed) {
> + error = devfreq_do(devfreq);
> +
> + /* Remove a devfreq with an error. */
> + if (error && error != -EAGAIN) {
> + dev_err(devfreq->dev, "Due to devfreq_do error(%d), devfreq(%s) is removed from the device\n",
> + error, devfreq->governor->name);
> +
> + list_del(&devfreq->node);
> + mutex_unlock(&devfreq->lock);
> + kfree(devfreq);
> + continue;
Should this error handling also unregister the OPP notifier? This
code duplicates portions of devfreq_remove_device. I propose instead
here we do the following:
mutex_unlock(&devfreq->lock);
_devfreq_remove_lock(devfreq);
/* this locks the list first,
* then locks devfreq->lock,
* then does the house cleaning
*/
continue;
> + }
> + devfreq->next_polling = devfreq->polling_jiffies;
> +
> + /* No more polling required (polling_ms changed) */
> + if (devfreq->next_polling == 0) {
> + mutex_unlock(&devfreq->lock);
> + continue;
> + }
> + } else {
> + devfreq->next_polling -= jiffies_passed;
> + }
> +
> + next_jiffies = (next_jiffies > devfreq->next_polling) ?
> + devfreq->next_polling : next_jiffies;
> +
> + mutex_unlock(&devfreq->lock);
> + }
> +
> + if (next_jiffies > 0 && next_jiffies < ULONG_MAX) {
> + polling = true;
> + queue_delayed_work(devfreq_wq, &devfreq_work, next_jiffies);
> + } else {
> + polling = false;
> + }
> +
> + mutex_unlock(&devfreq_list_lock);
Again, list should not be locked over this whole function. It blocks
other unrelated devfreq devices from scaling.
> +}
[snip]
> +/**
> + * devfreq_remove_device() - Remove devfreq feature from a device.
> + * @device: the device to remove devfreq feature.
> + */
> +int devfreq_remove_device(struct device *dev)
Why does this take a struct device*? Shouldn't it be a struct devfreq*?
If there is a case for removing a devfreq device with only struct
device* as input, how about:
int devfreq_remove_device(struct device *dev)
{
struct devfreq *devfreq;
mutex_lock(&devfreq_list_lock);
devfreq = find_device_devfreq(dev);
mutex_unlock(&devfreq_list_lock);
return _devfreq_remove_device(struct devfreq *df);
/* _devfreq_remove_device does the real work and can also be
called from devfreq_monitor */
}
Regards,
Mike
> +{
> + struct devfreq *devfreq;
> + struct srcu_notifier_head *nh;
> + int err = 0;
> +
> + if (!dev)
> + return -EINVAL;
> +
> + mutex_lock(&devfreq_list_lock);
> + devfreq = find_device_devfreq(dev);
> + if (IS_ERR(devfreq)) {
> + err = PTR_ERR(devfreq);
> + goto out;
> + }
> +
> + mutex_lock(&devfreq->lock);
> + nh = opp_get_notifier(dev);
> + if (IS_ERR(nh)) {
> + err = PTR_ERR(nh);
> + mutex_unlock(&devfreq->lock);
> + goto out;
> + }
> +
> + list_del(&devfreq->node);
> +
> + if (devfreq->governor->exit)
> + devfreq->governor->exit(devfreq);
> +
> + srcu_notifier_chain_unregister(nh, &devfreq->nb);
> + mutex_unlock(&devfreq->lock);
> + kfree(devfreq);
> +out:
> + mutex_unlock(&devfreq_list_lock);
> + return 0;
> +}
[snip]
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
