Re: [RFC PATCHv2 1/7] devfreq: event: Add new devfreq_event class to provide basic data for devfreq governor

From: Chanwoo Choi
Date: Mon Dec 15 2014 - 21:54:44 EST

Next message: Brian Norris: "Re: Maintainer abuse"
Previous message: Alexei Starovoitov: "Re: [LKP] [ipmi] BUG: key ffff880fcfc51ed0 not in .data!"
In reply to: Krzysztof Kozlowski: "Re: [RFC PATCHv2 1/7] devfreq: event: Add new devfreq_event class to provide basic data for devfreq governor"
Next in thread: Chanwoo Choi: "[RFC PATCHv2 7/7] ARM: dts: exynos: Add PPMU dt node to Exynos3250-based Rinato board"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Krzysztof,

On 12/15/2014 07:30 PM, Krzysztof Kozlowski wrote:
> On piÄ, 2014-12-12 at 12:42 +0900, Chanwoo Choi wrote:
>> Hi Krzysztof,
>>
>> I replied again this mail because I'll use the mutex for set_event()/get_event()
>> according to your comment. But, of_parse_phandle() seems that this function
>> don't need the of_node_put() function.
>>
>>
>> On 12/11/2014 11:13 AM, Chanwoo Choi wrote:
>>> Hi Krzysztof,
>>>
>>> First of all, thanks for your review.
>>>
>>> On 12/10/2014 06:37 PM, Krzysztof Kozlowski wrote:
>>>> On wto, 2014-12-09 at 23:13 +0900, Chanwoo Choi wrote:
>>>>> This patch add new devfreq_event class for devfreq_event device which provide
>>>>> raw data (e.g., memory bus utilization/GPU utilization). This raw data from
>>>>> devfreq_event data would be used for the governor of devfreq subsystem.
>>>>> - devfreq_event device : Provide raw data for governor of existing devfreq device
>>>>> - devfreq device : Monitor device state and change frequency/voltage of device
>>>>> using the raw data from devfreq_event device
>>>>>
>>>>> The devfreq subsystem support generic DVFS(Dynamic Voltage/Frequency Scaling)
>>>>> for Non-CPU Devices. The devfreq device would dertermine current device state
>>>>> using various governor (e.g., ondemand, performance, powersave). After completed
>>>>> determination of system state, devfreq device would change the frequency/voltage
>>>>> of devfreq device according to the result of governor.
>>>>>
>>>>> But, devfreq governor must need basic data which indicates current device state.
>>>>> Existing devfreq subsystem only consider devfreq device which check current system
>>>>> state and determine proper system state using basic data. There is no subsystem
>>>>> for device providing basic data to devfreq device.
>>>>>
>>>>> The devfreq subsystem must need devfreq_event device(data-provider device) for
>>>>> existing devfreq device. So, this patch add new devfreq_event class for
>>>>> devfreq_event device which read various basic data(e.g, memory bus utilization,
>>>>> GPU utilization) and provide measured data to existing devfreq device through
>>>>> standard APIs of devfreq_event class.
>>>>>
>>>>> The following description explains the feature of two kind of devfreq class:
>>>>> - devfreq class (existing)
>>>>> : devfreq consumer device use raw data from devfreq_event device for
>>>>> determining proper current system state and change voltage/frequency
>>>>> dynamically using various governors.
>>>>>
>>>>> - devfreq_event class (new)
>>>>> : Provide measured raw data to devfreq device for governor
>>>>>
>>>>> Cc: MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
>>>>> Cc: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
>>>>> Signed-off-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
>>>>> ---
>>>>> drivers/devfreq/Kconfig | 2 +
>>>>> drivers/devfreq/Makefile | 5 +-
>>>>> drivers/devfreq/devfreq-event.c | 302 ++++++++++++++++++++++++++++++++++++++++
>>>>> drivers/devfreq/event/Makefile | 1 +
>>>>> include/linux/devfreq.h | 141 +++++++++++++++++++
>>>>> 5 files changed, 450 insertions(+), 1 deletion(-)
>>>>> create mode 100644 drivers/devfreq/devfreq-event.c
>>>>> create mode 100644 drivers/devfreq/event/Makefile
>>>>>
>>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>>>> index faf4e70..4d15b62 100644
>>>>> --- a/drivers/devfreq/Kconfig
>>>>> +++ b/drivers/devfreq/Kconfig
>>>>> @@ -87,4 +87,6 @@ config ARM_EXYNOS5_BUS_DEVFREQ
>>>>> It reads PPMU counters of memory controllers and adjusts the
>>>>> operating frequencies and voltages with OPP support.
>>>>>
>>>>> +comment "DEVFREQ Event Drivers"
>>>>> +
>>>>> endif # PM_DEVFREQ
>>>>> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
>>>>> index 16138c9..a1ffabe 100644
>>>>> --- a/drivers/devfreq/Makefile
>>>>> +++ b/drivers/devfreq/Makefile
>>>>> @@ -1,4 +1,4 @@
>>>>> -obj-$(CONFIG_PM_DEVFREQ) += devfreq.o
>>>>> +obj-$(CONFIG_PM_DEVFREQ) += devfreq.o devfreq-event.o
>>>>> obj-$(CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND) += governor_simpleondemand.o
>>>>> obj-$(CONFIG_DEVFREQ_GOV_PERFORMANCE) += governor_performance.o
>>>>> obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE) += governor_powersave.o
>>>>> @@ -7,3 +7,6 @@ obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o
>>>>> # DEVFREQ Drivers
>>>>> obj-$(CONFIG_ARM_EXYNOS4_BUS_DEVFREQ) += exynos/
>>>>> obj-$(CONFIG_ARM_EXYNOS5_BUS_DEVFREQ) += exynos/
>>>>> +
>>>>> +# DEVFREQ Event Drivers
>>>>> +obj-$(CONFIG_PM_DEVFREQ) += event/
>>>>> diff --git a/drivers/devfreq/devfreq-event.c b/drivers/devfreq/devfreq-event.c
>>>>> new file mode 100644
>>>>> index 0000000..b47329f
>>>>> --- /dev/null
>>>>> +++ b/drivers/devfreq/devfreq-event.c
>>>>> @@ -0,0 +1,302 @@
>>>>> +/*
>>>>> + * devfreq-event: Generic DEVFREQ Event class driver
>>>>> + *
>>>>> + * Copyright (C) 2014 Samsung Electronics
>>>>> + * Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>> + * it under the terms of the GNU General Public License version 2 as
>>>>> + * published by the Free Software Foundation.
>>>>> + *
>>>>> + * This driver is based on drivers/devfreq/devfreq.c
>>>>> + */
>>>>> +
>>>>> +#include <linux/kernel.h>
>>>>> +#include <linux/sched.h>
>>>>> +#include <linux/errno.h>
>>>>> +#include <linux/err.h>
>>>>> +#include <linux/init.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/slab.h>
>>>>> +#include <linux/stat.h>
>>>>> +#include <linux/pm_opp.h>
>>>>> +#include <linux/devfreq.h>
>>>>> +#include <linux/workqueue.h>
>>>>> +#include <linux/platform_device.h>
>>>>> +#include <linux/list.h>
>>>>> +#include <linux/printk.h>
>>>>> +#include <linux/hrtimer.h>
>>>>> +#include <linux/of.h>
>>>>> +#include "governor.h"
>>>>> +
>>>>> +static struct class *devfreq_event_class;
>>>>> +
>>>>> +/* The list of all devfreq event list */
>>>>> +static LIST_HEAD(devfreq_event_list);
>>>>> +static DEFINE_MUTEX(devfreq_event_list_lock);
>>>>> +
>>>>> +#define to_devfreq_event(DEV) container_of(DEV, struct devfreq_event_dev, dev)
>>>>> +
>>>>> +struct devfreq_event_dev *devfreq_add_event_device(struct device *dev,
>>>>> + struct devfreq_event_desc *desc)
>>>>> +{
>>>>> + struct devfreq_event_dev *event_dev;
>>>>> + static atomic_t event_no = ATOMIC_INIT(0);
>>>>> + int ret;
>>>>> +
>>>>> + if (!dev || !desc)
>>>>> + return ERR_PTR(-EINVAL);
>>>>> +
>>>>> + if (!desc->name || !desc->ops)
>>>>> + return ERR_PTR(-EINVAL);
>>>>> +
>>>>> + event_dev = kzalloc(sizeof(struct devfreq_event_dev), GFP_KERNEL);
>>>>> + if (!event_dev)
>>>>> + return ERR_PTR(-ENOMEM);
>>>>
>>>> Is this memory freed anywhere when driver is removed? I couldn't find
>>>> it. I couldn't also find function like devfreq_remove_event_device()
>>>> which would be reverting all the work done when adding.
>>>
>>> You're right. I'll use devm_kzalloc instead of kzalloc.
>>>
>>>>
>>>>> +
>>>>> + mutex_lock(&devfreq_event_list_lock);
>>>>> +
>>>>> + mutex_init(&event_dev->lock);
>>>>> + event_dev->desc = desc;
>>>>> + event_dev->dev.parent = dev;
>>>>> + event_dev->dev.class = devfreq_event_class;
>>>>> +
>>>>> + dev_set_name(&event_dev->dev, "event.%d",
>>>>> + atomic_inc_return(&event_no) - 1);
>>>>> + ret = device_register(&event_dev->dev);
>>>>> + if (ret != 0) {
>>>>> + put_device(&event_dev->dev);
>>>>> + goto err;
>>>>> + }
>>>>> + dev_set_drvdata(&event_dev->dev, event_dev);
>>>>> +
>>>>> + /* Add devfreq event device to devfreq_event_list */
>>>>> + INIT_LIST_HEAD(&event_dev->node);
>>>>> + list_add(&event_dev->node, &devfreq_event_list);
>>>>> +
>>>>> + mutex_unlock(&devfreq_event_list_lock);
>>>>> +
>>>>> + return event_dev;
>>>>> +err:
>>>>
>>>> Missing 'mutex_unlock(&devfreq_event_list_lock)' here.
>>>
>>> OK. I'll fix it.
>>>
>>>>
>>>>
>>>>> + kfree(event_dev);
>>>>> + return ERR_PTR(ret);
>>>>> +}
>>>>> +EXPORT_SYMBOL(devfreq_add_event_device);
>>>>> +
>>>>> +struct devfreq_event_dev *devfreq_get_event_dev(const char *event_dev_name)
>>>>> +{
>>>>> + struct devfreq_event_dev *event_dev;
>>>>> +
>>>>> + mutex_lock(&devfreq_event_list_lock);
>>>>> + list_for_each_entry(event_dev, &devfreq_event_list, node) {
>>>>> + if (!strcmp(event_dev->desc->name, event_dev_name))
>>>>> + goto out;
>>>>> + }
>>>>> + event_dev = NULL;
>>>>> +out:
>>>>> + mutex_unlock(&devfreq_event_list_lock);
>>>>> +
>>>>> + return event_dev;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(devfreq_get_event_dev);
>>>>> +
>>>>> +struct devfreq_event_dev *devfreq_get_event_dev_by_phandle(struct device *dev,
>>>>> + int index)
>>>>> +{
>>>>> + struct device_node *node;
>>>>> + struct devfreq_event_dev *event_dev;
>>>>> +
>>>>> + if (!dev->of_node) {
>>>>> + dev_err(dev, "device does not have a device node entry\n");
>>>>> + return ERR_PTR(-EINVAL);
>>>>> + }
>>>>> +
>>>>> + node = of_parse_phandle(dev->of_node, "devfreq-events", index);
>>>>> + if (!node) {
>>>>> + dev_err(dev, "failed to get phandle in %s node\n",
>>>>> + dev->of_node->full_name);
>>>>> + return ERR_PTR(-ENODEV);
>>>>> + }
>>>>> +
>>>>> + event_dev = devfreq_get_event_dev(node->name);
>>>>> + if (!event_dev) {
>>>>> + dev_err(dev, "unable to get devfreq-event device : %s\n",
>>>>> + node->name);
>>>>
>>>> of_node_put() for node obtained with of_parse_phandle().
>>>
>>> OK. I'll add it.
>>
>> of_parse_phandle() seems that it don't need of_node_put().
>
> The of_parse_phandle() increases the refcount for node it returns.
>>From documentation:
>
> /**
> * Returns the device_node pointer with refcount incremented. Use
> * of_node_put() on it when done.
> */
>
> The function returns error condition but node was found and its refcnt
> was incremented. Then why do you think that of_node_put() is not
> necessary here?

OK, I'll call of_node_put() as following:

diff --git a/drivers/devfreq/devfreq-event.c b/drivers/devfreq/devfreq-event.c
index cd9c485..a1edafc 100644
--- a/drivers/devfreq/devfreq-event.c
+++ b/drivers/devfreq/devfreq-event.c
@@ -282,9 +282,12 @@ out:
if (!edev) {
dev_err(dev, "unable to get devfreq-event device : %s\n",
node->name);
+ of_node_put(node);
return ERR_PTR(-ENODEV);
}

+ of_node_put(node);
+
return edev;
}

>
>>
>>>
>>>>
>>>>> + return ERR_PTR(-ENODEV);
>>>>> + }
>>>>> +
>>>>> + return event_dev;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(devfreq_get_event_dev_by_phandle);
>>>>> +
>>>>> +int devfreq_put_event_dev(struct devfreq_event_dev *event_dev)
>>>>> +{
>>>>
>>>> of_node_put() here to decrement refcnt from of_parse_phandle()?
>>>
>>> It is my mistake. I think that devfreq_put_event_dev is not necesssary.
>>> I'll remove it.
>
> Then how to decrease the refcnt for node obtained in
> devfreq_get_event_dev_by_phandle()?

I'll call of_node_put in devfreq_event_get_edev_by_phandle() according to your comment.

Thanks for your review.

Best Regards,
Chanwoo Choi

--
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/

Next message: Brian Norris: "Re: Maintainer abuse"
Previous message: Alexei Starovoitov: "Re: [LKP] [ipmi] BUG: key ffff880fcfc51ed0 not in .data!"
In reply to: Krzysztof Kozlowski: "Re: [RFC PATCHv2 1/7] devfreq: event: Add new devfreq_event class to provide basic data for devfreq governor"
Next in thread: Chanwoo Choi: "[RFC PATCHv2 7/7] ARM: dts: exynos: Add PPMU dt node to Exynos3250-based Rinato board"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]