Re: [PATCH v3 3/3] PM / DEVFREQ: add sysfs interface (including user tickling)

[MyungJoo Ham]

Hello,

2011/7/3 Rafael J. Wysocki <rjw@xxxxxxx>:
> On Friday, May 27, 2011, MyungJoo Ham wrote:
>> 1. System-wide sysfs interface
>> - tickle_all ÂR: number of tickle_all execution
>> Â Â Â Â Â Â Â W: tickle all devfreq devices
>> - min_interval    ÂR: devfreq monitoring base interval in ms
>> - monitoring ÂR: shows whether devfreq monitoring is active or
>> Â not.
>>
>> 2. Device specific sysfs interface
>> - tickle   ÂR: number of tickle execution for the device
>> Â Â Â Â Â Â Â W: tickle the device
>>
>> Signed-off-by: MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
>> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
>>
>> --
>> Changed from v2
>> - add ABI entries for devfreq sysfs interface
>> ---
>> ÂDocumentation/ABI/testing/sysfs-devices-devfreq | Â 21 ++++
>> ÂDocumentation/ABI/testing/sysfs-power      |  43 ++++++++
>> Âdrivers/base/power/devfreq.c          Â| Â133 ++++++++++++++++++++++-
>> Âinclude/linux/devfreq.h             |  Â3 +
>> Â4 files changed, 199 insertions(+), 1 deletions(-)
>> Âcreate mode 100644 Documentation/ABI/testing/sysfs-devices-devfreq
>>
>> diff --git a/Documentation/ABI/testing/sysfs-devices-devfreq b/Documentation/ABI/testing/sysfs-devices-devfreq
>> new file mode 100644
>> index 0000000..7f35a64
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-devices-devfreq
>> @@ -0,0 +1,21 @@
>> +What: Â Â Â Â Â Â Â Â/sys/devices/.../devfreq/
>> +Date: Â Â Â Â Â Â Â ÂMay 2011
>> +Contact: Â Â MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
>> +Description:
>> + Â Â Â Â Â Â The /sys/device/.../devfreq directory will contain files
>> + Â Â Â Â Â Â that provide interfaces to DEVFREQ for a specific device.
>> +
>> +What: Â Â Â Â Â Â Â Â/sys/devices/.../devfreq/tickle
>> +Date: Â Â Â Â Â Â Â ÂMay 2011
>> +Contact: Â Â MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
>> +Description:
>> + Â Â Â Â Â Â The /sys/devices/.../devfreq/tickle file allows user space
>> + Â Â Â Â Â Â to force the corresponding device to operate at its maximum
>> + Â Â Â Â Â Â operable frequency instaneously and temporarily. After a
>> + Â Â Â Â Â Â designated duration has passed, the operating frequency returns
>> + Â Â Â Â Â Â to normal. When a user reads the tickle entry, it returns
>> + Â Â Â Â Â Â the number of tickle executions for the device. When a user
>> + Â Â Â Â Â Â writes to the tickle entry with the tickle duration in ms,
>> + Â Â Â Â Â Â the effect of device tickling is held for the designated
>> + Â Â Â Â Â Â duration. Note that the duration is rounded-up by
>> + Â Â Â Â Â Â the value DEVFREQ_INTERVAL defined in devfreq.c
>> diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
>> index b464d12..4d8434b 100644
>> --- a/Documentation/ABI/testing/sysfs-power
>> +++ b/Documentation/ABI/testing/sysfs-power
>> @@ -172,3 +172,46 @@ Description:
>>
>> Â Â Â Â Â Â Â Reading from this file will display the current value, which is
>> Â Â Â Â Â Â Â set to 1 MB by default.
>> +
>> +What: Â Â Â Â Â Â Â Â/sys/power/devfreq/
>> +Date: Â Â Â Â Â Â Â ÂMay 2011
>> +Contact: Â Â MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
>> +Description:
>> + Â Â Â Â Â Â The /sys/power/devfreq directory will contain files that will
>> + Â Â Â Â Â Â provide a unified interface to the DEVFREQ, a generic DVFS
>> + Â Â Â Â Â Â (dynamic voltage and frequency scaling) framework.
>> +
>> +What: Â Â Â Â Â Â Â Â/sys/power/devfreq/tickle_all
>> +Date: Â Â Â Â Â Â Â ÂMay 2011
>> +Contact: Â Â MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
>> +Description:
>> + Â Â Â Â Â Â The /sys/power/devfreq/tickle_all file allows user space to
>> + Â Â Â Â Â Â force every device with DEVFREQ to operate at the maximum
>> + Â Â Â Â Â Â frequency of the device instaneously and temporarily. After
>> + Â Â Â Â Â Â a designated delay has passed, the operating frequency returns
>> + Â Â Â Â Â Â to normal. If a user reads the tickle_all entry, it returns
>> + Â Â Â Â Â Â the number of tickle_all executions. When writing to the
>> + Â Â Â Â Â Â tickle_all entry, the user should supply with the duration of
>> + Â Â Â Â Â Â tickle in ms (the "designated delay" mentioned before). Then,
>> + Â Â Â Â Â Â the effect of tickle_all will hold for the denoted duration.
>> + Â Â Â Â Â Â Note that the duration is rounded by the monitoring period
>> + Â Â Â Â Â Â defined by DEVFREQ_INTERVAL in /drivers/base/power/devfreq.c.
>> +
>> +What: Â Â Â Â Â Â Â Â/sys/power/devfreq/min_interval
>> +Date: Â Â Â Â Â Â Â ÂMay 2011
>> +Contact: Â Â MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
>> +Description:
>> + Â Â Â Â Â Â The /sys/power/devfreq/min_interval file shows the monitoring
>> + Â Â Â Â Â Â period defined by DEVFREQ_INTERVAL in
>> + Â Â Â Â Â Â /drivers/base/power/devfreq.c. The duration of device tickling
>> + Â Â Â Â Â Â is rounded-up by DEVFREQ_INTERVAL.
>> +
>> +What: Â Â Â Â Â Â Â Â/sys/power/devfreq/monitoring
>> +Date: Â Â Â Â Â Â Â ÂMay 2011
>> +Contact: Â Â MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
>> +Description:
>> + Â Â Â Â Â Â The /sys/power/devfreq/monitoring file shows whether DEVFREQ
>> + Â Â Â Â Â Â is periodically monitoring. Periodic monitoring is activated
>> + Â Â Â Â Â Â if there is a device that wants periodic monitoring for DVFS or
>> + Â Â Â Â Â Â there is a device that is tickled (and the tickling duration is
>> + Â Â Â Â Â Â not yet expired).
>> diff --git a/drivers/base/power/devfreq.c b/drivers/base/power/devfreq.c
>> index 7648a94..709c138 100644
>> --- a/drivers/base/power/devfreq.c
>> +++ b/drivers/base/power/devfreq.c
>> @@ -40,6 +40,9 @@ static LIST_HEAD(devfreq_list);
>> Â/* Exclusive access to devfreq_list and its elements */
>> Âstatic DEFINE_MUTEX(devfreq_list_lock);
>>
>> +static struct kobject *devfreq_kobj;
>> +static struct attribute_group dev_attr_group;
>> +
>> Â/**
>> Â * find_device_devfreq() - find devfreq struct using device pointer
>> Â * @dev: Â Â device pointer used to lookup device DEVFREQ.
>> @@ -237,6 +240,8 @@ int devfreq_add_device(struct device *dev, struct devfreq_dev_profile *profile,
>> Â Â Â Â Â Â Â queue_delayed_work(devfreq_wq, &devfreq_work,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âmsecs_to_jiffies(DEVFREQ_INTERVAL));
>> Â Â Â }
>> +
>> + Â Â sysfs_update_group(&dev->kobj, &dev_attr_group);
>
> This appears to modify the attributes of the device, but anything like it is
> not mentioned in the documentation. ÂWhat's up?

It is mentioned "Documentation/ABI/testing/sysfs-devices-devfreq".

>
>> Âout:
>> Â Â Â mutex_unlock(&devfreq_list_lock);
>>
>> @@ -263,6 +268,8 @@ int devfreq_remove_device(struct device *dev)
>> Â Â Â Â Â Â Â return -EINVAL;
>> Â Â Â }
>>
>> + Â Â sysfs_remove_group(&dev->kobj, &dev_attr_group);
>> +
>> Â Â Â list_del(&devfreq->node);
>>
>> Â Â Â kfree(devfreq);
>> @@ -344,7 +351,7 @@ static int _devfreq_tickle_device(struct devfreq *df, unsigned long delay)
>> Â Â Â if (devfreq_wq && !polling) {
>> Â Â Â Â Â Â Â polling = true;
>> Â Â Â Â Â Â Â queue_delayed_work(devfreq_wq, &devfreq_work,
>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â msecs_to_jiffies(DEVFREQ_INTERVAL));
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âmsecs_to_jiffies(DEVFREQ_INTERVAL));
>
> This change doesn't seem to belong to this patch.

Oops. I'll rearrange the patch series.

>
>> Â Â Â }
>>
>> Â Â Â return err;
>> @@ -382,6 +389,116 @@ int devfreq_tickle_device(struct device *dev, unsigned long duration_ms)
>> Â Â Â return err;
>> Â}
>>
>> +static int num_tickle_all;
>> +static ssize_t tickle_all(struct device *dev, struct device_attribute *attr,
>> + Â Â Â Â Â Â Â Â Â Â Â const char *buf, size_t count)
>> +{
>> + Â Â int duration = 0;
>> + Â Â struct devfreq *tmp;
>> + Â Â unsigned long delay;
>> +
>> + Â Â sscanf(buf, "%d", &duration);
>> + Â Â if (duration < DEVFREQ_INTERVAL)
>> + Â Â Â Â Â Â duration = DEVFREQ_INTERVAL;
>> +
>> + Â Â if (unlikely(IS_ERR_OR_NULL(dev))) {
>> + Â Â Â Â Â Â pr_err("%s: Invalid parameters\n", __func__);
>
> Please say "null device" instead of in addition to the above.

Ok. I'll let it say "Null or invalid device" as it's IS_ERR_OR_NULL.

>
>> + Â Â Â Â Â Â return -EINVAL;
>> + Â Â }
>> +
>> + Â Â delay = DIV_ROUND_UP(duration, DEVFREQ_INTERVAL);
>> +
>> + Â Â mutex_lock(&devfreq_list_lock);
>> + Â Â list_for_each_entry(tmp, &devfreq_list, node) {
>> + Â Â Â Â Â Â _devfreq_tickle_device(tmp, delay);
>> + Â Â }
>> + Â Â mutex_unlock(&devfreq_list_lock);
>> +
>> + Â Â num_tickle_all++;
>> + Â Â return count;
>> +}
>> +
>> +static ssize_t show_num_tickle_all(struct device *dev,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstruct device_attribute *attr, char *buf)
>> +{
>> + Â Â return sprintf(buf, "%d\n", num_tickle_all);
>> +}
>> +
>> +static ssize_t show_min_interval(struct device *dev,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstruct device_attribute *attr, char *buf)
>> +{
>> + Â Â return sprintf(buf, "%d\n", DEVFREQ_INTERVAL);
>> +}
>> +
>> +static ssize_t show_monitoring(struct device *dev,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Âstruct device_attribute *attr, char *buf)
>> +{
>> + Â Â return sprintf(buf, "%d\n", monitoring ? 1 : 0);
>> +}
>> +
>> +static DEVICE_ATTR(tickle_all, 0644, show_num_tickle_all, tickle_all);
>> +static DEVICE_ATTR(min_interval, 0444, show_min_interval, NULL);
>> +static DEVICE_ATTR(monitoring, 0444, show_monitoring, NULL);
>> +static struct attribute *devfreq_entries[] = {
>> + Â Â &dev_attr_tickle_all.attr,
>> + Â Â &dev_attr_min_interval.attr,
>> + Â Â &dev_attr_monitoring.attr,
>> + Â Â NULL,
>> +};
>> +static struct attribute_group devfreq_attr_group = {
>> +   .name  = NULL,
>> + Â Â .attrs Â= devfreq_entries,
>> +};
>> +
>> +static ssize_t tickle(struct device *dev, struct device_attribute *attr,
>> + Â Â Â Â Â Â Â Â Â const char *buf, size_t count)
>> +{
>> + Â Â int duration;
>> + Â Â struct devfreq *df;
>> + Â Â unsigned long delay;
>> +
>> + Â Â sscanf(buf, "%d", &duration);
>> + Â Â if (duration < DEVFREQ_INTERVAL)
>> + Â Â Â Â Â Â duration = DEVFREQ_INTERVAL;
>> +
>> + Â Â if (unlikely(IS_ERR_OR_NULL(dev))) {
>> + Â Â Â Â Â Â pr_err("%s: Invalid parameters\n", __func__);
>
> Like above.

Yup.

>
>> + Â Â Â Â Â Â return -EINVAL;
>> + Â Â }
>> +
>> + Â Â delay = DIV_ROUND_UP(duration, DEVFREQ_INTERVAL);
>> +
>> + Â Â mutex_lock(&devfreq_list_lock);
>> + Â Â df = find_device_devfreq(dev);
>> + Â Â _devfreq_tickle_device(df, delay);
>> + Â Â mutex_unlock(&devfreq_list_lock);
>> +
>> + Â Â return count;
>> +}
>> +
>> +static ssize_t show_num_tickle(struct device *dev,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Âstruct device_attribute *attr, char *buf)
>> +{
>> + Â Â struct devfreq *df;
>> +
>> + Â Â df = find_device_devfreq(dev);
>> +
>> + Â Â if (!IS_ERR(df))
>> + Â Â Â Â Â Â return sprintf(buf, "%d\n", df->num_tickle);
>> +
>> + Â Â return PTR_ERR(df);
>> +}
>> +
>> +static DEVICE_ATTR(tickle, 0644, show_num_tickle, tickle);
>> +static struct attribute *dev_entries[] = {
>> + Â Â &dev_attr_tickle.attr,
>> + Â Â NULL,
>> +};
>> +static struct attribute_group dev_attr_group = {
>> +   .name  = "devfreq",
>> + Â Â .attrs Â= dev_entries,
>> +};
>> +
>> Âstatic int __init devfreq_init(void)
>> Â{
>> Â Â Â mutex_lock(&devfreq_list_lock);
>> @@ -389,6 +506,20 @@ static int __init devfreq_init(void)
>> Â Â Â polling = false;
>> Â Â Â devfreq_wq = create_freezable_workqueue("devfreq_wq");
>> Â Â Â INIT_DELAYED_WORK_DEFERRABLE(&devfreq_work, devfreq_monitor);
>> +
>> +#ifdef CONFIG_PM
>> + Â Â /* Create sysfs */
>> + Â Â devfreq_kobj = kobject_create_and_add("devfreq", power_kobj);
>
> Hmm, so power_kobj is global? ÂIt shouldn't be.

