Re: Re: [PATCHv4 1/8] devfreq: event: Add new devfreq_event class to provide basic data for devfreq governor
Dear Myungjoo,
Thanks for your review.
On 12/18/2014 03:24 PM, MyungJoo Ham wrote:
Hi Chanwoo,
I love the idea and I now have a little mechanical issues in your code.
---
drivers/devfreq/Kconfig | 2 +
drivers/devfreq/Makefile| 5 +-
drivers/devfreq/devfreq-event.c | 449
drivers/devfreq/event/Makefile | 1 +
include/linux/devfreq.h | 160 ++
5 files changed, 616 insertions(+), 1 deletion(-)
create mode 100644 drivers/devfreq/devfreq-event.c
create mode 100644 drivers/devfreq/event/Makefile
[]
[snip]
diff --git a/drivers/devfreq/devfreq-event.c
b/drivers/devfreq/devfreq-event.c
new file mode 100644
index 000..0e1948e
--- /dev/null
+++ b/drivers/devfreq/devfreq-event.c
@@ -0,0 +1,449 @@
+/*
+ * devfreq-event: Generic DEVFREQ Event class driver
DEVFREQ is a generic DVFS mechanism (or subsystem).
Plus, I thought devfreq-event is considered to be a framework
for devfreq event class drivers. Am I mistaken?
You're right. just class driver description is not proper.
I'll modify the description of devfreq-event.c as following:
or If you have other opinion, would you please let me know about it?
devfreq-event: DEVFREQ-Event Framework to provide raw data of Non-CPU
Devices.
devfreq-event: a framework to provide raw data and events of devfreq devices
should be enough.
[]
[snip / reversed maybe.. sorry]
+/**
+ * devfreq_event_is_enabled() - Check whether devfreq-event dev is
enabled or
+ * not.
+ * @edev : the devfreq-event device
+ *
+ * Note that this function check whether devfreq-event dev is enabled or
not.
+ * If return true, the devfreq-event dev is enabeld. If return false, the
+ * devfreq-event dev is disabled.
+ */
+bool devfreq_event_is_enabled(struct devfreq_event_dev *edev)
+{
+ bool enabled = false;
+
+ if (!edev || !edev-desc)
+ return enabled;
+
+ mutex_lock(edev-lock);
+
+ if (edev-enable_count 0)
+ enabled = true;
+
+ if (edev-desc-ops edev-desc-ops-is_enabled)
+ enabled |= edev-desc-ops-is_enabled(edev);
What does it mean when enabled_count 0 and ops-is_enabled() is false?
or..
What does it mean when enabled_count = 0 and ops-is_enabled() is true?
If you do enable_count in the subsystem, why would we rely on
ops-is_enabled()? Are you assuming that a device MAY turn itself off
without any kernel control (ops-disable()) and it is still a correct
behabior?
You're right. devfreq_event_is_enabled() has ambiguous operation according to
your comment.
I'll only control the enable_count in the subsystem without ops-is_enabled()
and then remove the is_enabled function in the structre devfreq_event_ops.
Best Regards,
Chanwoo Choi
[Off-Topic]
The name of devfreq-event may look quite intersecting with irq-driven governors,
which are being proposed these days.
Although they may look intersecting, we can have them independently;
this as a sub-class and that as a governor. And we can consider to
provide a common infrastructure for irq-driven mechanisms in devfreq or
devfreq-event when we irq-driven DVFS become more general, which I
expect in 2 ~ 3 years.
So for now, both can go independently.
Cheers!
MyungJoo
N�r��yb�X��ǧv�^�){.n�+{�x,�ȧ���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf
Re: [PATCHv4 1/8] devfreq: event: Add new devfreq_event class to provide basic data for devfreq governor
Dear Myungjoo,
On 12/19/2014 11:11 AM, MyungJoo Ham wrote:
Dear Myungjoo,
Thanks for your review.
On 12/18/2014 03:24 PM, MyungJoo Ham wrote:
Hi Chanwoo,
I love the idea and I now have a little mechanical issues in your code.
---
drivers/devfreq/Kconfig | 2 +
drivers/devfreq/Makefile| 5 +-
drivers/devfreq/devfreq-event.c | 449
drivers/devfreq/event/Makefile | 1 +
include/linux/devfreq.h | 160 ++
5 files changed, 616 insertions(+), 1 deletion(-)
create mode 100644 drivers/devfreq/devfreq-event.c
create mode 100644 drivers/devfreq/event/Makefile
[]
[snip]
diff --git a/drivers/devfreq/devfreq-event.c
b/drivers/devfreq/devfreq-event.c
new file mode 100644
index 000..0e1948e
--- /dev/null
+++ b/drivers/devfreq/devfreq-event.c
@@ -0,0 +1,449 @@
+/*
+ * devfreq-event: Generic DEVFREQ Event class driver
DEVFREQ is a generic DVFS mechanism (or subsystem).
Plus, I thought devfreq-event is considered to be a framework
for devfreq event class drivers. Am I mistaken?
You're right. just class driver description is not proper.
I'll modify the description of devfreq-event.c as following:
or If you have other opinion, would you please let me know about it?
devfreq-event: DEVFREQ-Event Framework to provide raw data of Non-CPU
Devices.
devfreq-event: a framework to provide raw data and events of devfreq devices
should be enough.
OK, I'll modify it.
[]
[snip / reversed maybe.. sorry]
+/**
+ * devfreq_event_is_enabled() - Check whether devfreq-event dev is
enabled or
+ * not.
+ * @edev : the devfreq-event device
+ *
+ * Note that this function check whether devfreq-event dev is enabled or
not.
+ * If return true, the devfreq-event dev is enabeld. If return false, the
+ * devfreq-event dev is disabled.
+ */
+bool devfreq_event_is_enabled(struct devfreq_event_dev *edev)
+{
+ bool enabled = false;
+
+ if (!edev || !edev-desc)
+ return enabled;
+
+ mutex_lock(edev-lock);
+
+ if (edev-enable_count 0)
+ enabled = true;
+
+ if (edev-desc-ops edev-desc-ops-is_enabled)
+ enabled |= edev-desc-ops-is_enabled(edev);
What does it mean when enabled_count 0 and ops-is_enabled() is false?
or..
What does it mean when enabled_count = 0 and ops-is_enabled() is true?
If you do enable_count in the subsystem, why would we rely on
ops-is_enabled()? Are you assuming that a device MAY turn itself off
without any kernel control (ops-disable()) and it is still a correct
behabior?
You're right. devfreq_event_is_enabled() has ambiguous operation according
to your comment.
I'll only control the enable_count in the subsystem without ops-is_enabled()
and then remove the is_enabled function in the structre devfreq_event_ops.
Best Regards,
Chanwoo Choi
[Off-Topic]
The name of devfreq-event may look quite intersecting with irq-driven
governors,
which are being proposed these days.
Although they may look intersecting, we can have them independently;
this as a sub-class and that as a governor. And we can consider to
provide a common infrastructure for irq-driven mechanisms in devfreq or
devfreq-event when we irq-driven DVFS become more general, which I
expect in 2 ~ 3 years.
So for now, both can go independently.
I understand your opinion.
I want to handle the devfreq-event framework independently from irq-driven
governor.
After completing the devfreq-event and the support for exynos-busfreq dt,
If you agree, I'll consider how to implement irq-driven governor as the devfreq
governor.
Best Regards,
Chanwoo Choi
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 1/8] devfreq: event: Add new devfreq_event class to provide basic data for devfreq governor
Hi Chanwoo,
I love the idea and I now have a little mechanical issues in your code.
---
drivers/devfreq/Kconfig | 2 +
drivers/devfreq/Makefile| 5 +-
drivers/devfreq/devfreq-event.c | 449
drivers/devfreq/event/Makefile | 1 +
include/linux/devfreq.h | 160 ++
5 files changed, 616 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/
It looks getting mature fast.
However, I would like to suggest you to
allow not to compile devfreq-event.c and not include its compiled object
if devfreq.c is required but devfreq-event.c is not required.
(e.g., add CONFIG_PM_DEVFREQ_EVENT and let it be enabled when needed)
just a little concern for lightweight devices.
(this change might require a bit more work on the header as well)
- Or do you think devfreq-event.c will become almost mandatory for
most devfreq drivers?
[snip]
diff --git a/drivers/devfreq/devfreq-event.c b/drivers/devfreq/devfreq-event.c
new file mode 100644
index 000..0e1948e
--- /dev/null
+++ b/drivers/devfreq/devfreq-event.c
@@ -0,0 +1,449 @@
+/*
+ * devfreq-event: Generic DEVFREQ Event class driver
DEVFREQ is a generic DVFS mechanism (or subsystem).
Plus, I thought devfreq-event is considered to be a framework
for devfreq event class drivers. Am I mistaken?
[snip]
+struct devfreq_event_dev *devfreq_event_add_edev(struct device *dev,
+ struct devfreq_event_desc
*desc)
+{
+ struct devfreq_event_dev *edev;
+ 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);
+
+ if (!desc-ops-set_event || !desc-ops-get_event)
+ return ERR_PTR(-EINVAL);
+
+ edev = devm_kzalloc(dev, sizeof(*edev), GFP_KERNEL);
+ if (!edev)
+ return ERR_PTR(-ENOMEM);
+
+ mutex_lock(devfreq_event_list_lock);
You seem to lock that global lock too long. That lock is only required
while you operate the list. The data to be protected by this mutex is
devfreq_event_list. Until the new entry is added to the list, the new
entry is free from protection. (may be delayed right before list_add)
+ mutex_init(edev-lock);
+ edev-desc = desc;
+ edev-dev.parent = dev;
+ edev-dev.class = devfreq_event_class;
+ edev-dev.release = devfreq_event_release_edev;
+
+ dev_set_name(edev-dev, event.%d, atomic_inc_return(event_no) -
1);
+ ret = device_register(edev-dev);
+ if (ret 0) {
+ put_device(edev-dev);
+ mutex_unlock(devfreq_event_list_lock);
+ return ERR_PTR(ret);
+ }
+ dev_set_drvdata(edev-dev, edev);
+
+ INIT_LIST_HEAD(edev-node);
+ list_add(edev-node, devfreq_event_list);
+ mutex_unlock(devfreq_event_list_lock);
+
+ return edev;
+}
[snip / reversed maybe.. sorry]
+/**
+ * devfreq_event_is_enabled() - Check whether devfreq-event dev is enabled or
+ * not.
+ * @edev : the devfreq-event device
+ *
+ * Note that this function check whether devfreq-event dev is enabled or not.
+ * If return true, the devfreq-event dev is enabeld. If return false, the
+ * devfreq-event dev is disabled.
+ */
+bool devfreq_event_is_enabled(struct devfreq_event_dev *edev)
+{
+ bool enabled = false;
+
+ if (!edev || !edev-desc)
+ return enabled;
+
+ mutex_lock(edev-lock);
+
+ if (edev-enable_count 0)
+ enabled
Re: [PATCHv4 1/8] devfreq: event: Add new devfreq_event class to provide basic data for devfreq governor
Dear Myungjoo,
Thanks for your review.
On 12/18/2014 03:24 PM, MyungJoo Ham wrote:
Hi Chanwoo,
I love the idea and I now have a little mechanical issues in your code.
---
drivers/devfreq/Kconfig | 2 +
drivers/devfreq/Makefile| 5 +-
drivers/devfreq/devfreq-event.c | 449
drivers/devfreq/event/Makefile | 1 +
include/linux/devfreq.h | 160 ++
5 files changed, 616 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/
It looks getting mature fast.
However, I would like to suggest you to
allow not to compile devfreq-event.c and not include its compiled object
if devfreq.c is required but devfreq-event.c is not required.
(e.g., add CONFIG_PM_DEVFREQ_EVENT and let it be enabled when needed)
just a little concern for lightweight devices.
(this change might require a bit more work on the header as well)
- Or do you think devfreq-event.c will become almost mandatory for
most devfreq drivers?
I agree your opinion.
I'll add CONFIG_PM_DEVFREQ_EVENT according to your comment.
[snip]
diff --git a/drivers/devfreq/devfreq-event.c
b/drivers/devfreq/devfreq-event.c
new file mode 100644
index 000..0e1948e
--- /dev/null
+++ b/drivers/devfreq/devfreq-event.c
@@ -0,0 +1,449 @@
+/*
+ * devfreq-event: Generic DEVFREQ Event class driver
DEVFREQ is a generic DVFS mechanism (or subsystem).
Plus, I thought devfreq-event is considered to be a framework
for devfreq event class drivers. Am I mistaken?
You're right. just class driver description is not proper.
I'll modify the description of devfreq-event.c as following:
or If you have other opinion, would you please let me know about it?
devfreq-event: DEVFREQ-Event Framework to provide raw data of Non-CPU
Devices.
[snip]
+struct devfreq_event_dev *devfreq_event_add_edev(struct device *dev,
+ struct devfreq_event_desc
*desc)
+{
+ struct devfreq_event_dev *edev;
+ 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);
+
+ if (!desc-ops-set_event || !desc-ops-get_event)
+ return ERR_PTR(-EINVAL);
+
+ edev = devm_kzalloc(dev, sizeof(*edev), GFP_KERNEL);
+ if (!edev)
+ return ERR_PTR(-ENOMEM);
+
+ mutex_lock(devfreq_event_list_lock);
You seem to lock that global lock too long. That lock is only required
while you operate the list. The data to be protected by this mutex is
devfreq_event_list. Until the new entry is added to the list, the new
entry is free from protection. (may be delayed right before list_add)
OK. I'll move global lock right before calling list_add() on below.
+ mutex_init(edev-lock);
+ edev-desc = desc;
+ edev-dev.parent = dev;
+ edev-dev.class = devfreq_event_class;
+ edev-dev.release = devfreq_event_release_edev;
+
+ dev_set_name(edev-dev, event.%d, atomic_inc_return(event_no) -
1);
+ ret = device_register(edev-dev);
+ if (ret 0) {
+ put_device(edev-dev);
+ mutex_unlock(devfreq_event_list_lock);
+ return ERR_PTR(ret);
+ }
+ dev_set_drvdata(edev-dev, edev);
+
+ INIT_LIST_HEAD(edev-node);
+ list_add(edev-node, devfreq_event_list);
+ mutex_unlock(devfreq_event_list_lock);
+
+ return edev;
+}
[snip / reversed maybe.. sorry]
+/**
+ * devfreq_event_is_enabled() - Check whether devfreq-event dev is enabled
or
+ *
