Re: [PATCH v8 5/5] PM / DEVFREQ: add basic governors

["Turquette, Mike" <mturquette@ti.com>]

On Mon, Aug 29, 2011 at 9:19 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> On Tue, Aug 30, 2011 at 3:58 AM, Turquette, Mike <mturquette@ti.com> wrote:
>> On Wed, Aug 24, 2011 at 1:22 AM, MyungJoo Ham <myungjoo.ham@samsung.com> 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@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm