From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754568AbcC1Gyd (ORCPT ); Mon, 28 Mar 2016 02:54:33 -0400 Received: from mailout2.samsung.com ([203.254.224.25]:35196 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753081AbcC1Gy3 convert rfc822-to-8bit (ORCPT ); Mon, 28 Mar 2016 02:54:29 -0400 X-AuditID: cbfee691-f795a6d0000012b5-6b-56f8d52021e2 MIME-version: 1.0 Content-type: text/plain; charset=utf-8 Content-transfer-encoding: 8BIT Message-id: <56F8D51F.1030306@samsung.com> Date: Mon, 28 Mar 2016 15:54:23 +0900 From: Chanwoo Choi User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 To: myungjoo.ham@samsung.com, =?UTF-8?B?67CV6rK966+8?= , =?UTF-8?B?7YGs7Ims7Iuc7Yag7ZSE?= , "kgene@kernel.org" Cc: "rjw@rjwysocki.net" , "robh+dt@kernel.org" , "pawel.moll@arm.com" , "mark.rutland@arm.com" , "ijc+devicetree@hellion.org.uk" , "galak@codeaurora.org" , "linux@arm.linux.org.uk" , "linux.amoon@gmail.com" , "m.reichl@fivetechno.de" , "tjakobi@math.uni-bielefeld.de" , =?UTF-8?B?64yA7J246riw?= , "linux-kernel@vger.kernel.org" , "linux-pm@vger.kernel.org" , "linux-samsung-soc@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "devicetree@vger.kernel.org" Subject: Re: [PATCH v6 06/21] PM / devfreq: Add new passive governor References: <1314094711.28841459134703593.JavaMail.weblogic@epmlwas02a> In-reply-to: <1314094711.28841459134703593.JavaMail.weblogic@epmlwas02a> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprBKsWRmVeSWpSXmKPExsWyRsSkWFfh6o8wg5ZWFYv5R86xWvS/Wchq ce7VSkaLSfcnsFi8fmFo0f/4NbPF2aY37BabHl9jtbi8aw6bxefeI4wWM87vY7JYt/EWu8Xt y7wWL4/8YLRYev0ik8XtxhVsFhOmr2WxOHP6EqtF694j7BZtqz+wOoh4rJm3htGjpbmHzeNy Xy+Tx6079R47Z91l91i5/Aubx6ZVnWwem5fUe/w7xu6x5Wo7i0ffllWMHp83yQXwRHHZpKTm ZJalFunbJXBl3OhrYymYYFIxYVYjWwNjr2YXIyeHhICJxJIjf1ghbDGJC/fWs3UxcnEICaxg lPh18QUbTNGZN+vYIRJLGSUe/JjLDpLgFRCU+DH5HksXIwcHs4C6xJQpuSBhZgERiabOHywQ trbEsoWvmSF6HzBKzFhyiRmiV0vi6tp1jCA2i4CqxLu5s8FsNqD4/hc32EBmigpESHSfqATp FRHYzSgx5/9aJhCHWWACu8Sfu7fABgkLuEjc67kKdpCQgLvE9V87WUGaOQU8JM5uNoV44A6H xIZ9BRC7BCS+TT4EdrOEgKzEpgPMECWSEgdX3GCZwCg+C8lnsxA+m4Xks1lIPlvAyLKKUTS1 ILmgOCm9yFSvODG3uDQvXS85P3cTIzDBnP73bOIOxvsHrA8xCnAwKvHwZlj+CBNiTSwrrsw9 xGgKdNBEZinR5HxgGssriTc0NjOyMDUxNTYytzRTEufVkf4ZLCSQnliSmp2aWpBaFF9UmpNa fIiRiYNTqoHR2zGp4vrv1L18DyL2P1LgjJj2K89Sl+9+z9n2P7N//E7323S3SqW2xX//wqLu OSeD2qZdWsH1NsZibtKlZepaOc26T0OU9CbyZH2PjZe7J22s8XOe1EWfsppji/qXZhY4//7W GWLr1/E0fvHxmtk6uvliO+yf1WrO4xbY9E69YPdJsXyZ23uVWIozEg21mIuKEwFzznwRKwMA AA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrNKsWRmVeSWpSXmKPExsVy+t9jAV2Fqz/CDF6tlrGYf+Qcq0X/m4Ws FuderWS0mHR/AovF6xeGFv2PXzNbnG16w26x6fE1VovLu+awWXzuPcJoMeP8PiaLdRtvsVvc vsxr8fLID0aLpdcvMlncblzBZjFh+loWizOnL7FatO49wm7RtvoDq4OIx5p5axg9Wpp72Dwu 9/Uyedy6U++xc9Zddo+Vy7+weWxa1cnmsXlJvce/Y+weW662s3j0bVnF6PF5k1wAT1QDo01G amJKapFCal5yfkpmXrqtkndwvHO8qZmBoa6hpYW5kkJeYm6qrZKLT4CuW2YO0JtKCmWJOaVA oYDE4mIlfTtME0JD3HQtYBojdH1DguB6jAzQQMIaxozXU9MLLhhX7P51g7WB8axGFyMnh4SA icSZN+vYIWwxiQv31rN1MXJxCAksZZR48GMuWIJXQFDix+R7LF2MHBzMAvISRy5lQ5jqElOm 5EKUP2CUmLHkEjNEuZbE1bXrGEFsFgFViXdzZ4PZbEDx/S9usIH0igpESHSfqATpFRHYzSgx 5/9aJhCHWWACu8Sfu7fABgkLuEjc67kKdoOQgLvE9V87WUGaOQU8JM5uNp3AKDALyXWzEK6b hXDdAkbmVYwSqQXJBcVJ6blGeanlesWJucWleel6yfm5mxjBCemZ9A7Gw7vcDzEKcDAq8fBm WP4IE2JNLCuuzD3EKMHBrCTCu/ESUIg3JbGyKrUoP76oNCe1+BCjKdB7E5mlRJPzgckyryTe 0NjEzMjSyNzQwsjYXEmc9/H/dWFCAumJJanZqakFqUUwfUwcnFINjH1y8746L/mZ3z1T4Fdg /WTHTIe7ou6uszP+tC7peKa35uicN8/3y7vGpDY8P317ceTPG78vTg/lemfZxnQyPndNTpxo 1lXevQnazr1vmZk3s62um53IefWdI29pxuSJ1UFRK9XPCqfM7ZA/6/tZW9LRP7Jhg8Rz25Xr pTm3b4oKOd4kuoB1qxJLcUaioRZzUXEiAFZLwUleAwAA DLP-Filter: Pass X-MTR: 20000000000000000@CPGS X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2016년 03월 28일 12:11, MyungJoo Ham wrote: > [] >> Suggested-by: Myungjoo Ham >> Signed-off-by: Chanwoo Choi >> [tjakobi: Reported RCU locking issue and cw00.choi fix it.] >> Reported-by: Tobias Jakobi >> [m.reichl and linux.amoon: Tested it on exynos4412-odroidu3 board] >> Tested-by: Markus Reichl >> Tested-by: Anand Moon >> --- >> drivers/devfreq/Kconfig | 7 ++ >> drivers/devfreq/Makefile | 1 + >> drivers/devfreq/devfreq.c | 17 ++++ >> drivers/devfreq/governor.h | 15 +++ >> drivers/devfreq/governor_passive.c | 192 +++++++++++++++++++++++++++++++++++++ >> include/linux/devfreq.h | 3 + >> 6 files changed, 235 insertions(+) >> create mode 100644 drivers/devfreq/governor_passive.c >> >> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig > > [] > >> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile >> index 8af8aaf922a8..2633087d5c63 100644 >> --- a/drivers/devfreq/Makefile >> +++ b/drivers/devfreq/Makefile >> @@ -4,6 +4,7 @@ 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 >> obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o >> +obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o >> >> # DEVFREQ Drivers >> obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 1d6c803804d5..9f84bbc2994c 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -478,7 +478,13 @@ static void _remove_devfreq(struct devfreq *devfreq) >> dev_warn(&devfreq->dev, "releasing devfreq which doesn't exist\n"); >> return; >> } >> + >> + if (devfreq->governor->type == DEVFREQ_GOV_PASSIVE) >> + devfreq_passive_unregister_notifier(devfreq); >> + >> list_del(&devfreq->node); >> + list_del_init(&devfreq->passive_node); >> + > > Let's do this inside governor_passive.c, you already have > the DEVFREQ_GOV_STOP signal. This may be called more frequently > because the START-STOP pair has wider usage than add-remove pair. > However, still, it's ok to be detached during the "STOP"ed but not > removed state. I agree the code about passive governor to drivers/devfreq/governor_passive.c. It makes the code clear. > >> mutex_unlock(&devfreq_list_lock); >> >> if (devfreq->governor) >> @@ -598,6 +604,17 @@ struct devfreq *devfreq_add_device(struct device *dev, >> goto err_init; >> } >> >> + if (devfreq->governor->type == DEVFREQ_GOV_PASSIVE) { >> + struct devfreq *parent_devfreq = (struct devfreq *)data; >> + list_add(&devfreq->passive_node, &parent_devfreq->passive_node); >> + >> + err = devfreq_passive_register_notifier(devfreq); >> + if (err <0) >> + goto err_init; >> + } else { >> + INIT_LIST_HEAD(&devfreq->passive_node); >> + } >> + > > Same as above. We can reuse DEVFREQ_GOV_START for this. > With DEVFREQ_GOV_START/STOP, we can entirely remove any modifications > in the devfreq.c, governor.h, devfreq.h. I agree. I'll modify it. > > Besides, such an approach removed the need for the patch 05/21. > We no longer (or yet) need "governor type". > >> return devfreq; >> >> err_init: >> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h >> index cf19b923c362..64d1dffcdb43 100644 >> --- a/drivers/devfreq/governor.h >> +++ b/drivers/devfreq/governor.h > > as mentioned above, we don't need to modify this file. OK. > >> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c >> new file mode 100644 >> index 000000000000..521e93b68c11 >> --- /dev/null >> +++ b/drivers/devfreq/governor_passive.c >> @@ -0,0 +1,192 @@ >> +/* >> + * linux/drivers/devfreq/governor_passive.c >> + * >> + * Copyright (C) 2016 Samsung Electronics >> + * Author: Chanwoo Choi >> + * >> + * 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. >> + */ >> + >> +#include >> +#include >> +#include >> +#include > > Doubly included I'll fix it. > >> +#include "governor.h" >> + > > [] > >> +static int devfreq_passive_event_handler(struct devfreq *devfreq, >> + unsigned int event, void *data) >> +{ >> + return 0; > > Let's handle DEVFREQ_GOV_START/STOP event here. OK. I'll register/unregister the DEVFREQ_TRANSITION_NOTIFIER by using DEVFREQ_GOV_START/STOP event. > >> +} >> + >> +static struct devfreq_governor devfreq_passive = { >> + .name = "passive", >> + .type = DEVFREQ_GOV_PASSIVE, > > Let's not use .type enum, yet. We don't this this. At least for now. OK. I'll remove it. > >> + .get_target_freq = devfreq_passive_get_target_freq, >> + .event_handler = devfreq_passive_event_handler, >> +}; >> + > > [] > >> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h When the devfreq device using passive governor registers the notifier to DEVFREQ_TRANSITION_NOTIFIER, the devfreq device need the one notifier block field as following: struct devfreq { ... ... struct notifier_block passive_nb; } static int devfreq_passive_event_handler(struct devfreq *devfreq, unsigned int event, void *data) { struct device *dev = devfreq->dev.parent; struct devfreq *parent = (struct devfreq *)devfreq->data; struct notifier_block *nb = &devfreq->passive_nb; int ret = 0; if (!parent) return -EPROBE_DEFER; switch (event) { case DEVFREQ_GOV_START: nb->notifier_call = devfreq_passive_notifier_call; ret = devm_devfreq_register_notifier(dev, parent, nb, DEVFREQ_TRANSITION_NOTIFIER); break; case DEVFREQ_GOV_STOP: devm_devfreq_unregister_notifier(dev, parent, nb, DEVFREQ_TRANSITION_NOTIFIER); break; default: break; } return ret; } Best Regards, Chanwoo Choi