On Tue, Aug 30, 2011 at 4:17 AM, Turquette, Mike <mturquette@xxxxxx> wrote:
>
> Sorry I'll have to rescind my ACK. I just reviewed patch 5/5 and the
> show_freq and restore_freq functions for the userspace governor do not
> protect the private data accesses with a mutex. Looking a bit further
> I see that they do call get_devfreq; I think the semantics for this
> are wrong.
>
> get_devfreq only protects the list as long as it takes to do a lookup.
> However neither struct devfreq or struct userspace_data have a mutex
> associated with them, so concurrent operations are not protected.
>
> One heavy-handed way of solving this is to not have get_devfreq
> release the mutex, and then have those functions call a new
> put_devfreq which does release the mutex. However that is really bad
> locking.
>
> What do you think about putting a mutex in struct devfreq? The rule
> for governors can be that access to the governor-specific private data
> requires taking that devfreq's mutex.
A lock in private data at DEVFREQ framework won't be needed as it is a
governor-specific issue; however, it appears that we need a locking
mechanism for accessing struct devfreq in general for governors.
Although we do not have any governor that writes something or reads
multiple times on struct devfreq (except for the private data) out of
get_target_freq ops, which does not need an additional lock, we may
have one soon.
Then, I will need to update the DEVFREQ core as well.
There will be two locks in devfreq.c: the global devfreq_list_lock and
per-devfreq devfreq_lock in struct devfreq.
And the role of devfreq_lock will be protecting the access to struct
devfreq (some functions in devfreq.c will use this lock and some usage
of devfreq_list_lock will be removed).
This will result in more general sysfs interface (or functions other
than the standard ops) support in governors.
Thank you so much!
Cheers,
MyungJoo.
>
> Regards,
> Mike
>
>> +
>> +/**
>> * devfreq_do() - Check the usage profile of a given device and configure
>> * frequency and voltage accordingly
>> * @devfreq: devfreq info of the given device
>> @@ -149,6 +167,8 @@ static void devfreq_monitor(struct work_struct *work)
>> dev_err(devfreq->dev, "Due to devfreq_do error(%d), devfreq(%s) is removed from the device\n",
>> error, devfreq->governor->name);
>>
>> + sysfs_unmerge_group(&devfreq->dev->kobj,
>> + &dev_attr_group);
>> list_del(&devfreq->node);
>> kfree(devfreq);
>>
>> @@ -239,6 +259,8 @@ int devfreq_add_device(struct device *dev, struct devfreq_dev_profile *profile,
>> queue_delayed_work(devfreq_wq, &devfreq_work,
>> devfreq->next_polling);
>> }
>> +
>> + sysfs_merge_group(&dev->kobj, &dev_attr_group);
>> out:
>> mutex_unlock(&devfreq_list_lock);
>>
>> @@ -271,6 +293,8 @@ int devfreq_remove_device(struct device *dev)
>> goto out;
>> }
>>
>> + sysfs_unmerge_group(&dev->kobj, &dev_attr_group);
>> +
>> list_del(&devfreq->node);
>> srcu_notifier_chain_unregister(nh, &devfreq->nb);
>> kfree(devfreq);
>> @@ -279,6 +303,132 @@ out:
>> return 0;
>> }
>>
>> +static ssize_t show_governor(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct devfreq *df = get_devfreq(dev);
>> +
>> + if (IS_ERR(df))
>> + return PTR_ERR(df);
>> + if (!df->governor)
>> + return -EINVAL;
>> +
>> + return sprintf(buf, "%s\n", df->governor->name);
>> +}
>> +
>> +static ssize_t show_freq(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct devfreq *df = get_devfreq(dev);
>> +
>> + if (IS_ERR(df))
>> + return PTR_ERR(df);
>> +
>> + return sprintf(buf, "%lu\n", df->previous_freq);
>> +}
>> +
>> +static ssize_t show_max_freq(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct devfreq *df = get_devfreq(dev);
>> + unsigned long freq = ULONG_MAX;
>> + struct opp *opp;
>> +
>> + if (IS_ERR(df))
>> + return PTR_ERR(df);
>> + if (!df->dev)
>> + return -EINVAL;
>> +
>> + opp = opp_find_freq_floor(df->dev, &freq);
>> + if (IS_ERR(opp))
>> + return PTR_ERR(opp);
>> +
>> + return sprintf(buf, "%lu\n", freq);
>> +}
>> +
>> +static ssize_t show_min_freq(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct devfreq *df = get_devfreq(dev);
>> + unsigned long freq = 0;
>> + struct opp *opp;
>> +
>> + if (IS_ERR(df))
>> + return PTR_ERR(df);
>> + if (!df->dev)
>> + return -EINVAL;
>> +
>> + opp = opp_find_freq_ceil(df->dev, &freq);
>> + if (IS_ERR(opp))
>> + return PTR_ERR(opp);
>> +
>> + return sprintf(buf, "%lu\n", freq);
>> +}
>> +
>> +static ssize_t show_polling_interval(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct devfreq *df = get_devfreq(dev);
>> +
>> + if (IS_ERR(df))
>> + return PTR_ERR(df);
>> + if (!df->profile)
>> + return -EINVAL;
>> +
>> + return sprintf(buf, "%d\n", df->profile->polling_ms);
>> +}
>> +
>> +static ssize_t store_polling_interval(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct devfreq *df = get_devfreq(dev);
>> + unsigned int value;
>> + int ret;
>> +
>> + if (IS_ERR(df))
>> + return PTR_ERR(df);
>> + if (!df->profile)
>> + return -EINVAL;
>> +
>> + ret = sscanf(buf, "%u", &value);
>> + if (ret != 1)
>> + return -EINVAL;
>> +
>> + df->profile->polling_ms = value;
>> + df->next_polling = df->polling_jiffies
>> + = msecs_to_jiffies(value);
>> +
>> + mutex_lock(&devfreq_list_lock);
>> + if (df->next_polling > 0 && !polling) {
>> + polling = true;
>> + queue_delayed_work(devfreq_wq, &devfreq_work,
>> + df->next_polling);
>> + }
>> + mutex_unlock(&devfreq_list_lock);
>> +
>> + return count;
>> +}
>> +
>> +static DEVICE_ATTR(devfreq_governor, 0444, show_governor, NULL);
>> +static DEVICE_ATTR(devfreq_cur_freq, 0444, show_freq, NULL);
>> +static DEVICE_ATTR(devfreq_max_freq, 0444, show_max_freq, NULL);
>> +static DEVICE_ATTR(devfreq_min_freq, 0444, show_min_freq, NULL);
>> +static DEVICE_ATTR(devfreq_polling_interval, 0644, show_polling_interval,
>> + store_polling_interval);
>> +static struct attribute *dev_entries[] = {
>> + &dev_attr_devfreq_governor.attr,
>> + &dev_attr_devfreq_cur_freq.attr,
>> + &dev_attr_devfreq_max_freq.attr,
>> + &dev_attr_devfreq_min_freq.attr,
>> + &dev_attr_devfreq_polling_interval.attr,
>> + NULL,
>> +};
>> +static struct attribute_group dev_attr_group = {
>> + .name = power_group_name,
>> + .attrs = dev_entries,
>> +};
>> +
>> /**
>> * devfreq_init() - Initialize data structure for devfreq framework and
>> * start polling registered devfreq devices.
>> --
>> 1.7.4.1
>>
>>
>
--
MyungJoo Ham (함명주), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
