| Message ID | 1314174131-14194-4-git-send-email-myungjoo.ham@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, Aug 24, 2011 at 1:22 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote: > Device specific sysfs interface /sys/devices/.../power/devfreq_* > - governor R: name of governor > - cur_freq R: current frequency > - max_freq R: maximum operable frequency > - min_freq R: minimum operable frequency > - polling_interval R: polling interval in ms given with devfreq profile > W: update polling interval. > > Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> Looks good to me. Reviewed-by: Mike Turquette <mturquette@ti.com> Regards, Mike > > -- > Changed from v7 > - removed set_freq from the common devfreq interface > - added get_devfreq, a mutex-protected wrapper for find_device_devfreq > (for sysfs interfaces and later with governor-support) > - corrected ABI documentation. > > Changed from v6 > - poling_interval is writable. > > Changed from v5 > - updated devferq_update usage. > > Changed from v4 > - removed system-wide sysfs interface > - removed tickling sysfs interface > - added set_freq for userspace governor (and any other governors that > require user input) > > Changed from v3 > - corrected sysfs API usage > - corrected error messages > - moved sysfs entry location > - added sysfs entries > > Changed from v2 > - add ABI entries for devfreq sysfs interface > --- > Documentation/ABI/testing/sysfs-devices-power | 37 ++++++ > drivers/devfreq/devfreq.c | 150 +++++++++++++++++++++++++ > 2 files changed, 187 insertions(+), 0 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-devices-power b/Documentation/ABI/testing/sysfs-devices-power > index 8ffbc25..57f4591 100644 > --- a/Documentation/ABI/testing/sysfs-devices-power > +++ b/Documentation/ABI/testing/sysfs-devices-power > @@ -165,3 +165,40 @@ Description: > > Not all drivers support this attribute. If it isn't supported, > attempts to read or write it will yield I/O errors. > + > +What: /sys/devices/.../power/devfreq_governor > +Date: July 2011 > +Contact: MyungJoo Ham <myungjoo.ham@samsung.com> > +Description: > + The /sys/devices/.../power/devfreq_governor shows the name > + of the governor used by the corresponding device. > + > +What: /sys/devices/.../power/devfreq_cur_freq > +Date: July 2011 > +Contact: MyungJoo Ham <myungjoo.ham@samsung.com> > +Description: > + The /sys/devices/.../power/devfreq_cur_freq shows the current > + frequency of the corresponding device. > + > +What: /sys/devices/.../power/devfreq_max_freq > +Date: July 2011 > +Contact: MyungJoo Ham <myungjoo.ham@samsung.com> > +Description: > + The /sys/devices/.../power/devfreq_max_freq shows the > + maximum operable frequency of the corresponding device. > + > +What: /sys/devices/.../power/devfreq_min_freq > +Date: July 2011 > +Contact: MyungJoo Ham <myungjoo.ham@samsung.com> > +Description: > + The /sys/devices/.../power/devfreq_min_freq shows the > + minimum operable frequency of the corresponding device. > + > +What: /sys/devices/.../power/devfreq_polling_interval > +Date: July 2011 > +Contact: MyungJoo Ham <myungjoo.ham@samsung.com> > +Description: > + The /sys/devices/.../power/devfreq_polling_interval sets and > + shows the requested polling interval of the corresponding > + device. The values are represented in ms. If the value is less > + than 1 jiffy, it is considered to be 0, which means no polling. > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index df63bdc..5bbece6 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -37,6 +37,8 @@ static struct delayed_work devfreq_work; > static LIST_HEAD(devfreq_list); > static DEFINE_MUTEX(devfreq_list_lock); > > +static struct attribute_group dev_attr_group; > + > /** > * find_device_devfreq() - find devfreq struct using device pointer > * @dev: device pointer used to lookup device devfreq. > @@ -62,6 +64,22 @@ static struct devfreq *find_device_devfreq(struct device *dev) > } > > /** > + * get_devfreq() - find devfreq struct. a wrapped find_device_devfreq() > + * with mutex protection. > + * @dev: device pointer used to lookup device devfreq. > + */ > +static struct devfreq *get_devfreq(struct device *dev) > +{ > + struct devfreq *ret; > + > + mutex_lock(&devfreq_list_lock); > + ret = find_device_devfreq(dev); > + mutex_unlock(&devfreq_list_lock); > + > + return ret; > +} > + > +/** > * 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 > >
On Wed, Aug 24, 2011 at 1:22 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote: > Device specific sysfs interface /sys/devices/.../power/devfreq_* > - governor R: name of governor > - cur_freq R: current frequency > - max_freq R: maximum operable frequency > - min_freq R: minimum operable frequency > - polling_interval R: polling interval in ms given with devfreq profile > W: update polling interval. > > Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > -- > Changed from v7 > - removed set_freq from the common devfreq interface > - added get_devfreq, a mutex-protected wrapper for find_device_devfreq > (for sysfs interfaces and later with governor-support) > - corrected ABI documentation. > > Changed from v6 > - poling_interval is writable. > > Changed from v5 > - updated devferq_update usage. > > Changed from v4 > - removed system-wide sysfs interface > - removed tickling sysfs interface > - added set_freq for userspace governor (and any other governors that > require user input) > > Changed from v3 > - corrected sysfs API usage > - corrected error messages > - moved sysfs entry location > - added sysfs entries > > Changed from v2 > - add ABI entries for devfreq sysfs interface > --- > Documentation/ABI/testing/sysfs-devices-power | 37 ++++++ > drivers/devfreq/devfreq.c | 150 +++++++++++++++++++++++++ > 2 files changed, 187 insertions(+), 0 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-devices-power b/Documentation/ABI/testing/sysfs-devices-power > index 8ffbc25..57f4591 100644 > --- a/Documentation/ABI/testing/sysfs-devices-power > +++ b/Documentation/ABI/testing/sysfs-devices-power > @@ -165,3 +165,40 @@ Description: > > Not all drivers support this attribute. If it isn't supported, > attempts to read or write it will yield I/O errors. > + > +What: /sys/devices/.../power/devfreq_governor > +Date: July 2011 > +Contact: MyungJoo Ham <myungjoo.ham@samsung.com> > +Description: > + The /sys/devices/.../power/devfreq_governor shows the name > + of the governor used by the corresponding device. > + > +What: /sys/devices/.../power/devfreq_cur_freq > +Date: July 2011 > +Contact: MyungJoo Ham <myungjoo.ham@samsung.com> > +Description: > + The /sys/devices/.../power/devfreq_cur_freq shows the current > + frequency of the corresponding device. > + > +What: /sys/devices/.../power/devfreq_max_freq > +Date: July 2011 > +Contact: MyungJoo Ham <myungjoo.ham@samsung.com> > +Description: > + The /sys/devices/.../power/devfreq_max_freq shows the > + maximum operable frequency of the corresponding device. > + > +What: /sys/devices/.../power/devfreq_min_freq > +Date: July 2011 > +Contact: MyungJoo Ham <myungjoo.ham@samsung.com> > +Description: > + The /sys/devices/.../power/devfreq_min_freq shows the > + minimum operable frequency of the corresponding device. > + > +What: /sys/devices/.../power/devfreq_polling_interval > +Date: July 2011 > +Contact: MyungJoo Ham <myungjoo.ham@samsung.com> > +Description: > + The /sys/devices/.../power/devfreq_polling_interval sets and > + shows the requested polling interval of the corresponding > + device. The values are represented in ms. If the value is less > + than 1 jiffy, it is considered to be 0, which means no polling. > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index df63bdc..5bbece6 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -37,6 +37,8 @@ static struct delayed_work devfreq_work; > static LIST_HEAD(devfreq_list); > static DEFINE_MUTEX(devfreq_list_lock); > > +static struct attribute_group dev_attr_group; > + > /** > * find_device_devfreq() - find devfreq struct using device pointer > * @dev: device pointer used to lookup device devfreq. > @@ -62,6 +64,22 @@ static struct devfreq *find_device_devfreq(struct device *dev) > } > > /** > + * get_devfreq() - find devfreq struct. a wrapped find_device_devfreq() > + * with mutex protection. > + * @dev: device pointer used to lookup device devfreq. > + */ > +static struct devfreq *get_devfreq(struct device *dev) > +{ > + struct devfreq *ret; > + > + mutex_lock(&devfreq_list_lock); > + ret = find_device_devfreq(dev); > + mutex_unlock(&devfreq_list_lock); > + > + return ret; > +} 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. 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 > >
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. 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 >> >> >
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 >
diff --git a/Documentation/ABI/testing/sysfs-devices-power b/Documentation/ABI/testing/sysfs-devices-power index 8ffbc25..57f4591 100644 --- a/Documentation/ABI/testing/sysfs-devices-power +++ b/Documentation/ABI/testing/sysfs-devices-power @@ -165,3 +165,40 @@ Description: Not all drivers support this attribute. If it isn't supported, attempts to read or write it will yield I/O errors. + +What: /sys/devices/.../power/devfreq_governor +Date: July 2011 +Contact: MyungJoo Ham <myungjoo.ham@samsung.com> +Description: + The /sys/devices/.../power/devfreq_governor shows the name + of the governor used by the corresponding device. + +What: /sys/devices/.../power/devfreq_cur_freq +Date: July 2011 +Contact: MyungJoo Ham <myungjoo.ham@samsung.com> +Description: + The /sys/devices/.../power/devfreq_cur_freq shows the current + frequency of the corresponding device. + +What: /sys/devices/.../power/devfreq_max_freq +Date: July 2011 +Contact: MyungJoo Ham <myungjoo.ham@samsung.com> +Description: + The /sys/devices/.../power/devfreq_max_freq shows the + maximum operable frequency of the corresponding device. + +What: /sys/devices/.../power/devfreq_min_freq +Date: July 2011 +Contact: MyungJoo Ham <myungjoo.ham@samsung.com> +Description: + The /sys/devices/.../power/devfreq_min_freq shows the + minimum operable frequency of the corresponding device. + +What: /sys/devices/.../power/devfreq_polling_interval +Date: July 2011 +Contact: MyungJoo Ham <myungjoo.ham@samsung.com> +Description: + The /sys/devices/.../power/devfreq_polling_interval sets and + shows the requested polling interval of the corresponding + device. The values are represented in ms. If the value is less + than 1 jiffy, it is considered to be 0, which means no polling. diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index df63bdc..5bbece6 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -37,6 +37,8 @@ static struct delayed_work devfreq_work; static LIST_HEAD(devfreq_list); static DEFINE_MUTEX(devfreq_list_lock); +static struct attribute_group dev_attr_group; + /** * find_device_devfreq() - find devfreq struct using device pointer * @dev: device pointer used to lookup device devfreq. @@ -62,6 +64,22 @@ static struct devfreq *find_device_devfreq(struct device *dev) } /** + * get_devfreq() - find devfreq struct. a wrapped find_device_devfreq() + * with mutex protection. + * @dev: device pointer used to lookup device devfreq. + */ +static struct devfreq *get_devfreq(struct device *dev) +{ + struct devfreq *ret; + + mutex_lock(&devfreq_list_lock); + ret = find_device_devfreq(dev); + mutex_unlock(&devfreq_list_lock); + + return ret; +} + +/** * 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.