* [PATCHv7 02/10] devfreq: event: Add the list of supported devfreq-event type
2015-01-07 23:51 [PATCHv7 00/10] devfreq: Add devfreq-event class to provide raw data for devfreq device Chanwoo Choi
@ 2015-01-07 23:51 ` Chanwoo Choi
0 siblings, 0 replies; 3+ messages in thread
From: Chanwoo Choi @ 2015-01-07 23:51 UTC (permalink / raw)
To: myungjoo.ham, kgene
Cc: kyungmin.park, rafael.j.wysocki, mark.rutland, a.kesavan,
tomasz.figa, k.kozlowski, b.zolnierkie, robh+dt, cw00.choi,
inki.dae, linux-pm, linux-kernel, linux-arm-kernel,
linux-samsung-soc
This patch adds the list of supported devfreq-event type as following.
Each devfreq-event device driver would support the various devfreq-event type
for devfreq governor at the same time.
- DEVFREQ_EVENT_TYPE_RAW_DATA
- DEVFREQ_EVENT_TYPE_UTILIZATION
- DEVFREQ_EVENT_TYPE_BANDWIDTH
- DEVFREQ_EVENT_TYPE_LATENCY
Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
drivers/devfreq/devfreq-event.c | 58 ++++++++++++++++++++++++++++++++++++-----
include/linux/devfreq-event.h | 25 +++++++++++++++---
2 files changed, 73 insertions(+), 10 deletions(-)
diff --git a/drivers/devfreq/devfreq-event.c b/drivers/devfreq/devfreq-event.c
index 81448ba..64c1764 100644
--- a/drivers/devfreq/devfreq-event.c
+++ b/drivers/devfreq/devfreq-event.c
@@ -20,6 +20,9 @@
#include <linux/list.h>
#include <linux/of.h>
+#define EVENT_TYPE_RAW_DATA_MAX ULONG_MAX
+#define EVENT_TYPE_UTILIZATION_MAX 100
+
static struct class *devfreq_event_class;
/* The list of all devfreq event list */
@@ -132,7 +135,8 @@ EXPORT_SYMBOL_GPL(devfreq_event_is_enabled);
* Note that this function set the event to the devfreq-event device to start
* for getting the event data which could be various event type.
*/
-int devfreq_event_set_event(struct devfreq_event_dev *edev)
+int devfreq_event_set_event(struct devfreq_event_dev *edev,
+ enum devfreq_event_type type)
{
int ret;
@@ -146,7 +150,15 @@ int devfreq_event_set_event(struct devfreq_event_dev *edev)
return -EPERM;
mutex_lock(&edev->lock);
- ret = edev->desc->ops->set_event(edev);
+
+ if ((edev->desc->type & type) == 0) {
+ dev_err(&edev->dev, "unsupported devfreq-event type\n");
+ mutex_unlock(&edev->lock);
+ return -EINVAL;
+ }
+
+ ret = edev->desc->ops->set_event(edev, type);
+
mutex_unlock(&edev->lock);
return ret;
@@ -162,6 +174,7 @@ EXPORT_SYMBOL_GPL(devfreq_event_set_event);
* after stoping the progress of whole sequence of devfreq-event dev.
*/
int devfreq_event_get_event(struct devfreq_event_dev *edev,
+ enum devfreq_event_type type,
struct devfreq_event_data *edata)
{
int ret;
@@ -175,18 +188,49 @@ int devfreq_event_get_event(struct devfreq_event_dev *edev,
if (!devfreq_event_is_enabled(edev))
return -EINVAL;
+ mutex_lock(&edev->lock);
+
+ if ((edev->desc->type & type) == 0) {
+ dev_err(&edev->dev, "unsupported devfreq-event type\n");
+ return -EINVAL;
+ }
+
edata->event = edata->total_event = 0;
+ ret = edev->desc->ops->get_event(edev, type, edata);
+ if (ret < 0
+ || edata->total_event <= 0
+ || edata->event > edata->total_event) {
+ edata->event = edata->total_event = 0;
+ mutex_unlock(&edev->lock);
+ return -EINVAL;
+ }
- mutex_lock(&edev->lock);
- ret = edev->desc->ops->get_event(edev, edata);
- mutex_unlock(&edev->lock);
+ switch (type) {
+ case DEVFREQ_EVENT_TYPE_RAW_DATA:
+ case DEVFREQ_EVENT_TYPE_BANDWIDTH:
+ case DEVFREQ_EVENT_TYPE_LATENCY:
+ if ((edata->event > EVENT_TYPE_RAW_DATA_MAX) ||
+ (edata->total_event > EVENT_TYPE_RAW_DATA_MAX)) {
+ edata->event = edata->total_event = 0;
+ ret = -EINVAL;
+ }
+ break;
+ case DEVFREQ_EVENT_TYPE_UTILIZATION:
+ edata->total_event = EVENT_TYPE_UTILIZATION_MAX;
- if ((edata->total_event <= 0)
- || (edata->event > edata->total_event)) {
+ if (edata->event > EVENT_TYPE_UTILIZATION_MAX) {
+ edata->event = edata->total_event = 0;
+ ret = -EINVAL;
+ }
+ break;
+ default:
edata->event = edata->total_event = 0;
ret = -EINVAL;
+ break;
}
+ mutex_unlock(&edev->lock);
+
return ret;
}
EXPORT_SYMBOL_GPL(devfreq_event_get_event);
diff --git a/include/linux/devfreq-event.h b/include/linux/devfreq-event.h
index b7363f5..13a5703 100644
--- a/include/linux/devfreq-event.h
+++ b/include/linux/devfreq-event.h
@@ -36,6 +36,14 @@ struct devfreq_event_dev {
const struct devfreq_event_desc *desc;
};
+/* The supported type by devfreq-event device */
+enum devfreq_event_type {
+ DEVFREQ_EVENT_TYPE_RAW_DATA = BIT(0),
+ DEVFREQ_EVENT_TYPE_UTILIZATION = BIT(1),
+ DEVFREQ_EVENT_TYPE_BANDWIDTH = BIT(2),
+ DEVFREQ_EVENT_TYPE_LATENCY = BIT(3),
+};
+
/**
* struct devfreq_event_data - the devfreq-event data
*
@@ -69,8 +77,10 @@ struct devfreq_event_ops {
int (*reset)(struct devfreq_event_dev *edev);
/* Mandatory functions */
- int (*set_event)(struct devfreq_event_dev *edev);
+ int (*set_event)(struct devfreq_event_dev *edev,
+ enum devfreq_event_type type);
int (*get_event)(struct devfreq_event_dev *edev,
+ enum devfreq_event_type type,
struct devfreq_event_data *edata);
};
@@ -79,6 +89,10 @@ struct devfreq_event_ops {
*
* @name : the name of devfreq-event device.
* @driver_data : the private data for devfreq-event driver.
+ * @event_type : the supported devfreq-event type among as following
+ * - DEVFREQ_EVENT_TYPE_UTILIZATION
+ * - DEVFREQ_EVENT_TYPE_BANDWIDTH
+ * - DEVFREQ_EVENT_TYPE_LATENCY
* @ops : the operation to control devfreq-event device.
*
* Each devfreq-event device is described with a this structure.
@@ -87,6 +101,7 @@ struct devfreq_event_ops {
struct devfreq_event_desc {
const char *name;
void *driver_data;
+ enum devfreq_event_type type;
struct devfreq_event_ops *ops;
};
@@ -95,8 +110,10 @@ struct devfreq_event_desc {
extern int devfreq_event_enable_edev(struct devfreq_event_dev *edev);
extern int devfreq_event_disable_edev(struct devfreq_event_dev *edev);
extern bool devfreq_event_is_enabled(struct devfreq_event_dev *edev);
-extern int devfreq_event_set_event(struct devfreq_event_dev *edev);
+extern int devfreq_event_set_event(struct devfreq_event_dev *edev,
+ enum devfreq_event_type type);
extern int devfreq_event_get_event(struct devfreq_event_dev *edev,
+ enum devfreq_event_type type,
struct devfreq_event_data *edata);
extern int devfreq_event_reset_event(struct devfreq_event_dev *edev);
extern void *devfreq_event_get_drvdata(struct devfreq_event_dev *edev);
@@ -123,12 +140,14 @@ static inline bool devfreq_event_is_enabled(struct devfreq_event_dev *edev)
return false;
}
-static inline int devfreq_event_set_event(struct devfreq_event_dev *edev)
+static inline int devfreq_event_set_event(struct devfreq_event_dev *edev,
+ enum devfreq_event_type type)
{
return -EINVAL;
}
static inline int devfreq_event_get_event(struct devfreq_event_dev *edev,
+ enum devfreq_event_type type,
struct devfreq_event_data *edata)
{
return -EINVAL;
--
1.8.5.5
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCHv7 02/10] devfreq: event: Add the list of supported devfreq-event type
@ 2015-01-12 7:15 MyungJoo Ham
2015-01-12 11:17 ` Chanwoo Choi
0 siblings, 1 reply; 3+ messages in thread
From: MyungJoo Ham @ 2015-01-12 7:15 UTC (permalink / raw)
To: 최찬우, kgene@kernel.org
Cc: 박경민, rafael.j.wysocki@intel.com,
mark.rutland@arm.com, ABHILASH KESAVAN, tomasz.figa@gmail.com,
Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
robh+dt@kernel.org, 대인기,
linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org
>
> This patch adds the list of supported devfreq-event type as following.
> Each devfreq-event device driver would support the various devfreq-event type
> for devfreq governor at the same time.
> - DEVFREQ_EVENT_TYPE_RAW_DATA
> - DEVFREQ_EVENT_TYPE_UTILIZATION
> - DEVFREQ_EVENT_TYPE_BANDWIDTH
> - DEVFREQ_EVENT_TYPE_LATENCY
Did you try to say:
A devfreq-event device may support multiple devfreq-event types
simultaneously.
If so, your switch expressions are going to screw up.
>
> Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
> drivers/devfreq/devfreq-event.c | 58 ++++++++++++++++++++++++++++++++++++-----
> include/linux/devfreq-event.h | 25 +++++++++++++++---
> 2 files changed, 73 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq-event.c b/drivers/devfreq/devfreq-event.c
> index 81448ba..64c1764 100644
> --- a/drivers/devfreq/devfreq-event.c
> +++ b/drivers/devfreq/devfreq-event.c
>
[]
> - mutex_lock(&edev->lock);
> - ret = edev->desc->ops->get_event(edev, edata);
> - mutex_unlock(&edev->lock);
> + switch (type) {
Bitwise value with switch? (what if type = RAW_DATA | BANDWIDTH, meaning
this is raw data of the bandwitdh.)
> + case DEVFREQ_EVENT_TYPE_RAW_DATA:
> + case DEVFREQ_EVENT_TYPE_BANDWIDTH:
> + case DEVFREQ_EVENT_TYPE_LATENCY:
> + if ((edata->event > EVENT_TYPE_RAW_DATA_MAX) ||
> + (edata->total_event > EVENT_TYPE_RAW_DATA_MAX)) {
Is it possible for unsigned long edata->event/total_event to be
> EVENT_TYPE_RAW_DATA_MAX = ULONG_MAX?
What was your intention?
If you were trying to detect overflow, you need to rethink about it.
If not, (overflow is harmless or not going to happen) you don't need to
check it.
> + edata->event = edata->total_event = 0;
> + ret = -EINVAL;
> + }
> + break;
> + case DEVFREQ_EVENT_TYPE_UTILIZATION:
> + edata->total_event = EVENT_TYPE_UTILIZATION_MAX;
>
> - if ((edata->total_event <= 0)
> - || (edata->event > edata->total_event)) {
> + if (edata->event > EVENT_TYPE_UTILIZATION_MAX) {
> + edata->event = edata->total_event = 0;
> + ret = -EINVAL;
> + }
> + break;
> + default:
> edata->event = edata->total_event = 0;
> ret = -EINVAL;
> + break;
> }
>
> + mutex_unlock(&edev->lock);
> +
> return ret;
> }
> EXPORT_SYMBOL_GPL(devfreq_event_get_event);
> diff --git a/include/linux/devfreq-event.h b/include/linux/devfreq-event.h
> index b7363f5..13a5703 100644
> --- a/include/linux/devfreq-event.h
> +++ b/include/linux/devfreq-event.h
> @@ -36,6 +36,14 @@ struct devfreq_event_dev {
> const struct devfreq_event_desc *desc;
> };
>
> +/* The supported type by devfreq-event device */
> +enum devfreq_event_type {
> + DEVFREQ_EVENT_TYPE_RAW_DATA = BIT(0),
> + DEVFREQ_EVENT_TYPE_UTILIZATION = BIT(1),
> + DEVFREQ_EVENT_TYPE_BANDWIDTH = BIT(2),
> + DEVFREQ_EVENT_TYPE_LATENCY = BIT(3),
> +};
> +
(Being curious) Is it possible to have multiple types
simultaneously?
[]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCHv7 02/10] devfreq: event: Add the list of supported devfreq-event type
2015-01-12 7:15 [PATCHv7 02/10] devfreq: event: Add the list of supported devfreq-event type MyungJoo Ham
@ 2015-01-12 11:17 ` Chanwoo Choi
0 siblings, 0 replies; 3+ messages in thread
From: Chanwoo Choi @ 2015-01-12 11:17 UTC (permalink / raw)
To: myungjoo.ham
Cc: kgene@kernel.org, 박경민,
rafael.j.wysocki@intel.com, mark.rutland@arm.com,
ABHILASH KESAVAN, tomasz.figa@gmail.com, Krzysztof Kozlowski,
Bartlomiej Zolnierkiewicz, robh+dt@kernel.org,
대인기, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org
Dear Myungjoo,
On 01/12/2015 04:15 PM, MyungJoo Ham wrote:
>>
>> This patch adds the list of supported devfreq-event type as following.
>> Each devfreq-event device driver would support the various devfreq-event type
>> for devfreq governor at the same time.
>> - DEVFREQ_EVENT_TYPE_RAW_DATA
>> - DEVFREQ_EVENT_TYPE_UTILIZATION
>> - DEVFREQ_EVENT_TYPE_BANDWIDTH
>> - DEVFREQ_EVENT_TYPE_LATENCY
>
> Did you try to say:
>
> A devfreq-event device may support multiple devfreq-event types
> simultaneously.
I think that one devfreq-event device can support multiple devfreq-event types.
but, devfreq-event device might provide only value at one point.
But,
This patch is ambiguous and includes a bug according to your comment
(below switch statement). I'll drop this patch on next patch-set.
This patch will be posted on further patch-set after resolving some issue.
Best Regards,
Chanwoo Choi
>
> If so, your switch expressions are going to screw up.
>
>
>>
>> Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>> drivers/devfreq/devfreq-event.c | 58 ++++++++++++++++++++++++++++++++++++-----
>> include/linux/devfreq-event.h | 25 +++++++++++++++---
>> 2 files changed, 73 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq-event.c b/drivers/devfreq/devfreq-event.c
>> index 81448ba..64c1764 100644
>> --- a/drivers/devfreq/devfreq-event.c
>> +++ b/drivers/devfreq/devfreq-event.c
>>
> []
>> - mutex_lock(&edev->lock);
>> - ret = edev->desc->ops->get_event(edev, edata);
>> - mutex_unlock(&edev->lock);
>> + switch (type) {
>
> Bitwise value with switch? (what if type = RAW_DATA | BANDWIDTH, meaning
> this is raw data of the bandwitdh.)
>
>> + case DEVFREQ_EVENT_TYPE_RAW_DATA:
>> + case DEVFREQ_EVENT_TYPE_BANDWIDTH:
>> + case DEVFREQ_EVENT_TYPE_LATENCY:
>> + if ((edata->event > EVENT_TYPE_RAW_DATA_MAX) ||
>> + (edata->total_event > EVENT_TYPE_RAW_DATA_MAX)) {
>
> Is it possible for unsigned long edata->event/total_event to be
> > EVENT_TYPE_RAW_DATA_MAX = ULONG_MAX?
>
> What was your intention?
>
> If you were trying to detect overflow, you need to rethink about it.
> If not, (overflow is harmless or not going to happen) you don't need to
> check it.
>
>
>> + edata->event = edata->total_event = 0;
>> + ret = -EINVAL;
>> + }
>> + break;
>> + case DEVFREQ_EVENT_TYPE_UTILIZATION:
>> + edata->total_event = EVENT_TYPE_UTILIZATION_MAX;
>>
>> - if ((edata->total_event <= 0)
>> - || (edata->event > edata->total_event)) {
>> + if (edata->event > EVENT_TYPE_UTILIZATION_MAX) {
>> + edata->event = edata->total_event = 0;
>> + ret = -EINVAL;
>> + }
>> + break;
>> + default:
>> edata->event = edata->total_event = 0;
>> ret = -EINVAL;
>> + break;
>> }
>>
>> + mutex_unlock(&edev->lock);
>> +
>> return ret;
>> }
>> EXPORT_SYMBOL_GPL(devfreq_event_get_event);
>> diff --git a/include/linux/devfreq-event.h b/include/linux/devfreq-event.h
>> index b7363f5..13a5703 100644
>> --- a/include/linux/devfreq-event.h
>> +++ b/include/linux/devfreq-event.h
>> @@ -36,6 +36,14 @@ struct devfreq_event_dev {
>> const struct devfreq_event_desc *desc;
>> };
>>
>> +/* The supported type by devfreq-event device */
>> +enum devfreq_event_type {
>> + DEVFREQ_EVENT_TYPE_RAW_DATA = BIT(0),
>> + DEVFREQ_EVENT_TYPE_UTILIZATION = BIT(1),
>> + DEVFREQ_EVENT_TYPE_BANDWIDTH = BIT(2),
>> + DEVFREQ_EVENT_TYPE_LATENCY = BIT(3),
>> +};
>> +
>
> (Being curious) Is it possible to have multiple types
> simultaneously?
>
>
> []
> N�����r��y���b�X��ǧv�^�){.n�+����{�����x,�ȧ�\x17��ܨ}���Ơz�&j:+v���\a����zZ+��+zf���h���~����i���z�\x1e�w���?����&�)ߢ^[fl===
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-01-12 11:17 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-01-12 7:15 [PATCHv7 02/10] devfreq: event: Add the list of supported devfreq-event type MyungJoo Ham
2015-01-12 11:17 ` Chanwoo Choi
-- strict thread matches above, loose matches on Subject: below --
2015-01-07 23:51 [PATCHv7 00/10] devfreq: Add devfreq-event class to provide raw data for devfreq device Chanwoo Choi
2015-01-07 23:51 ` [PATCHv7 02/10] devfreq: event: Add the list of supported devfreq-event type Chanwoo Choi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).