On Tue, Aug 30, 2011 at 3:58 AM, Turquette, Mike <mturquette@xxxxxx> wrote:
> On Wed, Aug 24, 2011 at 1:22 AM, MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx> wrote:
>> +What: /sys/devices/.../power/devfreq_userspace_set_freq
>
> How about just .../devfreq_set_freq? I think the userspace bit is
> implied and the name is very long.
>
Umm.. as this entry became userspace specific, I though I need it be
to distinguished from entries created by devfreq framework. However,
this one is created only while userspace is being used (device may
replace governors by calling remove and add); thus, there wouldn't be
any duplicated name issues. So, I think removing userspace from the
name should be fine. I will do so in the next revision.
>> +
>> +static ssize_t show_freq(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct devfreq *devfreq = get_devfreq(dev);
>> + struct userspace_data *data;
>> + int err = 0;
>> +
>> + if (IS_ERR(devfreq)) {
>> + err = PTR_ERR(devfreq);
>> + goto out;
>> + }
>> + data = devfreq->data;
>> +
>> + if (data->valid)
>> + err = sprintf(buf, "%lu\n", data->user_frequency);
>> + else
>> + err = sprintf(buf, "undefined\n");
>> +out:
>> + return err;
>> +}
>
> Shouldn't accesses to devfreq->data be protected by a mutex?
>
> Regards,
> Mike
No, it doesn't need mutex here. Although generally, a mutex will be
needed for simliar codes, in this specific case, devfreq->data does
not need a mutex protection.
There are two variables in data: "user_frequency" and "valid"
- valid is initially false and becomes true at store_freq() and never changes.
- store_freq() writes user_frequency and then writes valid as true.
(no one writes false on valid)
- user_frequency is read only when valid is true.
Thus, the case where mutex protection serializes with some dictinction is:
1. Init. (valid = false)
2. store_freq() writes user_frequency = X
3. show_freq()/devfreq_userspace_func() reads valid (false)
4. store_freq() writes valid = true
5. show_freq()/devfreq_userspace_func() returns without reading
devfreq_userspace_func()
6. store_freq() returns.
into
A.
1. Init (valid = false)
2. store_freq() starts and finishes
3. show_freq()/devfreq_userspace_func() starts and finishes
B.
1. Init (valid = false)
2. show_freq()/devfreq_userspace_func() starts and finishes
3. store_freq() starts and finishes
where B results in the same thing as non-mutex version does.
If there is an operation that makes valid false, we will need a mutex
(local to the governor) anyway.
Anyway, I agree with you on the point that governors might need a
locking mechanism in general; not just on the private data, but on the
devfreq access. I'll put more on this issue on the other thread.
Thank you.
Cheers,
MyungJoo
>
>> +
>> +static DEVICE_ATTR(devfreq_userspace_set_freq, 0644, show_freq, store_freq);
>> +static struct attribute *dev_entries[] = {
>> + &dev_attr_devfreq_userspace_set_freq.attr,
>> + NULL,
>> +};
>> +static struct attribute_group dev_attr_group = {
>> + .name = power_group_name,
>> + .attrs = dev_entries,
>> +};
>> +
>> +static int userspace_init(struct devfreq *devfreq)
>> +{
>> + int err = 0;
>> + struct userspace_data *data = kzalloc(sizeof(struct userspace_data),
>> + GFP_KERNEL);
>> +
>> + if (!data) {
>> + err = -ENOMEM;
>> + goto out;
>> + }
>> + data->valid = false;
>> + devfreq->data = data;
>> +
>> + sysfs_merge_group(&devfreq->dev->kobj, &dev_attr_group);
>> +out:
>> + return err;
>> +}
>> +
>> +static void userspace_exit(struct devfreq *devfreq)
>> +{
>> + sysfs_unmerge_group(&devfreq->dev->kobj, &dev_attr_group);
>> + kfree(devfreq->data);
>> + devfreq->data = NULL;
>> +}
>> +
>> +struct devfreq_governor devfreq_userspace = {
>> + .name = "userspace",
>> + .get_target_freq = devfreq_userspace_func,
>> + .init = userspace_init,
>> + .exit = userspace_exit,
>> +};
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index fdc6916..cbafcdf 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -13,6 +13,7 @@
>> #ifndef __LINUX_DEVFREQ_H__
>> #define __LINUX_DEVFREQ_H__
>>
>> +#include <linux/opp.h>
>> #include <linux/notifier.h>
>>
>> #define DEVFREQ_NAME_LEN 16
>> @@ -65,6 +66,8 @@ struct devfreq_governor {
>> * "devfreq_monitor" executions to reevaluate
>> * frequency/voltage of the device. Set by
>> * profile's polling_ms interval.
>> + * @user_set_freq User specified adequete frequency value (thru sysfs
>> + * interface). Governors may and may not use this value.
>> * @data Private data of the governor. The devfreq framework does not
>> * touch this.
>> *
>> @@ -82,6 +85,7 @@ struct devfreq {
>> unsigned long previous_freq;
>> unsigned int next_polling;
>>
>> + unsigned long user_set_freq; /* governors may ignore this. */
>> void *data; /* private data for governors */
>> };
>>
>> @@ -91,6 +95,37 @@ extern int devfreq_add_device(struct device *dev,
>> struct devfreq_governor *governor,
>> void *data);
>> extern int devfreq_remove_device(struct device *dev);
>> +
>> +#ifdef CONFIG_DEVFREQ_GOV_POWERSAVE
>> +extern struct devfreq_governor devfreq_powersave;
>> +#endif
>> +#ifdef CONFIG_DEVFREQ_GOV_PERFORMANCE
>> +extern struct devfreq_governor devfreq_performance;
>> +#endif
>> +#ifdef CONFIG_DEVFREQ_GOV_USERSPACE
>> +extern struct devfreq_governor devfreq_userspace;
>> +#endif
>> +#ifdef CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND
>> +extern struct devfreq_governor devfreq_simple_ondemand;
>> +/**
>> + * struct devfreq_simple_ondemand_data - void *data fed to struct devfreq
>> + * and devfreq_add_device
>> + * @ upthreshold If the load is over this value, the frequency jumps.
>> + * Specify 0 to use the default. Valid value = 0 to 100.
>> + * @ downdifferential If the load is under upthreshold - downdifferential,
>> + * the governor may consider slowing the frequency down.
>> + * Specify 0 to use the default. Valid value = 0 to 100.
>> + * downdifferential < upthreshold must hold.
>> + *
>> + * If the fed devfreq_simple_ondemand_data pointer is NULL to the governor,
>> + * the governor uses the default values.
>> + */
>> +struct devfreq_simple_ondemand_data {
>> + unsigned int upthreshold;
>> + unsigned int downdifferential;
>> +};
>> +#endif
>> +
>> #else /* !CONFIG_PM_DEVFREQ */
>> static int devfreq_add_device(struct device *dev,
>> struct devfreq_dev_profile *profile,
>> @@ -104,6 +139,12 @@ static int devfreq_remove_device(struct device *dev)
>> {
>> return 0;
>> }
>> +
>> +#define devfreq_powersave NULL
>> +#define devfreq_performance NULL
>> +#define devfreq_userspace NULL
>> +#define devfreq_simple_ondemand NULL
>> +
>> #endif /* CONFIG_PM_DEVFREQ */
>>
>> #endif /* __LINUX_DEVFREQ_H__ */
>> --
>> 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
