Re: [PATCH v8 3/5] PM / DEVFREQ: add common sysfs interfaces

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

On Mon, Aug 29, 2011 at 9:28 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> On Tue, Aug 30, 2011 at 4:17 AM, Turquette, Mike <mturquette@ti.com> 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.

Sounds good.  Also, please add a comment somewhere in devfreq.h
(probably above definition for struct devfreq) that the lock must also
be held by governors when accessing devfreq->data.

Regards,
Mike

> 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@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm