| Message ID | 20190214013042.254790-4-mka@chromium.org |
|---|---|
| State | Deferred |
| Headers | show |
| Series | PM / devfreq: Refactor load monitoring | expand |
Hi Matthias,
Looks good to me for making the function to remove the duplicate code.
But, When I just tested the kernel build, following warnings occur
about devfreq_governor_stop().
In file included from ./include/linux/devfreq.h:16:0,
from drivers/devfreq/devfreq.c:23:
drivers/devfreq/devfreq.c: In function ‘devfreq_governor_stop’:
drivers/devfreq/devfreq.c:619:17: warning: format ‘%s’ expects
argument of type ‘char *’, but argument 4 has type ‘int’ [-Wformat=]
dev_warn(dev, "%s: Governor %s not stopped: %d\n",
^
./include/linux/device.h:1380:22: note: in definition of macro ‘dev_fmt’
#define dev_fmt(fmt) fmt
^
drivers/devfreq/devfreq.c:619:3: note: in expansion of macro ‘dev_warn’
dev_warn(dev, "%s: Governor %s not stopped: %d\n",
^
drivers/devfreq/devfreq.c:619:17: warning: format ‘%d’ expects a
matching ‘int’ argument [-Wformat=]
dev_warn(dev, "%s: Governor %s not stopped: %d\n",
^
./include/linux/device.h:1380:22: note: in definition of macro ‘dev_fmt’
#define dev_fmt(fmt) fmt
^
drivers/devfreq/devfreq.c:619:3: note: in expansion of macro ‘dev_warn’
dev_warn(dev, "%s: Governor %s not stopped: %d\n",
2019년 2월 14일 (목) 오후 7:16, Matthias Kaehlcke <mka@chromium.org>님이 작성:
>
> The following pattern is repeated multiple times:
>
> - call governor event handler with DEVFREQ_GOV_START/STOP
> - check return value
> - in case of error log a message
>
> Add devfreq_governor_start/stop() helper functions with this code
> and call them instead.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> drivers/devfreq/devfreq.c | 96 +++++++++++++++++++++++----------------
> 1 file changed, 58 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 7fab6c4cf719b..eb86461648d74 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -580,6 +580,53 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
> return ret;
> }
>
> +/**
> + * devfreq_governor_start() - Start the devfreq governor of the device.
> + * @devfreq: the devfreq instance
> + */
> +static int devfreq_governor_start(struct devfreq *devfreq)
> +{
> + struct device *dev = devfreq->dev.parent;
> + int err;
> +
> + if (WARN(mutex_is_locked(&devfreq->lock),
> + "mutex must *not* be held by the caller\n"))
> + return -EINVAL;
> +
> + err = devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_START,
> + NULL);
> + if (err) {
> + dev_err(dev, "Governor %s not started: %d\n",
> + devfreq->governor->name, err);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * devfreq_governor_stop() - Stop the devfreq governor of the device.
> + * @devfreq: the devfreq instance
> + */
> +static int devfreq_governor_stop(struct devfreq *devfreq)
> +{
> + struct device *dev = devfreq->dev.parent;
> + int err;
> +
> + if (WARN(mutex_is_locked(&devfreq->lock),
> + "mutex must *not* be held by the caller\n"))
> + return -EINVAL;
> +
> + err = devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_STOP, NULL);
> + if (err) {
> + dev_warn(dev, "%s: Governor %s not stopped: %d\n",
> + devfreq->governor->name, err);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> /**
> * devfreq_dev_release() - Callback for struct device to release the device.
> * @dev: the devfreq device
> @@ -720,13 +767,9 @@ struct devfreq *devfreq_add_device(struct device *dev,
> }
>
> devfreq->governor = governor;
> - err = devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_START,
> - NULL);
> - if (err) {
> - dev_err(dev, "%s: Unable to start governor for the device\n",
> - __func__);
> + err = devfreq_governor_start(devfreq);
> + if (err)
> goto err_init;
> - }
>
> list_add(&devfreq->node, &devfreq_list);
>
> @@ -1029,24 +1072,13 @@ int devfreq_add_governor(struct devfreq_governor *governor)
> dev_warn(dev,
> "%s: Governor %s already present\n",
> __func__, devfreq->governor->name);
> - ret = devfreq->governor->event_handler(devfreq,
> - DEVFREQ_GOV_STOP, NULL);
> - if (ret) {
> - dev_warn(dev,
> - "%s: Governor %s stop = %d\n",
> - __func__,
> - devfreq->governor->name, ret);
> - }
> + ret = devfreq_governor_stop(devfreq);
> +
> /* Fall through */
> }
> +
> devfreq->governor = governor;
> - ret = devfreq->governor->event_handler(devfreq,
> - DEVFREQ_GOV_START, NULL);
> - if (ret) {
> - dev_warn(dev, "%s: Governor %s start=%d\n",
> - __func__, devfreq->governor->name,
> - ret);
> - }
> + devfreq_governor_start(devfreq);
> }
> }
>
> @@ -1081,7 +1113,6 @@ int devfreq_remove_governor(struct devfreq_governor *governor)
> goto err_out;
> }
> list_for_each_entry(devfreq, &devfreq_list, node) {
> - int ret;
> struct device *dev = devfreq->dev.parent;
>
> if (!strncmp(devfreq->governor_name, governor->name,
> @@ -1093,13 +1124,8 @@ int devfreq_remove_governor(struct devfreq_governor *governor)
> continue;
> /* Fall through */
> }
> - ret = devfreq->governor->event_handler(devfreq,
> - DEVFREQ_GOV_STOP, NULL);
> - if (ret) {
> - dev_warn(dev, "%s: Governor %s stop=%d\n",
> - __func__, devfreq->governor->name,
> - ret);
> - }
> +
> + devfreq_governor_stop(devfreq);
> devfreq->governor = NULL;
> }
> }
> @@ -1149,19 +1175,13 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
> }
>
> if (df->governor) {
> - ret = df->governor->event_handler(df, DEVFREQ_GOV_STOP, NULL);
> - if (ret) {
> - dev_warn(dev, "%s: Governor %s not stopped(%d)\n",
> - __func__, df->governor->name, ret);
> + ret = devfreq_governor_stop(df);
> + if (ret)
> goto out;
> - }
> }
> df->governor = governor;
> strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
> - ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
> - if (ret)
> - dev_warn(dev, "%s: Governor %s not started(%d)\n",
> - __func__, df->governor->name, ret);
> + ret = devfreq_governor_start(df);
> out:
> mutex_unlock(&devfreq_list_lock);
>
> --
> 2.20.1.791.gb4d0f1c61a-goog
>
Hi Matthias, When I contributed the something to devfreq.c, if the newly added functions are internal/static, just added the function without 'devfreq_' prefix in order to distinguish them from the exported function as following: - find_available_min_freq() - find_available_max_freq() - set_freq_table() So, the governor_start/stop are the static function used only in devfreq.c, in order to sustain the consistency of function naming, I recommened that changes them as following: - devfreq_governor_start -> governor_start - devfreq_governor_stop -> governor_stop 2019년 2월 14일 (목) 오후 11:12, Chanwoo Choi <cwchoi00@gmail.com>님이 작성: > > Hi Matthias, > > Looks good to me for making the function to remove the duplicate code. > But, When I just tested the kernel build, following warnings occur > about devfreq_governor_stop(). > > In file included from ./include/linux/devfreq.h:16:0, > from drivers/devfreq/devfreq.c:23: > drivers/devfreq/devfreq.c: In function ‘devfreq_governor_stop’: > drivers/devfreq/devfreq.c:619:17: warning: format ‘%s’ expects > argument of type ‘char *’, but argument 4 has type ‘int’ [-Wformat=] > dev_warn(dev, "%s: Governor %s not stopped: %d\n", > ^ > ./include/linux/device.h:1380:22: note: in definition of macro ‘dev_fmt’ > #define dev_fmt(fmt) fmt > ^ > drivers/devfreq/devfreq.c:619:3: note: in expansion of macro ‘dev_warn’ > dev_warn(dev, "%s: Governor %s not stopped: %d\n", > ^ > drivers/devfreq/devfreq.c:619:17: warning: format ‘%d’ expects a > matching ‘int’ argument [-Wformat=] > dev_warn(dev, "%s: Governor %s not stopped: %d\n", > ^ > ./include/linux/device.h:1380:22: note: in definition of macro ‘dev_fmt’ > #define dev_fmt(fmt) fmt > ^ > drivers/devfreq/devfreq.c:619:3: note: in expansion of macro ‘dev_warn’ > dev_warn(dev, "%s: Governor %s not stopped: %d\n", > > 2019년 2월 14일 (목) 오후 7:16, Matthias Kaehlcke <mka@chromium.org>님이 작성: > > > > The following pattern is repeated multiple times: > > > > - call governor event handler with DEVFREQ_GOV_START/STOP > > - check return value > > - in case of error log a message > > > > Add devfreq_governor_start/stop() helper functions with this code > > and call them instead. > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > drivers/devfreq/devfreq.c | 96 +++++++++++++++++++++++---------------- > > 1 file changed, 58 insertions(+), 38 deletions(-) > > > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > > index 7fab6c4cf719b..eb86461648d74 100644 > > --- a/drivers/devfreq/devfreq.c > > +++ b/drivers/devfreq/devfreq.c > > @@ -580,6 +580,53 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, > > return ret; > > } > > > > +/** > > + * devfreq_governor_start() - Start the devfreq governor of the device. > > + * @devfreq: the devfreq instance > > + */ > > +static int devfreq_governor_start(struct devfreq *devfreq) > > +{ > > + struct device *dev = devfreq->dev.parent; > > + int err; > > + > > + if (WARN(mutex_is_locked(&devfreq->lock), > > + "mutex must *not* be held by the caller\n")) > > + return -EINVAL; > > + > > + err = devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_START, > > + NULL); > > + if (err) { > > + dev_err(dev, "Governor %s not started: %d\n", > > + devfreq->governor->name, err); > > + return err; > > + } > > + > > + return 0; > > +} > > + > > +/** > > + * devfreq_governor_stop() - Stop the devfreq governor of the device. > > + * @devfreq: the devfreq instance > > + */ > > +static int devfreq_governor_stop(struct devfreq *devfreq) > > +{ > > + struct device *dev = devfreq->dev.parent; > > + int err; > > + > > + if (WARN(mutex_is_locked(&devfreq->lock), > > + "mutex must *not* be held by the caller\n")) > > + return -EINVAL; > > + > > + err = devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_STOP, NULL); > > + if (err) { > > + dev_warn(dev, "%s: Governor %s not stopped: %d\n", > > + devfreq->governor->name, err); > > + return err; > > + } > > + > > + return 0; > > +} > > + > > /** > > * devfreq_dev_release() - Callback for struct device to release the device. > > * @dev: the devfreq device > > @@ -720,13 +767,9 @@ struct devfreq *devfreq_add_device(struct device *dev, > > } > > > > devfreq->governor = governor; > > - err = devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_START, > > - NULL); > > - if (err) { > > - dev_err(dev, "%s: Unable to start governor for the device\n", > > - __func__); > > + err = devfreq_governor_start(devfreq); > > + if (err) > > goto err_init; > > - } > > > > list_add(&devfreq->node, &devfreq_list); > > > > @@ -1029,24 +1072,13 @@ int devfreq_add_governor(struct devfreq_governor *governor) > > dev_warn(dev, > > "%s: Governor %s already present\n", > > __func__, devfreq->governor->name); > > - ret = devfreq->governor->event_handler(devfreq, > > - DEVFREQ_GOV_STOP, NULL); > > - if (ret) { > > - dev_warn(dev, > > - "%s: Governor %s stop = %d\n", > > - __func__, > > - devfreq->governor->name, ret); > > - } > > + ret = devfreq_governor_stop(devfreq); > > + > > /* Fall through */ > > } > > + > > devfreq->governor = governor; > > - ret = devfreq->governor->event_handler(devfreq, > > - DEVFREQ_GOV_START, NULL); > > - if (ret) { > > - dev_warn(dev, "%s: Governor %s start=%d\n", > > - __func__, devfreq->governor->name, > > - ret); > > - } > > + devfreq_governor_start(devfreq); > > } > > } > > > > @@ -1081,7 +1113,6 @@ int devfreq_remove_governor(struct devfreq_governor *governor) > > goto err_out; > > } > > list_for_each_entry(devfreq, &devfreq_list, node) { > > - int ret; > > struct device *dev = devfreq->dev.parent; > > > > if (!strncmp(devfreq->governor_name, governor->name, > > @@ -1093,13 +1124,8 @@ int devfreq_remove_governor(struct devfreq_governor *governor) > > continue; > > /* Fall through */ > > } > > - ret = devfreq->governor->event_handler(devfreq, > > - DEVFREQ_GOV_STOP, NULL); > > - if (ret) { > > - dev_warn(dev, "%s: Governor %s stop=%d\n", > > - __func__, devfreq->governor->name, > > - ret); > > - } > > + > > + devfreq_governor_stop(devfreq); > > devfreq->governor = NULL; > > } > > } > > @@ -1149,19 +1175,13 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr, > > } > > > > if (df->governor) { > > - ret = df->governor->event_handler(df, DEVFREQ_GOV_STOP, NULL); > > - if (ret) { > > - dev_warn(dev, "%s: Governor %s not stopped(%d)\n", > > - __func__, df->governor->name, ret); > > + ret = devfreq_governor_stop(df); > > + if (ret) > > goto out; > > - } > > } > > df->governor = governor; > > strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN); > > - ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL); > > - if (ret) > > - dev_warn(dev, "%s: Governor %s not started(%d)\n", > > - __func__, df->governor->name, ret); > > + ret = devfreq_governor_start(df); > > out: > > mutex_unlock(&devfreq_list_lock); > > > > -- > > 2.20.1.791.gb4d0f1c61a-goog > > > > > -- > Best Regards, > Chanwoo Choi
On Thu, Feb 14, 2019 at 11:12:55PM +0900, Chanwoo Choi wrote: > Hi Matthias, > > Looks good to me for making the function to remove the duplicate code. > But, When I just tested the kernel build, following warnings occur > about devfreq_governor_stop(). > > In file included from ./include/linux/devfreq.h:16:0, > from drivers/devfreq/devfreq.c:23: > drivers/devfreq/devfreq.c: In function ‘devfreq_governor_stop’: > drivers/devfreq/devfreq.c:619:17: warning: format ‘%s’ expects > argument of type ‘char *’, but argument 4 has type ‘int’ [-Wformat=] > dev_warn(dev, "%s: Governor %s not stopped: %d\n", > ^ > ./include/linux/device.h:1380:22: note: in definition of macro ‘dev_fmt’ > #define dev_fmt(fmt) fmt > ^ > drivers/devfreq/devfreq.c:619:3: note: in expansion of macro ‘dev_warn’ > dev_warn(dev, "%s: Governor %s not stopped: %d\n", > ^ > drivers/devfreq/devfreq.c:619:17: warning: format ‘%d’ expects a > matching ‘int’ argument [-Wformat=] > dev_warn(dev, "%s: Governor %s not stopped: %d\n", > ^ > ./include/linux/device.h:1380:22: note: in definition of macro ‘dev_fmt’ > #define dev_fmt(fmt) fmt > ^ > drivers/devfreq/devfreq.c:619:3: note: in expansion of macro ‘dev_warn’ > dev_warn(dev, "%s: Governor %s not stopped: %d\n", For some reason the warnings don't pop up in my 4.19 build and I missed them when compile testing upstream :( I'll fix the format string in the next version. Thanks for the review and build test! Matthias
Hi Chanwoo, On Thu, Feb 14, 2019 at 11:32:40PM +0900, Chanwoo Choi wrote: > Hi Matthias, > > When I contributed the something to devfreq.c, if the newly added functions > are internal/static, just added the function without 'devfreq_' prefix > in order to distinguish them from the exported function as following: > - find_available_min_freq() > - find_available_max_freq() > - set_freq_table() > > So, the governor_start/stop are the static function used only in devfreq.c, > in order to sustain the consistency of function naming, I recommened > that changes them as following: > - devfreq_governor_start -> governor_start > - devfreq_governor_stop -> governor_stop Sounds good, I'll update this in the next version. Thanks Matthias
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 7fab6c4cf719b..eb86461648d74 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -580,6 +580,53 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, return ret; } +/** + * devfreq_governor_start() - Start the devfreq governor of the device. + * @devfreq: the devfreq instance + */ +static int devfreq_governor_start(struct devfreq *devfreq) +{ + struct device *dev = devfreq->dev.parent; + int err; + + if (WARN(mutex_is_locked(&devfreq->lock), + "mutex must *not* be held by the caller\n")) + return -EINVAL; + + err = devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_START, + NULL); + if (err) { + dev_err(dev, "Governor %s not started: %d\n", + devfreq->governor->name, err); + return err; + } + + return 0; +} + +/** + * devfreq_governor_stop() - Stop the devfreq governor of the device. + * @devfreq: the devfreq instance + */ +static int devfreq_governor_stop(struct devfreq *devfreq) +{ + struct device *dev = devfreq->dev.parent; + int err; + + if (WARN(mutex_is_locked(&devfreq->lock), + "mutex must *not* be held by the caller\n")) + return -EINVAL; + + err = devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_STOP, NULL); + if (err) { + dev_warn(dev, "%s: Governor %s not stopped: %d\n", + devfreq->governor->name, err); + return err; + } + + return 0; +} + /** * devfreq_dev_release() - Callback for struct device to release the device. * @dev: the devfreq device @@ -720,13 +767,9 @@ struct devfreq *devfreq_add_device(struct device *dev, } devfreq->governor = governor; - err = devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_START, - NULL); - if (err) { - dev_err(dev, "%s: Unable to start governor for the device\n", - __func__); + err = devfreq_governor_start(devfreq); + if (err) goto err_init; - } list_add(&devfreq->node, &devfreq_list); @@ -1029,24 +1072,13 @@ int devfreq_add_governor(struct devfreq_governor *governor) dev_warn(dev, "%s: Governor %s already present\n", __func__, devfreq->governor->name); - ret = devfreq->governor->event_handler(devfreq, - DEVFREQ_GOV_STOP, NULL); - if (ret) { - dev_warn(dev, - "%s: Governor %s stop = %d\n", - __func__, - devfreq->governor->name, ret); - } + ret = devfreq_governor_stop(devfreq); + /* Fall through */ } + devfreq->governor = governor; - ret = devfreq->governor->event_handler(devfreq, - DEVFREQ_GOV_START, NULL); - if (ret) { - dev_warn(dev, "%s: Governor %s start=%d\n", - __func__, devfreq->governor->name, - ret); - } + devfreq_governor_start(devfreq); } } @@ -1081,7 +1113,6 @@ int devfreq_remove_governor(struct devfreq_governor *governor) goto err_out; } list_for_each_entry(devfreq, &devfreq_list, node) { - int ret; struct device *dev = devfreq->dev.parent; if (!strncmp(devfreq->governor_name, governor->name, @@ -1093,13 +1124,8 @@ int devfreq_remove_governor(struct devfreq_governor *governor) continue; /* Fall through */ } - ret = devfreq->governor->event_handler(devfreq, - DEVFREQ_GOV_STOP, NULL); - if (ret) { - dev_warn(dev, "%s: Governor %s stop=%d\n", - __func__, devfreq->governor->name, - ret); - } + + devfreq_governor_stop(devfreq); devfreq->governor = NULL; } } @@ -1149,19 +1175,13 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr, } if (df->governor) { - ret = df->governor->event_handler(df, DEVFREQ_GOV_STOP, NULL); - if (ret) { - dev_warn(dev, "%s: Governor %s not stopped(%d)\n", - __func__, df->governor->name, ret); + ret = devfreq_governor_stop(df); + if (ret) goto out; - } } df->governor = governor; strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN); - ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL); - if (ret) - dev_warn(dev, "%s: Governor %s not started(%d)\n", - __func__, df->governor->name, ret); + ret = devfreq_governor_start(df); out: mutex_unlock(&devfreq_list_lock);
The following pattern is repeated multiple times: - call governor event handler with DEVFREQ_GOV_START/STOP - check return value - in case of error log a message Add devfreq_governor_start/stop() helper functions with this code and call them instead. Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- drivers/devfreq/devfreq.c | 96 +++++++++++++++++++++++---------------- 1 file changed, 58 insertions(+), 38 deletions(-)