Yes, it is global and it's declared at include/linux/kobject.h.

>
> Generally, whatever adds attributes to /sys/power should be located in
> kernel/power/ .

I've put it at /sys/power because these attributes are global to every
device with devfreq.

Anyway, if you think that's a weird location for it, could you please
give me some hints on where would be proper to locate system-wide
devfreq sysfs attributes?
Or, do you think kernel/power/devfreq.c would be a proper location for devfreq?

Or, what about allowing every /sys/devices/.../devfreq/global/* to be
the same "global" attributes of devfreq?

>
>> + Â Â if (!devfreq_kobj) {
>> + Â Â Â Â Â Â pr_err("Unable to create DEVFREQ kobject.\n");
>> + Â Â Â Â Â Â goto out;
>> + Â Â }
>> + Â Â if (sysfs_create_group(devfreq_kobj, &devfreq_attr_group)) {
>> + Â Â Â Â Â Â pr_err("Unable to create DEVFREQ sysfs entries.\n");
>> + Â Â Â Â Â Â goto out;
>> + Â Â }
>> +#endif
>> +out:
>> Â Â Â mutex_unlock(&devfreq_list_lock);
>>
>> Â Â Â devfreq_monitor(&devfreq_work.work);
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index 1ec9a40..69334e7 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -59,6 +59,7 @@ struct devfreq_governor {
>> Â * Â Â Â Â Â at each executino of devfreq_monitor, tickle is decremented.
>> Â * Â Â Â Â Â User may tickle a device-devfreq in order to set maximum
>> Â * Â Â Â Â Â frequency instaneously with some guaranteed duration.
>> + * @num_tickle    number of tickle calls.
>> Â *
>> Â * This structure stores the DEVFREQ information for a give device.
>> Â */
>> @@ -72,6 +73,8 @@ struct devfreq {
>> Â Â Â unsigned long previous_freq;
>> Â Â Â unsigned int next_polling;
>> Â Â Â unsigned int tickle;
>> +
>> + Â Â unsigned int num_tickle;
>> Â};
>>
>> Â#if defined(CONFIG_PM_DEVFREQ)
>
> It looks like the above two changes should be moved to a separate patch.

These two changes are for the sysfs interface as counting number of
tickle is added with sysfs interface.

>
> Thanks,
> Rafael
>


Thank you,
- MyungJoo
-- 
MyungJoo Ham (íëì), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/