On Mon, Aug 29, 2011 at 9:19 PM, MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx> wrote:
> 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.
It is your choice. You have a good point that it is specific to the
"userspace" governor. I hadn't thought of that at the time, so I
don't care either way.
Regards,
Mike
>>> +
>>> +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
