On Mon, Aug 29, 2011 at 9:28 PM, MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx> wrote:
> 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.
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@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
