| Message ID | 58fdd2c226a4e76a3d9427baab7dd5c23af842ab.1569319738.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PM / devfreq: Add dev_pm_qos support | expand |
On Tue, Sep 24, 2019 at 01:11:29PM +0300, Leonard Crestez wrote: > Register notifiers with the PM QoS framework in order to respond to > requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY. > > No notifiers are added by this patch but PM QoS constraints can be > imposed externally (for example from other devices). > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > --- > drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++ > include/linux/devfreq.h | 5 +++ > 2 files changed, 80 insertions(+) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index eee403e70c84..784f3e40536a 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -22,15 +22,18 @@ > #include <linux/platform_device.h> > #include <linux/list.h> > #include <linux/printk.h> > #include <linux/hrtimer.h> > #include <linux/of.h> > +#include <linux/pm_qos.h> > #include "governor.h" > > #define CREATE_TRACE_POINTS > #include <trace/events/devfreq.h> > > +#define HZ_PER_KHZ 1000 > + > static struct class *devfreq_class; > > /* > * devfreq core provides delayed work based load monitoring helper > * functions. Governors can use these or can implement their own > @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) > static void get_freq_range(struct devfreq *devfreq, > unsigned long *min_freq, > unsigned long *max_freq) > { > unsigned long *freq_table = devfreq->profile->freq_table; > + unsigned long qos_min_freq, qos_max_freq; > > lockdep_assert_held(&devfreq->lock); > > /* > * Init min/max frequency from freq table. > @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq, > } else { > *min_freq = freq_table[devfreq->profile->max_state - 1]; > *max_freq = freq_table[0]; > } > > + /* constraints from PM QoS */ > + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent, > + DEV_PM_QOS_MIN_FREQUENCY); > + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, > + DEV_PM_QOS_MIN_FREQUENCY); This needs to be DEV_PM_QOS_MAX_FREQUENCY. I missed this in the earlier reviews, but stumbled across it when testing. !Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
On 24.09.2019 22:11, Matthias Kaehlcke wrote: > On Tue, Sep 24, 2019 at 01:11:29PM +0300, Leonard Crestez wrote: >> Register notifiers with the PM QoS framework in order to respond to >> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY. >> >> No notifiers are added by this patch but PM QoS constraints can be >> imposed externally (for example from other devices). >> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >> --- >> drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++ >> include/linux/devfreq.h | 5 +++ >> 2 files changed, 80 insertions(+) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index eee403e70c84..784f3e40536a 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -22,15 +22,18 @@ >> #include <linux/platform_device.h> >> #include <linux/list.h> >> #include <linux/printk.h> >> #include <linux/hrtimer.h> >> #include <linux/of.h> >> +#include <linux/pm_qos.h> >> #include "governor.h" >> >> #define CREATE_TRACE_POINTS >> #include <trace/events/devfreq.h> >> >> +#define HZ_PER_KHZ 1000 >> + >> static struct class *devfreq_class; >> >> /* >> * devfreq core provides delayed work based load monitoring helper >> * functions. Governors can use these or can implement their own >> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) >> static void get_freq_range(struct devfreq *devfreq, >> unsigned long *min_freq, >> unsigned long *max_freq) >> { >> unsigned long *freq_table = devfreq->profile->freq_table; >> + unsigned long qos_min_freq, qos_max_freq; >> >> lockdep_assert_held(&devfreq->lock); >> >> /* >> * Init min/max frequency from freq table. >> @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq, >> } else { >> *min_freq = freq_table[devfreq->profile->max_state - 1]; >> *max_freq = freq_table[0]; >> } >> >> + /* constraints from PM QoS */ >> + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent, >> + DEV_PM_QOS_MIN_FREQUENCY); >> + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, >> + DEV_PM_QOS_MIN_FREQUENCY); > > This needs to be DEV_PM_QOS_MAX_FREQUENCY. I missed this in the earlier > reviews, but stumbled across it when testing. > > !Reviewed-by: Matthias Kaehlcke <mka@chromium.org> I broke it in v8 while processing comments. Thanks for catching it. -- Regards, Leonard
On 19. 9. 24. 오후 7:11, Leonard Crestez wrote: > Register notifiers with the PM QoS framework in order to respond to > requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY. > > No notifiers are added by this patch but PM QoS constraints can be > imposed externally (for example from other devices). > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > --- > drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++ > include/linux/devfreq.h | 5 +++ > 2 files changed, 80 insertions(+) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index eee403e70c84..784f3e40536a 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -22,15 +22,18 @@ > #include <linux/platform_device.h> > #include <linux/list.h> > #include <linux/printk.h> > #include <linux/hrtimer.h> > #include <linux/of.h> > +#include <linux/pm_qos.h> > #include "governor.h" > > #define CREATE_TRACE_POINTS > #include <trace/events/devfreq.h> > > +#define HZ_PER_KHZ 1000 > + > static struct class *devfreq_class; > > /* > * devfreq core provides delayed work based load monitoring helper > * functions. Governors can use these or can implement their own > @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) > static void get_freq_range(struct devfreq *devfreq, > unsigned long *min_freq, > unsigned long *max_freq) > { > unsigned long *freq_table = devfreq->profile->freq_table; > + unsigned long qos_min_freq, qos_max_freq; > > lockdep_assert_held(&devfreq->lock); > > /* > * Init min/max frequency from freq table. > @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq, > } else { > *min_freq = freq_table[devfreq->profile->max_state - 1]; > *max_freq = freq_table[0]; > } > > + /* constraints from PM QoS */ As I commented on patch4, 'constraints' -> 'Constraint' because first verb have to be used as the sigular verbs. I prefer to use following comments: /* Constraint minimum/maximum frequency from PM QoS constraints */ > + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent, > + DEV_PM_QOS_MIN_FREQUENCY); > + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, > + DEV_PM_QOS_MIN_FREQUENCY); > + *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); > + *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); > + > /* constraints from sysfs */ > *min_freq = max(*min_freq, devfreq->min_freq); > *max_freq = min(*max_freq, devfreq->max_freq); > > /* constraints from OPP interface */ > @@ -606,10 +618,49 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, > mutex_unlock(&devfreq->lock); > > return ret; > } > > +/** > + * qos_notifier_call() - Common handler for QoS constraints. > + * @devfreq: the devfreq instance. > + */ > +static int qos_notifier_call(struct devfreq *devfreq) > +{ > + int err; > + > + mutex_lock(&devfreq->lock); > + err = update_devfreq(devfreq); > + mutex_unlock(&devfreq->lock); > + if (err) > + dev_err(devfreq->dev.parent, > + "failed to update frequency for PM QoS constraints (%d)\n", Is it not over 80 char? > + err); > + > + return NOTIFY_OK; > +} > + > +/** > + * qos_min_notifier_call() - Callback for QoS min_freq changes. > + * @nb: Should be devfreq->nb_min > + */ > +static int qos_min_notifier_call(struct notifier_block *nb, > + unsigned long val, void *ptr) > +{ > + return qos_notifier_call(container_of(nb, struct devfreq, nb_min)); > +} > + > +/** > + * qos_max_notifier_call() - Callback for QoS max_freq changes. > + * @nb: Should be devfreq->nb_max > + */ > +static int qos_max_notifier_call(struct notifier_block *nb, > + unsigned long val, void *ptr) > +{ > + return qos_notifier_call(container_of(nb, struct devfreq, nb_max)); > +} > + > /** > * devfreq_dev_release() - Callback for struct device to release the device. > * @dev: the devfreq device > * > * Remove devfreq from the list and release its resources. > @@ -620,10 +671,15 @@ static void devfreq_dev_release(struct device *dev) > > mutex_lock(&devfreq_list_lock); > list_del(&devfreq->node); > mutex_unlock(&devfreq_list_lock); > > + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max, > + DEV_PM_QOS_MAX_FREQUENCY); > + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min, > + DEV_PM_QOS_MIN_FREQUENCY); > + Just print error with dev_err() without stopping the release step. I prefer to handle the return value if kernel API provides the error code. > if (devfreq->profile->exit) > devfreq->profile->exit(devfreq->dev.parent); > > kfree(devfreq->time_in_state); > kfree(devfreq->trans_table); > @@ -733,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev, > if (err) { > put_device(&devfreq->dev); > goto err_out; > } > > + /* > + * Register notifiers for updates to min/max_freq after device is > + * initialized (and we can handle notifications) but before the > + * governor is started (which should do an initial enforcement of > + * constraints). > + */ My previous comment is not enough why I prefer to remove it. Sorry. Actually, until now, the devfreq_add_device() don't have the detailed comments because the line code is not too long. But, at the present time, devfreq_add_device() is too long. It means that the detailed comment are necessary. So, I'll add the detailed comment for each step of devfreq_add_device() on separate patch to keep the same style. I'll send the patch to you for the review. > + devfreq->nb_min.notifier_call = qos_min_notifier_call; > + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min, > + DEV_PM_QOS_MIN_FREQUENCY); > + if (err) > + goto err_devfreq; > + > + devfreq->nb_max.notifier_call = qos_max_notifier_call; > + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max, > + DEV_PM_QOS_MAX_FREQUENCY); > + if (err) > + goto err_devfreq; > + > mutex_lock(&devfreq_list_lock); > > governor = try_then_request_governor(devfreq->governor_name); > if (IS_ERR(governor)) { > dev_err(dev, "%s: Unable to find governor for the device\n", > @@ -760,10 +834,11 @@ struct devfreq *devfreq_add_device(struct device *dev, > > return devfreq; > > err_init: > mutex_unlock(&devfreq_list_lock); > +err_devfreq: > devfreq_remove_device(devfreq); > return ERR_PTR(err); > > err_dev: > /* > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h > index c3cbc15fdf08..dac0dffeabb4 100644 > --- a/include/linux/devfreq.h > +++ b/include/linux/devfreq.h > @@ -134,10 +134,12 @@ struct devfreq_dev_profile { > * @total_trans: Number of devfreq transitions > * @trans_table: Statistics of devfreq transitions > * @time_in_state: Statistics of devfreq states > * @last_stat_updated: The last time stat updated > * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier > + * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY > + * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY > * > * This structure stores the devfreq information for a give device. > * > * Note that when a governor accesses entries in struct devfreq in its > * functions except for the context of callbacks defined in struct > @@ -176,10 +178,13 @@ struct devfreq { > unsigned int *trans_table; > unsigned long *time_in_state; > unsigned long last_stat_updated; > > struct srcu_notifier_head transition_notifier_list; > + > + struct notifier_block nb_min; > + struct notifier_block nb_max; > }; > > struct devfreq_freqs { > unsigned long old; > unsigned long new; >
On 19. 9. 25. 오전 4:22, Leonard Crestez wrote: > On 24.09.2019 22:11, Matthias Kaehlcke wrote: >> On Tue, Sep 24, 2019 at 01:11:29PM +0300, Leonard Crestez wrote: >>> Register notifiers with the PM QoS framework in order to respond to >>> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY. >>> >>> No notifiers are added by this patch but PM QoS constraints can be >>> imposed externally (for example from other devices). >>> >>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >>> --- >>> drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++ >>> include/linux/devfreq.h | 5 +++ >>> 2 files changed, 80 insertions(+) >>> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>> index eee403e70c84..784f3e40536a 100644 >>> --- a/drivers/devfreq/devfreq.c >>> +++ b/drivers/devfreq/devfreq.c >>> @@ -22,15 +22,18 @@ >>> #include <linux/platform_device.h> >>> #include <linux/list.h> >>> #include <linux/printk.h> >>> #include <linux/hrtimer.h> >>> #include <linux/of.h> >>> +#include <linux/pm_qos.h> >>> #include "governor.h" >>> >>> #define CREATE_TRACE_POINTS >>> #include <trace/events/devfreq.h> >>> >>> +#define HZ_PER_KHZ 1000 >>> + >>> static struct class *devfreq_class; >>> >>> /* >>> * devfreq core provides delayed work based load monitoring helper >>> * functions. Governors can use these or can implement their own >>> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) >>> static void get_freq_range(struct devfreq *devfreq, >>> unsigned long *min_freq, >>> unsigned long *max_freq) >>> { >>> unsigned long *freq_table = devfreq->profile->freq_table; >>> + unsigned long qos_min_freq, qos_max_freq; >>> >>> lockdep_assert_held(&devfreq->lock); >>> >>> /* >>> * Init min/max frequency from freq table. >>> @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq, >>> } else { >>> *min_freq = freq_table[devfreq->profile->max_state - 1]; >>> *max_freq = freq_table[0]; >>> } >>> >>> + /* constraints from PM QoS */ >>> + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>> + DEV_PM_QOS_MIN_FREQUENCY); >>> + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>> + DEV_PM_QOS_MIN_FREQUENCY); > >> This needs to be DEV_PM_QOS_MAX_FREQUENCY. I missed this in the earlier >> reviews, but stumbled across it when testing. >> >> !Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > > I broke it in v8 while processing comments. Thanks for catching it. Did you test this patchset with v8? Maybe it is not working with this mistake. > > -- > Regards, > Leonard > >
On Wed, Sep 25, 2019 at 11:15:36AM +0900, Chanwoo Choi wrote: > On 19. 9. 24. 오후 7:11, Leonard Crestez wrote: > > Register notifiers with the PM QoS framework in order to respond to > > requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY. > > > > No notifiers are added by this patch but PM QoS constraints can be > > imposed externally (for example from other devices). > > > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++ > > include/linux/devfreq.h | 5 +++ > > 2 files changed, 80 insertions(+) > > > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > > index eee403e70c84..784f3e40536a 100644 > > --- a/drivers/devfreq/devfreq.c > > +++ b/drivers/devfreq/devfreq.c > > @@ -22,15 +22,18 @@ > > #include <linux/platform_device.h> > > #include <linux/list.h> > > #include <linux/printk.h> > > #include <linux/hrtimer.h> > > #include <linux/of.h> > > +#include <linux/pm_qos.h> > > #include "governor.h" > > > > #define CREATE_TRACE_POINTS > > #include <trace/events/devfreq.h> > > > > +#define HZ_PER_KHZ 1000 > > + > > static struct class *devfreq_class; > > > > /* > > * devfreq core provides delayed work based load monitoring helper > > * functions. Governors can use these or can implement their own > > @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) > > static void get_freq_range(struct devfreq *devfreq, > > unsigned long *min_freq, > > unsigned long *max_freq) > > { > > unsigned long *freq_table = devfreq->profile->freq_table; > > + unsigned long qos_min_freq, qos_max_freq; > > > > lockdep_assert_held(&devfreq->lock); > > > > /* > > * Init min/max frequency from freq table. > > @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq, > > } else { > > *min_freq = freq_table[devfreq->profile->max_state - 1]; > > *max_freq = freq_table[0]; > > } > > > > + /* constraints from PM QoS */ > > As I commented on patch4, > 'constraints' -> 'Constraint' because first verb have to be used > as the sigular verbs. 'constraints' is not a verb in this case, but the plural of the noun constraint > I prefer to use following comments: > > /* Constraint minimum/maximum frequency from PM QoS constraints */ I'm not convinced that this is really an improvement. First there is the repeated use of 'Constraint ... constraints'. An improvement could be to use 'Limit' instead of 'Constraint'. Anyway from the code it is pretty obvious that the min/max frequencies are limited. Are you suggesting to also say further below "Limit minimum/maximum frequency from sysfs constraints" and the same for the OPP ones? If you really want a verb the comments could be something like "Apply <interface> constraints" or "Apply constraints from <interface>". IMO the current comments provide enough information, unnecessarily verbose comments can rather get in the way of code readability. > > + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent, > > + DEV_PM_QOS_MIN_FREQUENCY); > > + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, > > + DEV_PM_QOS_MIN_FREQUENCY); > > + *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); > > + *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); > > + > > /* constraints from sysfs */ > > *min_freq = max(*min_freq, devfreq->min_freq); > > *max_freq = min(*max_freq, devfreq->max_freq); > > > > /* constraints from OPP interface */ > > @@ -606,10 +618,49 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, > > mutex_unlock(&devfreq->lock); > > > > return ret; > > } > > > > +/** > > + * qos_notifier_call() - Common handler for QoS constraints. > > + * @devfreq: the devfreq instance. > > + */ > > +static int qos_notifier_call(struct devfreq *devfreq) > > +{ > > + int err; > > + > > + mutex_lock(&devfreq->lock); > > + err = update_devfreq(devfreq); > > + mutex_unlock(&devfreq->lock); > > + if (err) > > + dev_err(devfreq->dev.parent, > > + "failed to update frequency for PM QoS constraints (%d)\n", > > Is it not over 80 char? It is and Leonard split it up initially, however I asked him to not break it up because: 2) Breaking long lines and strings However, *never* break user-visible strings such as printk messages, because that breaks the ability to grep for them. Documentation/process/coding-style.rst Line length could be reduced a bit by adjusting indentation though. > > > + err); > > + > > + return NOTIFY_OK; > > +} > > + > > +/** > > + * qos_min_notifier_call() - Callback for QoS min_freq changes. > > + * @nb: Should be devfreq->nb_min > > + */ > > +static int qos_min_notifier_call(struct notifier_block *nb, > > + unsigned long val, void *ptr) > > +{ > > + return qos_notifier_call(container_of(nb, struct devfreq, nb_min)); > > +} > > + > > +/** > > + * qos_max_notifier_call() - Callback for QoS max_freq changes. > > + * @nb: Should be devfreq->nb_max > > + */ > > +static int qos_max_notifier_call(struct notifier_block *nb, > > + unsigned long val, void *ptr) > > +{ > > + return qos_notifier_call(container_of(nb, struct devfreq, nb_max)); > > +} > > + > > /** > > * devfreq_dev_release() - Callback for struct device to release the device. > > * @dev: the devfreq device > > * > > * Remove devfreq from the list and release its resources. > > @@ -620,10 +671,15 @@ static void devfreq_dev_release(struct device *dev) > > > > mutex_lock(&devfreq_list_lock); > > list_del(&devfreq->node); > > mutex_unlock(&devfreq_list_lock); > > > > + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max, > > + DEV_PM_QOS_MAX_FREQUENCY); > > + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min, > > + DEV_PM_QOS_MIN_FREQUENCY); > > + > > Just print error with dev_err() without stopping the release step. > > I prefer to handle the return value if kernel API provides > the error code. > > > if (devfreq->profile->exit) > > devfreq->profile->exit(devfreq->dev.parent); > > > > kfree(devfreq->time_in_state); > > kfree(devfreq->trans_table); > > @@ -733,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev, > > if (err) { > > put_device(&devfreq->dev); > > goto err_out; > > } > > > > + /* > > + * Register notifiers for updates to min/max_freq after device is > > + * initialized (and we can handle notifications) but before the > > + * governor is started (which should do an initial enforcement of > > + * constraints). > > + */ > > My previous comment is not enough why I prefer to remove it. Sorry. > Actually, until now, the devfreq_add_device() don't have the detailed > comments because the line code is not too long. But, at the present time, > devfreq_add_device() is too long. It means that the detailed comment > are necessary. > > So, I'll add the detailed comment for each step of devfreq_add_device() > on separate patch to keep the same style. I'll send the patch to you > for the review. > > > + devfreq->nb_min.notifier_call = qos_min_notifier_call; > > + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min, > > + DEV_PM_QOS_MIN_FREQUENCY); > > + if (err) > > + goto err_devfreq; > > + > > + devfreq->nb_max.notifier_call = qos_max_notifier_call; > > + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max, > > + DEV_PM_QOS_MAX_FREQUENCY); > > + if (err) > > + goto err_devfreq; > > + > > mutex_lock(&devfreq_list_lock); > > > > governor = try_then_request_governor(devfreq->governor_name); > > if (IS_ERR(governor)) { > > dev_err(dev, "%s: Unable to find governor for the device\n", > > @@ -760,10 +834,11 @@ struct devfreq *devfreq_add_device(struct device *dev, > > > > return devfreq; > > > > err_init: > > mutex_unlock(&devfreq_list_lock); > > +err_devfreq: > > devfreq_remove_device(devfreq); > > return ERR_PTR(err); > > > > err_dev: > > /* > > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h > > index c3cbc15fdf08..dac0dffeabb4 100644 > > --- a/include/linux/devfreq.h > > +++ b/include/linux/devfreq.h > > @@ -134,10 +134,12 @@ struct devfreq_dev_profile { > > * @total_trans: Number of devfreq transitions > > * @trans_table: Statistics of devfreq transitions > > * @time_in_state: Statistics of devfreq states > > * @last_stat_updated: The last time stat updated > > * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier > > + * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY > > + * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY > > * > > * This structure stores the devfreq information for a give device. > > * > > * Note that when a governor accesses entries in struct devfreq in its > > * functions except for the context of callbacks defined in struct > > @@ -176,10 +178,13 @@ struct devfreq { > > unsigned int *trans_table; > > unsigned long *time_in_state; > > unsigned long last_stat_updated; > > > > struct srcu_notifier_head transition_notifier_list; > > + > > + struct notifier_block nb_min; > > + struct notifier_block nb_max; > > }; > > > > struct devfreq_freqs { > > unsigned long old; > > unsigned long new; > > > > > -- > Best Regards, > Chanwoo Choi > Samsung Electronics
On 25.09.2019 05:13, Chanwoo Choi wrote: > On 19. 9. 25. 오전 4:22, Leonard Crestez wrote: >> On 24.09.2019 22:11, Matthias Kaehlcke wrote: >>> On Tue, Sep 24, 2019 at 01:11:29PM +0300, Leonard Crestez wrote: >>>> Register notifiers with the PM QoS framework in order to respond to >>>> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY. >>>> >>>> No notifiers are added by this patch but PM QoS constraints can be >>>> imposed externally (for example from other devices). >>>> >>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >>>> --- >>>> drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++ >>>> include/linux/devfreq.h | 5 +++ >>>> 2 files changed, 80 insertions(+) >>>> >>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>> index eee403e70c84..784f3e40536a 100644 >>>> --- a/drivers/devfreq/devfreq.c >>>> +++ b/drivers/devfreq/devfreq.c >>>> @@ -22,15 +22,18 @@ >>>> #include <linux/platform_device.h> >>>> #include <linux/list.h> >>>> #include <linux/printk.h> >>>> #include <linux/hrtimer.h> >>>> #include <linux/of.h> >>>> +#include <linux/pm_qos.h> >>>> #include "governor.h" >>>> >>>> #define CREATE_TRACE_POINTS >>>> #include <trace/events/devfreq.h> >>>> >>>> +#define HZ_PER_KHZ 1000 >>>> + >>>> static struct class *devfreq_class; >>>> >>>> /* >>>> * devfreq core provides delayed work based load monitoring helper >>>> * functions. Governors can use these or can implement their own >>>> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) >>>> static void get_freq_range(struct devfreq *devfreq, >>>> unsigned long *min_freq, >>>> unsigned long *max_freq) >>>> { >>>> unsigned long *freq_table = devfreq->profile->freq_table; >>>> + unsigned long qos_min_freq, qos_max_freq; >>>> >>>> lockdep_assert_held(&devfreq->lock); >>>> >>>> /* >>>> * Init min/max frequency from freq table. >>>> @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq, >>>> } else { >>>> *min_freq = freq_table[devfreq->profile->max_state - 1]; >>>> *max_freq = freq_table[0]; >>>> } >>>> >>>> + /* constraints from PM QoS */ >>>> + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>> + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>> + DEV_PM_QOS_MIN_FREQUENCY); > >>> This needs to be DEV_PM_QOS_MAX_FREQUENCY. I missed this in the earlier >>> reviews, but stumbled across it when testing. >>> >>> !Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >> >> I broke it in v8 while processing comments. Thanks for catching it. > > Did you test this patchset with v8? > Maybe it is not working with this mistake. I ran some scripts which test that min_freq requests work as expected (using imx interconnect+devfreq). They don't touch max_freq. -- Regards, Leonard
On 25.09.2019 05:11, Chanwoo Choi wrote: > On 19. 9. 24. 오후 7:11, Leonard Crestez wrote: >> Register notifiers with the PM QoS framework in order to respond to >> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY. >> >> No notifiers are added by this patch but PM QoS constraints can be >> imposed externally (for example from other devices). >> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >> --- >> drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++ >> include/linux/devfreq.h | 5 +++ >> 2 files changed, 80 insertions(+) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index eee403e70c84..784f3e40536a 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -22,15 +22,18 @@ >> #include <linux/platform_device.h> >> #include <linux/list.h> >> #include <linux/printk.h> >> #include <linux/hrtimer.h> >> #include <linux/of.h> >> +#include <linux/pm_qos.h> >> #include "governor.h" >> >> #define CREATE_TRACE_POINTS >> #include <trace/events/devfreq.h> >> >> +#define HZ_PER_KHZ 1000 >> + >> static struct class *devfreq_class; >> >> /* >> * devfreq core provides delayed work based load monitoring helper >> * functions. Governors can use these or can implement their own >> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) >> static void get_freq_range(struct devfreq *devfreq, >> unsigned long *min_freq, >> unsigned long *max_freq) >> { >> unsigned long *freq_table = devfreq->profile->freq_table; >> + unsigned long qos_min_freq, qos_max_freq; >> >> lockdep_assert_held(&devfreq->lock); >> >> /* >> * Init min/max frequency from freq table. >> @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq, >> } else { >> *min_freq = freq_table[devfreq->profile->max_state - 1]; >> *max_freq = freq_table[0]; >> } >> >> + /* constraints from PM QoS */ > > As I commented on patch4, > 'constraints' -> 'Constraint' because first verb have to be used > as the sigular verbs. Already discussed for another patch; I will modify to "Apply constraints from PM QoS" instead. > I prefer to use following comments: > > /* Constraint minimum/maximum frequency from PM QoS constraints */ > >> + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent, >> + DEV_PM_QOS_MIN_FREQUENCY); >> + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, >> + DEV_PM_QOS_MIN_FREQUENCY); >> + *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); >> + *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); >> + >> /* constraints from sysfs */ >> *min_freq = max(*min_freq, devfreq->min_freq); >> *max_freq = min(*max_freq, devfreq->max_freq); >> >> /* constraints from OPP interface */ >> @@ -606,10 +618,49 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, >> mutex_unlock(&devfreq->lock); >> >> return ret; >> } >> >> +/** >> + * qos_notifier_call() - Common handler for QoS constraints. >> + * @devfreq: the devfreq instance. >> + */ >> +static int qos_notifier_call(struct devfreq *devfreq) >> +{ >> + int err; >> + >> + mutex_lock(&devfreq->lock); >> + err = update_devfreq(devfreq); >> + mutex_unlock(&devfreq->lock); >> + if (err) >> + dev_err(devfreq->dev.parent, >> + "failed to update frequency for PM QoS constraints (%d)\n", > > Is it not over 80 char? Yes but coding style explicitly forbids breaking strings. >> + err); >> + >> + return NOTIFY_OK; >> +} >> + >> +/** >> + * qos_min_notifier_call() - Callback for QoS min_freq changes. >> + * @nb: Should be devfreq->nb_min >> + */ >> +static int qos_min_notifier_call(struct notifier_block *nb, >> + unsigned long val, void *ptr) >> +{ >> + return qos_notifier_call(container_of(nb, struct devfreq, nb_min)); >> +} >> + >> +/** >> + * qos_max_notifier_call() - Callback for QoS max_freq changes. >> + * @nb: Should be devfreq->nb_max >> + */ >> +static int qos_max_notifier_call(struct notifier_block *nb, >> + unsigned long val, void *ptr) >> +{ >> + return qos_notifier_call(container_of(nb, struct devfreq, nb_max)); >> +} >> + >> /** >> * devfreq_dev_release() - Callback for struct device to release the device. >> * @dev: the devfreq device >> * >> * Remove devfreq from the list and release its resources. >> @@ -620,10 +671,15 @@ static void devfreq_dev_release(struct device *dev) >> >> mutex_lock(&devfreq_list_lock); >> list_del(&devfreq->node); >> mutex_unlock(&devfreq_list_lock); >> >> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max, >> + DEV_PM_QOS_MAX_FREQUENCY); >> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min, >> + DEV_PM_QOS_MIN_FREQUENCY); >> + > > Just print error with dev_err() without stopping the release step. > > I prefer to handle the return value if kernel API provides > the error code. > >> if (devfreq->profile->exit) >> devfreq->profile->exit(devfreq->dev.parent); >> >> kfree(devfreq->time_in_state); >> kfree(devfreq->trans_table); >> @@ -733,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev, >> if (err) { >> put_device(&devfreq->dev); >> goto err_out; >> } >> >> + /* >> + * Register notifiers for updates to min/max_freq after device is >> + * initialized (and we can handle notifications) but before the >> + * governor is started (which should do an initial enforcement of >> + * constraints). >> + */ > > My previous comment is not enough why I prefer to remove it. Sorry. > Actually, until now, the devfreq_add_device() don't have the detailed > comments because the line code is not too long. But, at the present time, > devfreq_add_device() is too long. It means that the detailed comment > are necessary. > > So, I'll add the detailed comment for each step of devfreq_add_device() > on separate patch to keep the same style. I'll send the patch to you > for the review. This is very likely to result in merge conflicts, maybe wait for my series to go in first? >> + devfreq->nb_min.notifier_call = qos_min_notifier_call; >> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min, >> + DEV_PM_QOS_MIN_FREQUENCY); >> + if (err) >> + goto err_devfreq; >> + >> + devfreq->nb_max.notifier_call = qos_max_notifier_call; >> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max, >> + DEV_PM_QOS_MAX_FREQUENCY); >> + if (err) >> + goto err_devfreq; >> + >> mutex_lock(&devfreq_list_lock); >> >> governor = try_then_request_governor(devfreq->governor_name); >> if (IS_ERR(governor)) { >> dev_err(dev, "%s: Unable to find governor for the device\n", >> @@ -760,10 +834,11 @@ struct devfreq *devfreq_add_device(struct device *dev, >> >> return devfreq; >> >> err_init: >> mutex_unlock(&devfreq_list_lock); >> +err_devfreq: >> devfreq_remove_device(devfreq); >> return ERR_PTR(err); >> >> err_dev: >> /* >> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >> index c3cbc15fdf08..dac0dffeabb4 100644 >> --- a/include/linux/devfreq.h >> +++ b/include/linux/devfreq.h >> @@ -134,10 +134,12 @@ struct devfreq_dev_profile { >> * @total_trans: Number of devfreq transitions >> * @trans_table: Statistics of devfreq transitions >> * @time_in_state: Statistics of devfreq states >> * @last_stat_updated: The last time stat updated >> * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier >> + * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY >> + * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY >> * >> * This structure stores the devfreq information for a give device. >> * >> * Note that when a governor accesses entries in struct devfreq in its >> * functions except for the context of callbacks defined in struct >> @@ -176,10 +178,13 @@ struct devfreq { >> unsigned int *trans_table; >> unsigned long *time_in_state; >> unsigned long last_stat_updated; >> >> struct srcu_notifier_head transition_notifier_list; >> + >> + struct notifier_block nb_min; >> + struct notifier_block nb_max; >> }; >> >> struct devfreq_freqs { >> unsigned long old; >> unsigned long new; >> > >
Hi, On 19. 9. 26. 오전 4:40, Leonard Crestez wrote: > On 25.09.2019 05:13, Chanwoo Choi wrote: >> On 19. 9. 25. 오전 4:22, Leonard Crestez wrote: >>> On 24.09.2019 22:11, Matthias Kaehlcke wrote: >>>> On Tue, Sep 24, 2019 at 01:11:29PM +0300, Leonard Crestez wrote: >>>>> Register notifiers with the PM QoS framework in order to respond to >>>>> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY. >>>>> >>>>> No notifiers are added by this patch but PM QoS constraints can be >>>>> imposed externally (for example from other devices). >>>>> >>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >>>>> --- >>>>> drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++ >>>>> include/linux/devfreq.h | 5 +++ >>>>> 2 files changed, 80 insertions(+) >>>>> >>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>>> index eee403e70c84..784f3e40536a 100644 >>>>> --- a/drivers/devfreq/devfreq.c >>>>> +++ b/drivers/devfreq/devfreq.c >>>>> @@ -22,15 +22,18 @@ >>>>> #include <linux/platform_device.h> >>>>> #include <linux/list.h> >>>>> #include <linux/printk.h> >>>>> #include <linux/hrtimer.h> >>>>> #include <linux/of.h> >>>>> +#include <linux/pm_qos.h> >>>>> #include "governor.h" >>>>> >>>>> #define CREATE_TRACE_POINTS >>>>> #include <trace/events/devfreq.h> >>>>> >>>>> +#define HZ_PER_KHZ 1000 >>>>> + >>>>> static struct class *devfreq_class; >>>>> >>>>> /* >>>>> * devfreq core provides delayed work based load monitoring helper >>>>> * functions. Governors can use these or can implement their own >>>>> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) >>>>> static void get_freq_range(struct devfreq *devfreq, >>>>> unsigned long *min_freq, >>>>> unsigned long *max_freq) >>>>> { >>>>> unsigned long *freq_table = devfreq->profile->freq_table; >>>>> + unsigned long qos_min_freq, qos_max_freq; >>>>> >>>>> lockdep_assert_held(&devfreq->lock); >>>>> >>>>> /* >>>>> * Init min/max frequency from freq table. >>>>> @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq, >>>>> } else { >>>>> *min_freq = freq_table[devfreq->profile->max_state - 1]; >>>>> *max_freq = freq_table[0]; >>>>> } >>>>> >>>>> + /* constraints from PM QoS */ >>>>> + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>> + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>>> + DEV_PM_QOS_MIN_FREQUENCY); > >>>> This needs to be DEV_PM_QOS_MAX_FREQUENCY. I missed this in the earlier >>>> reviews, but stumbled across it when testing. >>>> >>>> !Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >>> >>> I broke it in v8 while processing comments. Thanks for catching it. >> >> Did you test this patchset with v8? >> Maybe it is not working with this mistake. > > I ran some scripts which test that min_freq requests work as expected > (using imx interconnect+devfreq). They don't touch max_freq. We always have to test the code before contributing the patch. Please test all cases for these patches. > > -- > Regards, > Leonard >
On 19. 9. 26. 오전 6:18, Leonard Crestez wrote: > On 25.09.2019 05:11, Chanwoo Choi wrote: >> On 19. 9. 24. 오후 7:11, Leonard Crestez wrote: >>> Register notifiers with the PM QoS framework in order to respond to >>> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY. >>> >>> No notifiers are added by this patch but PM QoS constraints can be >>> imposed externally (for example from other devices). >>> >>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >>> --- >>> drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++ >>> include/linux/devfreq.h | 5 +++ >>> 2 files changed, 80 insertions(+) >>> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>> index eee403e70c84..784f3e40536a 100644 >>> --- a/drivers/devfreq/devfreq.c >>> +++ b/drivers/devfreq/devfreq.c >>> @@ -22,15 +22,18 @@ >>> #include <linux/platform_device.h> >>> #include <linux/list.h> >>> #include <linux/printk.h> >>> #include <linux/hrtimer.h> >>> #include <linux/of.h> >>> +#include <linux/pm_qos.h> >>> #include "governor.h" >>> >>> #define CREATE_TRACE_POINTS >>> #include <trace/events/devfreq.h> >>> >>> +#define HZ_PER_KHZ 1000 >>> + >>> static struct class *devfreq_class; >>> >>> /* >>> * devfreq core provides delayed work based load monitoring helper >>> * functions. Governors can use these or can implement their own >>> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) >>> static void get_freq_range(struct devfreq *devfreq, >>> unsigned long *min_freq, >>> unsigned long *max_freq) >>> { >>> unsigned long *freq_table = devfreq->profile->freq_table; >>> + unsigned long qos_min_freq, qos_max_freq; >>> >>> lockdep_assert_held(&devfreq->lock); >>> >>> /* >>> * Init min/max frequency from freq table. >>> @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq, >>> } else { >>> *min_freq = freq_table[devfreq->profile->max_state - 1]; >>> *max_freq = freq_table[0]; >>> } >>> >>> + /* constraints from PM QoS */ >> >> As I commented on patch4, >> 'constraints' -> 'Constraint' because first verb have to be used >> as the sigular verbs. > > Already discussed for another patch; I will modify to "Apply constraints > from PM QoS" instead. I agree the new comment with 'Apply constraints ... '. > >> I prefer to use following comments: >> >> /* Constraint minimum/maximum frequency from PM QoS constraints */ >> >>> + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>> + DEV_PM_QOS_MIN_FREQUENCY); >>> + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>> + DEV_PM_QOS_MIN_FREQUENCY); >>> + *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); >>> + *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); >>> + >>> /* constraints from sysfs */ >>> *min_freq = max(*min_freq, devfreq->min_freq); >>> *max_freq = min(*max_freq, devfreq->max_freq); >>> >>> /* constraints from OPP interface */ >>> @@ -606,10 +618,49 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, >>> mutex_unlock(&devfreq->lock); >>> >>> return ret; >>> } >>> >>> +/** >>> + * qos_notifier_call() - Common handler for QoS constraints. >>> + * @devfreq: the devfreq instance. >>> + */ >>> +static int qos_notifier_call(struct devfreq *devfreq) >>> +{ >>> + int err; >>> + >>> + mutex_lock(&devfreq->lock); >>> + err = update_devfreq(devfreq); >>> + mutex_unlock(&devfreq->lock); >>> + if (err) >>> + dev_err(devfreq->dev.parent, >>> + "failed to update frequency for PM QoS constraints (%d)\n", >> >> Is it not over 80 char? > > Yes but coding style explicitly forbids breaking strings. > >>> + err); >>> + >>> + return NOTIFY_OK; >>> +} >>> + >>> +/** >>> + * qos_min_notifier_call() - Callback for QoS min_freq changes. >>> + * @nb: Should be devfreq->nb_min >>> + */ >>> +static int qos_min_notifier_call(struct notifier_block *nb, >>> + unsigned long val, void *ptr) >>> +{ >>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_min)); >>> +} >>> + >>> +/** >>> + * qos_max_notifier_call() - Callback for QoS max_freq changes. >>> + * @nb: Should be devfreq->nb_max >>> + */ >>> +static int qos_max_notifier_call(struct notifier_block *nb, >>> + unsigned long val, void *ptr) >>> +{ >>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_max)); >>> +} >>> + >>> /** >>> * devfreq_dev_release() - Callback for struct device to release the device. >>> * @dev: the devfreq device >>> * >>> * Remove devfreq from the list and release its resources. >>> @@ -620,10 +671,15 @@ static void devfreq_dev_release(struct device *dev) >>> >>> mutex_lock(&devfreq_list_lock); >>> list_del(&devfreq->node); >>> mutex_unlock(&devfreq_list_lock); >>> >>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max, >>> + DEV_PM_QOS_MAX_FREQUENCY); >>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min, >>> + DEV_PM_QOS_MIN_FREQUENCY); >>> + >> >> Just print error with dev_err() without stopping the release step. >> >> I prefer to handle the return value if kernel API provides >> the error code. How about? >> >>> if (devfreq->profile->exit) >>> devfreq->profile->exit(devfreq->dev.parent); >>> >>> kfree(devfreq->time_in_state); >>> kfree(devfreq->trans_table); >>> @@ -733,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev, >>> if (err) { >>> put_device(&devfreq->dev); >>> goto err_out; >>> } >>> >>> + /* >>> + * Register notifiers for updates to min/max_freq after device is >>> + * initialized (and we can handle notifications) but before the >>> + * governor is started (which should do an initial enforcement of >>> + * constraints). >>> + */ >> >> My previous comment is not enough why I prefer to remove it. Sorry. >> Actually, until now, the devfreq_add_device() don't have the detailed >> comments because the line code is not too long. But, at the present time, >> devfreq_add_device() is too long. It means that the detailed comment >> are necessary. >> >> So, I'll add the detailed comment for each step of devfreq_add_device() >> on separate patch to keep the same style. I'll send the patch to you >> for the review. > > This is very likely to result in merge conflicts, maybe wait for my > series to go in first? I'll send the separate patch after finished the review of these patches. So, if you agree, please remove this comment on this patch. You can review the detailed comments on separate patch when I send. > >>> + devfreq->nb_min.notifier_call = qos_min_notifier_call; >>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min, >>> + DEV_PM_QOS_MIN_FREQUENCY); >>> + if (err) >>> + goto err_devfreq; >>> + >>> + devfreq->nb_max.notifier_call = qos_max_notifier_call; >>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max, >>> + DEV_PM_QOS_MAX_FREQUENCY); >>> + if (err) >>> + goto err_devfreq; >>> + >>> mutex_lock(&devfreq_list_lock); >>> >>> governor = try_then_request_governor(devfreq->governor_name); >>> if (IS_ERR(governor)) { >>> dev_err(dev, "%s: Unable to find governor for the device\n", >>> @@ -760,10 +834,11 @@ struct devfreq *devfreq_add_device(struct device *dev, >>> >>> return devfreq; >>> >>> err_init: >>> mutex_unlock(&devfreq_list_lock); >>> +err_devfreq: >>> devfreq_remove_device(devfreq); >>> return ERR_PTR(err); >>> >>> err_dev: >>> /* >>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >>> index c3cbc15fdf08..dac0dffeabb4 100644 >>> --- a/include/linux/devfreq.h >>> +++ b/include/linux/devfreq.h >>> @@ -134,10 +134,12 @@ struct devfreq_dev_profile { >>> * @total_trans: Number of devfreq transitions >>> * @trans_table: Statistics of devfreq transitions >>> * @time_in_state: Statistics of devfreq states >>> * @last_stat_updated: The last time stat updated >>> * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier >>> + * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY >>> + * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY >>> * >>> * This structure stores the devfreq information for a give device. >>> * >>> * Note that when a governor accesses entries in struct devfreq in its >>> * functions except for the context of callbacks defined in struct >>> @@ -176,10 +178,13 @@ struct devfreq { >>> unsigned int *trans_table; >>> unsigned long *time_in_state; >>> unsigned long last_stat_updated; >>> >>> struct srcu_notifier_head transition_notifier_list; >>> + >>> + struct notifier_block nb_min; >>> + struct notifier_block nb_max; >>> }; >>> >>> struct devfreq_freqs { >>> unsigned long old; >>> unsigned long new; >>> >> >> >
On 19. 9. 26. 오전 6:18, Leonard Crestez wrote: > On 25.09.2019 05:11, Chanwoo Choi wrote: >> On 19. 9. 24. 오후 7:11, Leonard Crestez wrote: >>> Register notifiers with the PM QoS framework in order to respond to >>> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY. >>> >>> No notifiers are added by this patch but PM QoS constraints can be >>> imposed externally (for example from other devices). >>> >>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >>> --- >>> drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++ >>> include/linux/devfreq.h | 5 +++ >>> 2 files changed, 80 insertions(+) >>> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>> index eee403e70c84..784f3e40536a 100644 >>> --- a/drivers/devfreq/devfreq.c >>> +++ b/drivers/devfreq/devfreq.c >>> @@ -22,15 +22,18 @@ >>> #include <linux/platform_device.h> >>> #include <linux/list.h> >>> #include <linux/printk.h> >>> #include <linux/hrtimer.h> >>> #include <linux/of.h> >>> +#include <linux/pm_qos.h> >>> #include "governor.h" >>> >>> #define CREATE_TRACE_POINTS >>> #include <trace/events/devfreq.h> >>> >>> +#define HZ_PER_KHZ 1000 >>> + >>> static struct class *devfreq_class; >>> >>> /* >>> * devfreq core provides delayed work based load monitoring helper >>> * functions. Governors can use these or can implement their own >>> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) >>> static void get_freq_range(struct devfreq *devfreq, >>> unsigned long *min_freq, >>> unsigned long *max_freq) >>> { >>> unsigned long *freq_table = devfreq->profile->freq_table; >>> + unsigned long qos_min_freq, qos_max_freq; >>> >>> lockdep_assert_held(&devfreq->lock); >>> >>> /* >>> * Init min/max frequency from freq table. >>> @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq, >>> } else { >>> *min_freq = freq_table[devfreq->profile->max_state - 1]; >>> *max_freq = freq_table[0]; >>> } >>> >>> + /* constraints from PM QoS */ >> >> As I commented on patch4, >> 'constraints' -> 'Constraint' because first verb have to be used >> as the sigular verbs. > > Already discussed for another patch; I will modify to "Apply constraints > from PM QoS" instead. > >> I prefer to use following comments: >> >> /* Constraint minimum/maximum frequency from PM QoS constraints */ >> >>> + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>> + DEV_PM_QOS_MIN_FREQUENCY); >>> + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>> + DEV_PM_QOS_MIN_FREQUENCY); >>> + *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); >>> + *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); >>> + >>> /* constraints from sysfs */ >>> *min_freq = max(*min_freq, devfreq->min_freq); >>> *max_freq = min(*max_freq, devfreq->max_freq); >>> >>> /* constraints from OPP interface */ >>> @@ -606,10 +618,49 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, >>> mutex_unlock(&devfreq->lock); >>> >>> return ret; >>> } >>> >>> +/** >>> + * qos_notifier_call() - Common handler for QoS constraints. >>> + * @devfreq: the devfreq instance. >>> + */ >>> +static int qos_notifier_call(struct devfreq *devfreq) >>> +{ >>> + int err; >>> + >>> + mutex_lock(&devfreq->lock); >>> + err = update_devfreq(devfreq); >>> + mutex_unlock(&devfreq->lock); >>> + if (err) >>> + dev_err(devfreq->dev.parent, >>> + "failed to update frequency for PM QoS constraints (%d)\n", >> >> Is it not over 80 char? > > Yes but coding style explicitly forbids breaking strings. I want to make it within 80 char. How about following comment? dev_err(devfreq->dev.parent, "failed to update frequency from PM QoS (%d)\n", >>> + err); >>> + >>> + return NOTIFY_OK; >>> +} >>> + >>> +/** >>> + * qos_min_notifier_call() - Callback for QoS min_freq changes. >>> + * @nb: Should be devfreq->nb_min >>> + */ >>> +static int qos_min_notifier_call(struct notifier_block *nb, >>> + unsigned long val, void *ptr) >>> +{ >>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_min)); >>> +} >>> + >>> +/** >>> + * qos_max_notifier_call() - Callback for QoS max_freq changes. >>> + * @nb: Should be devfreq->nb_max >>> + */ >>> +static int qos_max_notifier_call(struct notifier_block *nb, >>> + unsigned long val, void *ptr) >>> +{ >>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_max)); >>> +} >>> + >>> /** >>> * devfreq_dev_release() - Callback for struct device to release the device. >>> * @dev: the devfreq device >>> * >>> * Remove devfreq from the list and release its resources. >>> @@ -620,10 +671,15 @@ static void devfreq_dev_release(struct device *dev) >>> >>> mutex_lock(&devfreq_list_lock); >>> list_del(&devfreq->node); >>> mutex_unlock(&devfreq_list_lock); >>> >>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max, >>> + DEV_PM_QOS_MAX_FREQUENCY); >>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min, >>> + DEV_PM_QOS_MIN_FREQUENCY); >>> + >> >> Just print error with dev_err() without stopping the release step. >> >> I prefer to handle the return value if kernel API provides >> the error code. >> >>> if (devfreq->profile->exit) >>> devfreq->profile->exit(devfreq->dev.parent); >>> >>> kfree(devfreq->time_in_state); >>> kfree(devfreq->trans_table); >>> @@ -733,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev, >>> if (err) { >>> put_device(&devfreq->dev); >>> goto err_out; >>> } >>> >>> + /* >>> + * Register notifiers for updates to min/max_freq after device is >>> + * initialized (and we can handle notifications) but before the >>> + * governor is started (which should do an initial enforcement of >>> + * constraints). >>> + */ >> >> My previous comment is not enough why I prefer to remove it. Sorry. >> Actually, until now, the devfreq_add_device() don't have the detailed >> comments because the line code is not too long. But, at the present time, >> devfreq_add_device() is too long. It means that the detailed comment >> are necessary. >> >> So, I'll add the detailed comment for each step of devfreq_add_device() >> on separate patch to keep the same style. I'll send the patch to you >> for the review. > > This is very likely to result in merge conflicts, maybe wait for my > series to go in first? > >>> + devfreq->nb_min.notifier_call = qos_min_notifier_call; >>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min, >>> + DEV_PM_QOS_MIN_FREQUENCY); >>> + if (err) >>> + goto err_devfreq; >>> + >>> + devfreq->nb_max.notifier_call = qos_max_notifier_call; >>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max, >>> + DEV_PM_QOS_MAX_FREQUENCY); >>> + if (err) >>> + goto err_devfreq; >>> + >>> mutex_lock(&devfreq_list_lock); >>> >>> governor = try_then_request_governor(devfreq->governor_name); >>> if (IS_ERR(governor)) { >>> dev_err(dev, "%s: Unable to find governor for the device\n", >>> @@ -760,10 +834,11 @@ struct devfreq *devfreq_add_device(struct device *dev, >>> >>> return devfreq; >>> >>> err_init: >>> mutex_unlock(&devfreq_list_lock); >>> +err_devfreq: >>> devfreq_remove_device(devfreq); >>> return ERR_PTR(err); >>> >>> err_dev: >>> /* >>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >>> index c3cbc15fdf08..dac0dffeabb4 100644 >>> --- a/include/linux/devfreq.h >>> +++ b/include/linux/devfreq.h >>> @@ -134,10 +134,12 @@ struct devfreq_dev_profile { >>> * @total_trans: Number of devfreq transitions >>> * @trans_table: Statistics of devfreq transitions >>> * @time_in_state: Statistics of devfreq states >>> * @last_stat_updated: The last time stat updated >>> * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier >>> + * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY >>> + * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY >>> * >>> * This structure stores the devfreq information for a give device. >>> * >>> * Note that when a governor accesses entries in struct devfreq in its >>> * functions except for the context of callbacks defined in struct >>> @@ -176,10 +178,13 @@ struct devfreq { >>> unsigned int *trans_table; >>> unsigned long *time_in_state; >>> unsigned long last_stat_updated; >>> >>> struct srcu_notifier_head transition_notifier_list; >>> + >>> + struct notifier_block nb_min; >>> + struct notifier_block nb_max; >>> }; >>> >>> struct devfreq_freqs { >>> unsigned long old; >>> unsigned long new; >>> >> >> >
On 2019-09-26 4:07 AM, Chanwoo Choi wrote: > On 19. 9. 26. 오전 6:18, Leonard Crestez wrote: >> On 25.09.2019 05:11, Chanwoo Choi wrote: >>> On 19. 9. 24. 오후 7:11, Leonard Crestez wrote: >>>> Register notifiers with the PM QoS framework in order to respond to >>>> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY. >>>> >>>> No notifiers are added by this patch but PM QoS constraints can be >>>> imposed externally (for example from other devices). >>>> >>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >>>> --- >>>> drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++ >>>> include/linux/devfreq.h | 5 +++ >>>> 2 files changed, 80 insertions(+) >>>> >>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>> index eee403e70c84..784f3e40536a 100644 >>>> --- a/drivers/devfreq/devfreq.c >>>> +++ b/drivers/devfreq/devfreq.c >>>> @@ -22,15 +22,18 @@ >>>> #include <linux/platform_device.h> >>>> #include <linux/list.h> >>>> #include <linux/printk.h> >>>> #include <linux/hrtimer.h> >>>> #include <linux/of.h> >>>> +#include <linux/pm_qos.h> >>>> #include "governor.h" >>>> >>>> #define CREATE_TRACE_POINTS >>>> #include <trace/events/devfreq.h> >>>> >>>> +#define HZ_PER_KHZ 1000 >>>> + >>>> static struct class *devfreq_class; >>>> >>>> /* >>>> * devfreq core provides delayed work based load monitoring helper >>>> * functions. Governors can use these or can implement their own >>>> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) >>>> static void get_freq_range(struct devfreq *devfreq, >>>> unsigned long *min_freq, >>>> unsigned long *max_freq) >>>> { >>>> unsigned long *freq_table = devfreq->profile->freq_table; >>>> + unsigned long qos_min_freq, qos_max_freq; >>>> >>>> lockdep_assert_held(&devfreq->lock); >>>> >>>> /* >>>> * Init min/max frequency from freq table. >>>> @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq, >>>> } else { >>>> *min_freq = freq_table[devfreq->profile->max_state - 1]; >>>> *max_freq = freq_table[0]; >>>> } >>>> >>>> + /* constraints from PM QoS */ >>> >>> As I commented on patch4, >>> 'constraints' -> 'Constraint' because first verb have to be used >>> as the sigular verbs. >> >> Already discussed for another patch; I will modify to "Apply constraints >> from PM QoS" instead. > > I agree the new comment with 'Apply constraints ... '. > >> >>> I prefer to use following comments: >>> >>> /* Constraint minimum/maximum frequency from PM QoS constraints */ >>> >>>> + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>> + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>> + *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); >>>> + *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); >>>> + >>>> /* constraints from sysfs */ >>>> *min_freq = max(*min_freq, devfreq->min_freq); >>>> *max_freq = min(*max_freq, devfreq->max_freq); >>>> >>>> /* constraints from OPP interface */ >>>> @@ -606,10 +618,49 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, >>>> mutex_unlock(&devfreq->lock); >>>> >>>> return ret; >>>> } >>>> >>>> +/** >>>> + * qos_notifier_call() - Common handler for QoS constraints. >>>> + * @devfreq: the devfreq instance. >>>> + */ >>>> +static int qos_notifier_call(struct devfreq *devfreq) >>>> +{ >>>> + int err; >>>> + >>>> + mutex_lock(&devfreq->lock); >>>> + err = update_devfreq(devfreq); >>>> + mutex_unlock(&devfreq->lock); >>>> + if (err) >>>> + dev_err(devfreq->dev.parent, >>>> + "failed to update frequency for PM QoS constraints (%d)\n", >>> >>> Is it not over 80 char? >> >> Yes but coding style explicitly forbids breaking strings. >> >>>> + err); >>>> + >>>> + return NOTIFY_OK; >>>> +} >>>> + >>>> +/** >>>> + * qos_min_notifier_call() - Callback for QoS min_freq changes. >>>> + * @nb: Should be devfreq->nb_min >>>> + */ >>>> +static int qos_min_notifier_call(struct notifier_block *nb, >>>> + unsigned long val, void *ptr) >>>> +{ >>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_min)); >>>> +} >>>> + >>>> +/** >>>> + * qos_max_notifier_call() - Callback for QoS max_freq changes. >>>> + * @nb: Should be devfreq->nb_max >>>> + */ >>>> +static int qos_max_notifier_call(struct notifier_block *nb, >>>> + unsigned long val, void *ptr) >>>> +{ >>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_max)); >>>> +} >>>> + >>>> /** >>>> * devfreq_dev_release() - Callback for struct device to release the device. >>>> * @dev: the devfreq device >>>> * >>>> * Remove devfreq from the list and release its resources. >>>> @@ -620,10 +671,15 @@ static void devfreq_dev_release(struct device *dev) >>>> >>>> mutex_lock(&devfreq_list_lock); >>>> list_del(&devfreq->node); >>>> mutex_unlock(&devfreq_list_lock); >>>> >>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max, >>>> + DEV_PM_QOS_MAX_FREQUENCY); >>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min, >>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>> + >>> >>> Just print error with dev_err() without stopping the release step. >>> >>> I prefer to handle the return value if kernel API provides >>> the error code. > > How about? Modified to dev_warn. This also applies to PATCH 6 so I replied there. >>>> if (devfreq->profile->exit) >>>> devfreq->profile->exit(devfreq->dev.parent); >>>> >>>> kfree(devfreq->time_in_state); >>>> kfree(devfreq->trans_table); >>>> @@ -733,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>> if (err) { >>>> put_device(&devfreq->dev); >>>> goto err_out; >>>> } >>>> >>>> + /* >>>> + * Register notifiers for updates to min/max_freq after device is >>>> + * initialized (and we can handle notifications) but before the >>>> + * governor is started (which should do an initial enforcement of >>>> + * constraints). >>>> + */ >>> >>> My previous comment is not enough why I prefer to remove it. Sorry. >>> Actually, until now, the devfreq_add_device() don't have the detailed >>> comments because the line code is not too long. But, at the present time, >>> devfreq_add_device() is too long. It means that the detailed comment >>> are necessary. >>> >>> So, I'll add the detailed comment for each step of devfreq_add_device() >>> on separate patch to keep the same style. I'll send the patch to you >>> for the review. >> >> This is very likely to result in merge conflicts, maybe wait for my >> series to go in first? > > I'll send the separate patch after finished the review of these patches. > So, if you agree, please remove this comment on this patch. > > You can review the detailed comments on separate patch when I send. This patch already contains comments and they explain the code being added. Doesn't it make more sense for comments and code to go in together? I think the comment is a resonable explanation as to why notifiers are registered at that specific step in the initialization sequence. >>>> + devfreq->nb_min.notifier_call = qos_min_notifier_call; >>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min, >>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>> + if (err) >>>> + goto err_devfreq; >>>> + >>>> + devfreq->nb_max.notifier_call = qos_max_notifier_call; >>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max, >>>> + DEV_PM_QOS_MAX_FREQUENCY); >>>> + if (err) >>>> + goto err_devfreq; >>>> + >>>> mutex_lock(&devfreq_list_lock); >>>> >>>> governor = try_then_request_governor(devfreq->governor_name); >>>> if (IS_ERR(governor)) { >>>> dev_err(dev, "%s: Unable to find governor for the device\n", >>>> @@ -760,10 +834,11 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>> >>>> return devfreq; >>>> >>>> err_init: >>>> mutex_unlock(&devfreq_list_lock); >>>> +err_devfreq: >>>> devfreq_remove_device(devfreq); >>>> return ERR_PTR(err); >>>> >>>> err_dev: >>>> /* >>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >>>> index c3cbc15fdf08..dac0dffeabb4 100644 >>>> --- a/include/linux/devfreq.h >>>> +++ b/include/linux/devfreq.h >>>> @@ -134,10 +134,12 @@ struct devfreq_dev_profile { >>>> * @total_trans: Number of devfreq transitions >>>> * @trans_table: Statistics of devfreq transitions >>>> * @time_in_state: Statistics of devfreq states >>>> * @last_stat_updated: The last time stat updated >>>> * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier >>>> + * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY >>>> + * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY >>>> * >>>> * This structure stores the devfreq information for a give device. >>>> * >>>> * Note that when a governor accesses entries in struct devfreq in its >>>> * functions except for the context of callbacks defined in struct >>>> @@ -176,10 +178,13 @@ struct devfreq { >>>> unsigned int *trans_table; >>>> unsigned long *time_in_state; >>>> unsigned long last_stat_updated; >>>> >>>> struct srcu_notifier_head transition_notifier_list; >>>> + >>>> + struct notifier_block nb_min; >>>> + struct notifier_block nb_max; >>>> }; >>>> >>>> struct devfreq_freqs { >>>> unsigned long old; >>>> unsigned long new;
On 19. 9. 26. 오후 10:43, Leonard Crestez wrote: > On 2019-09-26 4:07 AM, Chanwoo Choi wrote: >> On 19. 9. 26. 오전 6:18, Leonard Crestez wrote: >>> On 25.09.2019 05:11, Chanwoo Choi wrote: >>>> On 19. 9. 24. 오후 7:11, Leonard Crestez wrote: >>>>> Register notifiers with the PM QoS framework in order to respond to >>>>> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY. >>>>> >>>>> No notifiers are added by this patch but PM QoS constraints can be >>>>> imposed externally (for example from other devices). >>>>> >>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >>>>> --- >>>>> drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++ >>>>> include/linux/devfreq.h | 5 +++ >>>>> 2 files changed, 80 insertions(+) >>>>> >>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>>> index eee403e70c84..784f3e40536a 100644 >>>>> --- a/drivers/devfreq/devfreq.c >>>>> +++ b/drivers/devfreq/devfreq.c >>>>> @@ -22,15 +22,18 @@ >>>>> #include <linux/platform_device.h> >>>>> #include <linux/list.h> >>>>> #include <linux/printk.h> >>>>> #include <linux/hrtimer.h> >>>>> #include <linux/of.h> >>>>> +#include <linux/pm_qos.h> >>>>> #include "governor.h" >>>>> >>>>> #define CREATE_TRACE_POINTS >>>>> #include <trace/events/devfreq.h> >>>>> >>>>> +#define HZ_PER_KHZ 1000 >>>>> + >>>>> static struct class *devfreq_class; >>>>> >>>>> /* >>>>> * devfreq core provides delayed work based load monitoring helper >>>>> * functions. Governors can use these or can implement their own >>>>> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) >>>>> static void get_freq_range(struct devfreq *devfreq, >>>>> unsigned long *min_freq, >>>>> unsigned long *max_freq) >>>>> { >>>>> unsigned long *freq_table = devfreq->profile->freq_table; >>>>> + unsigned long qos_min_freq, qos_max_freq; >>>>> >>>>> lockdep_assert_held(&devfreq->lock); >>>>> >>>>> /* >>>>> * Init min/max frequency from freq table. >>>>> @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq, >>>>> } else { >>>>> *min_freq = freq_table[devfreq->profile->max_state - 1]; >>>>> *max_freq = freq_table[0]; >>>>> } >>>>> >>>>> + /* constraints from PM QoS */ >>>> >>>> As I commented on patch4, >>>> 'constraints' -> 'Constraint' because first verb have to be used >>>> as the sigular verbs. >>> >>> Already discussed for another patch; I will modify to "Apply constraints >>> from PM QoS" instead. >> >> I agree the new comment with 'Apply constraints ... '. >> >>> >>>> I prefer to use following comments: >>>> >>>> /* Constraint minimum/maximum frequency from PM QoS constraints */ >>>> >>>>> + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>> + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>> + *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); >>>>> + *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); >>>>> + >>>>> /* constraints from sysfs */ >>>>> *min_freq = max(*min_freq, devfreq->min_freq); >>>>> *max_freq = min(*max_freq, devfreq->max_freq); >>>>> >>>>> /* constraints from OPP interface */ >>>>> @@ -606,10 +618,49 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, >>>>> mutex_unlock(&devfreq->lock); >>>>> >>>>> return ret; >>>>> } >>>>> >>>>> +/** >>>>> + * qos_notifier_call() - Common handler for QoS constraints. >>>>> + * @devfreq: the devfreq instance. >>>>> + */ >>>>> +static int qos_notifier_call(struct devfreq *devfreq) >>>>> +{ >>>>> + int err; >>>>> + >>>>> + mutex_lock(&devfreq->lock); >>>>> + err = update_devfreq(devfreq); >>>>> + mutex_unlock(&devfreq->lock); >>>>> + if (err) >>>>> + dev_err(devfreq->dev.parent, >>>>> + "failed to update frequency for PM QoS constraints (%d)\n", >>>> >>>> Is it not over 80 char? >>> >>> Yes but coding style explicitly forbids breaking strings. >>> >>>>> + err); >>>>> + >>>>> + return NOTIFY_OK; >>>>> +} >>>>> + >>>>> +/** >>>>> + * qos_min_notifier_call() - Callback for QoS min_freq changes. >>>>> + * @nb: Should be devfreq->nb_min >>>>> + */ >>>>> +static int qos_min_notifier_call(struct notifier_block *nb, >>>>> + unsigned long val, void *ptr) >>>>> +{ >>>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_min)); >>>>> +} >>>>> + >>>>> +/** >>>>> + * qos_max_notifier_call() - Callback for QoS max_freq changes. >>>>> + * @nb: Should be devfreq->nb_max >>>>> + */ >>>>> +static int qos_max_notifier_call(struct notifier_block *nb, >>>>> + unsigned long val, void *ptr) >>>>> +{ >>>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_max)); >>>>> +} >>>>> + >>>>> /** >>>>> * devfreq_dev_release() - Callback for struct device to release the device. >>>>> * @dev: the devfreq device >>>>> * >>>>> * Remove devfreq from the list and release its resources. >>>>> @@ -620,10 +671,15 @@ static void devfreq_dev_release(struct device *dev) >>>>> >>>>> mutex_lock(&devfreq_list_lock); >>>>> list_del(&devfreq->node); >>>>> mutex_unlock(&devfreq_list_lock); >>>>> >>>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max, >>>>> + DEV_PM_QOS_MAX_FREQUENCY); >>>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min, >>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>> + >>>> >>>> Just print error with dev_err() without stopping the release step. >>>> >>>> I prefer to handle the return value if kernel API provides >>>> the error code. >> >> How about? > > Modified to dev_warn. This also applies to PATCH 6 so I replied there. > >>>>> if (devfreq->profile->exit) >>>>> devfreq->profile->exit(devfreq->dev.parent); >>>>> >>>>> kfree(devfreq->time_in_state); >>>>> kfree(devfreq->trans_table); >>>>> @@ -733,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>>> if (err) { >>>>> put_device(&devfreq->dev); >>>>> goto err_out; >>>>> } >>>>> >>>>> + /* >>>>> + * Register notifiers for updates to min/max_freq after device is >>>>> + * initialized (and we can handle notifications) but before the >>>>> + * governor is started (which should do an initial enforcement of >>>>> + * constraints). >>>>> + */ >>>> >>>> My previous comment is not enough why I prefer to remove it. Sorry. >>>> Actually, until now, the devfreq_add_device() don't have the detailed >>>> comments because the line code is not too long. But, at the present time, >>>> devfreq_add_device() is too long. It means that the detailed comment >>>> are necessary. >>>> >>>> So, I'll add the detailed comment for each step of devfreq_add_device() >>>> on separate patch to keep the same style. I'll send the patch to you >>>> for the review. >>> >>> This is very likely to result in merge conflicts, maybe wait for my >>> series to go in first? >> >> I'll send the separate patch after finished the review of these patches. >> So, if you agree, please remove this comment on this patch. >> >> You can review the detailed comments on separate patch when I send. > This patch already contains comments and they explain the code being > added. Doesn't it make more sense for comments and code to go in together? > > I think the comment is a resonable explanation as to why notifiers are > registered at that specific step in the initialization sequence. If you add this comment on this patch, OK. just I have some comments. /* * Register notifiers for updates to min/max_freq after device is * initialized (and we can handle notifications) but before the I think that 'device is initialized' is not needed. It is always true, it don't need to add the additional comments. because dev_pm_qos_add_notifier() must need the 'devfreq->dev'. The this code prove the call sequence between them. About 'us', don't use 'we'. The subject is 'devfreq' or other device instead of us. * governor is started (which should do an initial enforcement of * constraints). */ Actually, it doesn't matter the initialization step between governor and PM_QOS registration. If start governor and then register PM_QOS, the governor can decide the new frequency. And then PM_QOS request the new constraints for min/max frequency. On request time of PM_QOS, devfreq can detect this time and then apply the constraints from PM_QOS. The user of devfreq's PM_QOS must need the phandle or device instance of devfreq deivce. It means that user of devfreq's PM_QOS can request the any constraints of PM_QOS_MIN/MAX through PM_QOS interface. So, it doesn't need to add the following comments because following comment is not always mandatory. " governor is started (which should do an initial enforcement of constraints)." Also, you don't need to use parentheses on comments. Actually, I think that following comments are enough. /* Register PM QoS notifiers for updating to min/max freq */ > >>>>> + devfreq->nb_min.notifier_call = qos_min_notifier_call; >>>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min, >>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>> + if (err) >>>>> + goto err_devfreq; >>>>> + >>>>> + devfreq->nb_max.notifier_call = qos_max_notifier_call; >>>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max, >>>>> + DEV_PM_QOS_MAX_FREQUENCY); >>>>> + if (err) >>>>> + goto err_devfreq; >>>>> + >>>>> mutex_lock(&devfreq_list_lock); >>>>> >>>>> governor = try_then_request_governor(devfreq->governor_name); >>>>> if (IS_ERR(governor)) { >>>>> dev_err(dev, "%s: Unable to find governor for the device\n", >>>>> @@ -760,10 +834,11 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>>> >>>>> return devfreq; >>>>> >>>>> err_init: >>>>> mutex_unlock(&devfreq_list_lock); >>>>> +err_devfreq: >>>>> devfreq_remove_device(devfreq); >>>>> return ERR_PTR(err); >>>>> >>>>> err_dev: >>>>> /* >>>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >>>>> index c3cbc15fdf08..dac0dffeabb4 100644 >>>>> --- a/include/linux/devfreq.h >>>>> +++ b/include/linux/devfreq.h >>>>> @@ -134,10 +134,12 @@ struct devfreq_dev_profile { >>>>> * @total_trans: Number of devfreq transitions >>>>> * @trans_table: Statistics of devfreq transitions >>>>> * @time_in_state: Statistics of devfreq states >>>>> * @last_stat_updated: The last time stat updated >>>>> * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier >>>>> + * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY >>>>> + * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY >>>>> * >>>>> * This structure stores the devfreq information for a give device. >>>>> * >>>>> * Note that when a governor accesses entries in struct devfreq in its >>>>> * functions except for the context of callbacks defined in struct >>>>> @@ -176,10 +178,13 @@ struct devfreq { >>>>> unsigned int *trans_table; >>>>> unsigned long *time_in_state; >>>>> unsigned long last_stat_updated; >>>>> >>>>> struct srcu_notifier_head transition_notifier_list; >>>>> + >>>>> + struct notifier_block nb_min; >>>>> + struct notifier_block nb_max; >>>>> }; >>>>> >>>>> struct devfreq_freqs { >>>>> unsigned long old; >>>>> unsigned long new;
On 2019-09-26 4:14 AM, Chanwoo Choi wrote: > On 19. 9. 26. 오전 6:18, Leonard Crestez wrote: >> On 25.09.2019 05:11, Chanwoo Choi wrote: >>> On 19. 9. 24. 오후 7:11, Leonard Crestez wrote: >>>> Register notifiers with the PM QoS framework in order to respond to >>>> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY. >>>> >>>> No notifiers are added by this patch but PM QoS constraints can be >>>> imposed externally (for example from other devices). >>>> >>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >>>> --- >>>> drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++ >>>> include/linux/devfreq.h | 5 +++ >>>> 2 files changed, 80 insertions(+) >>>> >>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>> index eee403e70c84..784f3e40536a 100644 >>>> --- a/drivers/devfreq/devfreq.c >>>> +++ b/drivers/devfreq/devfreq.c >>>> @@ -22,15 +22,18 @@ >>>> #include <linux/platform_device.h> >>>> #include <linux/list.h> >>>> #include <linux/printk.h> >>>> #include <linux/hrtimer.h> >>>> #include <linux/of.h> >>>> +#include <linux/pm_qos.h> >>>> #include "governor.h" >>>> >>>> #define CREATE_TRACE_POINTS >>>> #include <trace/events/devfreq.h> >>>> >>>> +#define HZ_PER_KHZ 1000 >>>> + >>>> static struct class *devfreq_class; >>>> >>>> /* >>>> * devfreq core provides delayed work based load monitoring helper >>>> * functions. Governors can use these or can implement their own >>>> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) >>>> static void get_freq_range(struct devfreq *devfreq, >>>> unsigned long *min_freq, >>>> unsigned long *max_freq) >>>> { >>>> unsigned long *freq_table = devfreq->profile->freq_table; >>>> + unsigned long qos_min_freq, qos_max_freq; >>>> >>>> lockdep_assert_held(&devfreq->lock); >>>> >>>> /* >>>> * Init min/max frequency from freq table. >>>> @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq, >>>> } else { >>>> *min_freq = freq_table[devfreq->profile->max_state - 1]; >>>> *max_freq = freq_table[0]; >>>> } >>>> >>>> + /* constraints from PM QoS */ >>> >>> As I commented on patch4, >>> 'constraints' -> 'Constraint' because first verb have to be used >>> as the sigular verbs. >> >> Already discussed for another patch; I will modify to "Apply constraints >> from PM QoS" instead. >> >>> I prefer to use following comments: >>> >>> /* Constraint minimum/maximum frequency from PM QoS constraints */ >>> >>>> + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>> + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>> + *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); >>>> + *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); >>>> + >>>> /* constraints from sysfs */ >>>> *min_freq = max(*min_freq, devfreq->min_freq); >>>> *max_freq = min(*max_freq, devfreq->max_freq); >>>> >>>> /* constraints from OPP interface */ >>>> @@ -606,10 +618,49 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, >>>> mutex_unlock(&devfreq->lock); >>>> >>>> return ret; >>>> } >>>> >>>> +/** >>>> + * qos_notifier_call() - Common handler for QoS constraints. >>>> + * @devfreq: the devfreq instance. >>>> + */ >>>> +static int qos_notifier_call(struct devfreq *devfreq) >>>> +{ >>>> + int err; >>>> + >>>> + mutex_lock(&devfreq->lock); >>>> + err = update_devfreq(devfreq); >>>> + mutex_unlock(&devfreq->lock); >>>> + if (err) >>>> + dev_err(devfreq->dev.parent, >>>> + "failed to update frequency for PM QoS constraints (%d)\n", >>> >>> Is it not over 80 char? >> >> Yes but coding style explicitly forbids breaking strings. > > I want to make it within 80 char. How about following comment? > > dev_err(devfreq->dev.parent, > "failed to update frequency from PM QoS (%d)\n", Yes, shrinking the comment and aligning with open parenthesis can keep it under 80 chars. Maybe it could be shrunk further to "failed update for PM QoS" >>>> + err); >>>> + >>>> + return NOTIFY_OK; >>>> +} >>>> + >>>> +/** >>>> + * qos_min_notifier_call() - Callback for QoS min_freq changes. >>>> + * @nb: Should be devfreq->nb_min >>>> + */ >>>> +static int qos_min_notifier_call(struct notifier_block *nb, >>>> + unsigned long val, void *ptr) >>>> +{ >>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_min)); >>>> +} >>>> + >>>> +/** >>>> + * qos_max_notifier_call() - Callback for QoS max_freq changes. >>>> + * @nb: Should be devfreq->nb_max >>>> + */ >>>> +static int qos_max_notifier_call(struct notifier_block *nb, >>>> + unsigned long val, void *ptr) >>>> +{ >>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_max)); >>>> +} >>>> + >>>> /** >>>> * devfreq_dev_release() - Callback for struct device to release the device. >>>> * @dev: the devfreq device >>>> * >>>> * Remove devfreq from the list and release its resources. >>>> @@ -620,10 +671,15 @@ static void devfreq_dev_release(struct device *dev) >>>> >>>> mutex_lock(&devfreq_list_lock); >>>> list_del(&devfreq->node); >>>> mutex_unlock(&devfreq_list_lock); >>>> >>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max, >>>> + DEV_PM_QOS_MAX_FREQUENCY); >>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min, >>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>> + >>> >>> Just print error with dev_err() without stopping the release step. >>> >>> I prefer to handle the return value if kernel API provides >>> the error code. >>> >>>> if (devfreq->profile->exit) >>>> devfreq->profile->exit(devfreq->dev.parent); >>>> >>>> kfree(devfreq->time_in_state); >>>> kfree(devfreq->trans_table); >>>> @@ -733,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>> if (err) { >>>> put_device(&devfreq->dev); >>>> goto err_out; >>>> } >>>> >>>> + /* >>>> + * Register notifiers for updates to min/max_freq after device is >>>> + * initialized (and we can handle notifications) but before the >>>> + * governor is started (which should do an initial enforcement of >>>> + * constraints). >>>> + */ >>> >>> My previous comment is not enough why I prefer to remove it. Sorry. >>> Actually, until now, the devfreq_add_device() don't have the detailed >>> comments because the line code is not too long. But, at the present time, >>> devfreq_add_device() is too long. It means that the detailed comment >>> are necessary. >>> >>> So, I'll add the detailed comment for each step of devfreq_add_device() >>> on separate patch to keep the same style. I'll send the patch to you >>> for the review. >> >> This is very likely to result in merge conflicts, maybe wait for my >> series to go in first? >> >>>> + devfreq->nb_min.notifier_call = qos_min_notifier_call; >>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min, >>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>> + if (err) >>>> + goto err_devfreq; >>>> + >>>> + devfreq->nb_max.notifier_call = qos_max_notifier_call; >>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max, >>>> + DEV_PM_QOS_MAX_FREQUENCY); >>>> + if (err) >>>> + goto err_devfreq; >>>> + >>>> mutex_lock(&devfreq_list_lock); >>>> >>>> governor = try_then_request_governor(devfreq->governor_name); >>>> if (IS_ERR(governor)) { >>>> dev_err(dev, "%s: Unable to find governor for the device\n", >>>> @@ -760,10 +834,11 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>> >>>> return devfreq; >>>> >>>> err_init: >>>> mutex_unlock(&devfreq_list_lock); >>>> +err_devfreq: >>>> devfreq_remove_device(devfreq); >>>> return ERR_PTR(err); >>>> >>>> err_dev: >>>> /* >>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >>>> index c3cbc15fdf08..dac0dffeabb4 100644 >>>> --- a/include/linux/devfreq.h >>>> +++ b/include/linux/devfreq.h >>>> @@ -134,10 +134,12 @@ struct devfreq_dev_profile { >>>> * @total_trans: Number of devfreq transitions >>>> * @trans_table: Statistics of devfreq transitions >>>> * @time_in_state: Statistics of devfreq states >>>> * @last_stat_updated: The last time stat updated >>>> * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier >>>> + * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY >>>> + * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY >>>> * >>>> * This structure stores the devfreq information for a give device. >>>> * >>>> * Note that when a governor accesses entries in struct devfreq in its >>>> * functions except for the context of callbacks defined in struct >>>> @@ -176,10 +178,13 @@ struct devfreq { >>>> unsigned int *trans_table; >>>> unsigned long *time_in_state; >>>> unsigned long last_stat_updated; >>>> >>>> struct srcu_notifier_head transition_notifier_list; >>>> + >>>> + struct notifier_block nb_min; >>>> + struct notifier_block nb_max; >>>> }; >>>> >>>> struct devfreq_freqs { >>>> unsigned long old; >>>> unsigned long new; >>>> >>> >>> >> > >
On 2019-09-27 4:45 AM, Chanwoo Choi wrote: > On 19. 9. 26. 오후 10:43, Leonard Crestez wrote: >> On 2019-09-26 4:07 AM, Chanwoo Choi wrote: >>> On 19. 9. 26. 오전 6:18, Leonard Crestez wrote: >>>> On 25.09.2019 05:11, Chanwoo Choi wrote: >>>>> On 19. 9. 24. 오후 7:11, Leonard Crestez wrote: >>>>>> Register notifiers with the PM QoS framework in order to respond to >>>>>> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY. >>>>>> >>>>>> No notifiers are added by this patch but PM QoS constraints can be >>>>>> imposed externally (for example from other devices). >>>>>> >>>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>>>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >>>>>> --- >>>>>> drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++ >>>>>> include/linux/devfreq.h | 5 +++ >>>>>> 2 files changed, 80 insertions(+) >>>>>> >>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>>>> index eee403e70c84..784f3e40536a 100644 >>>>>> --- a/drivers/devfreq/devfreq.c >>>>>> +++ b/drivers/devfreq/devfreq.c >>>>>> @@ -22,15 +22,18 @@ >>>>>> #include <linux/platform_device.h> >>>>>> #include <linux/list.h> >>>>>> #include <linux/printk.h> >>>>>> #include <linux/hrtimer.h> >>>>>> #include <linux/of.h> >>>>>> +#include <linux/pm_qos.h> >>>>>> #include "governor.h" >>>>>> >>>>>> #define CREATE_TRACE_POINTS >>>>>> #include <trace/events/devfreq.h> >>>>>> >>>>>> +#define HZ_PER_KHZ 1000 >>>>>> + >>>>>> static struct class *devfreq_class; >>>>>> >>>>>> /* >>>>>> * devfreq core provides delayed work based load monitoring helper >>>>>> * functions. Governors can use these or can implement their own >>>>>> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) >>>>>> static void get_freq_range(struct devfreq *devfreq, >>>>>> unsigned long *min_freq, >>>>>> unsigned long *max_freq) >>>>>> { >>>>>> unsigned long *freq_table = devfreq->profile->freq_table; >>>>>> + unsigned long qos_min_freq, qos_max_freq; >>>>>> >>>>>> lockdep_assert_held(&devfreq->lock); >>>>>> >>>>>> /* >>>>>> * Init min/max frequency from freq table. >>>>>> @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq, >>>>>> } else { >>>>>> *min_freq = freq_table[devfreq->profile->max_state - 1]; >>>>>> *max_freq = freq_table[0]; >>>>>> } >>>>>> >>>>>> + /* constraints from PM QoS */ >>>>> >>>>> As I commented on patch4, >>>>> 'constraints' -> 'Constraint' because first verb have to be used >>>>> as the sigular verbs. >>>> >>>> Already discussed for another patch; I will modify to "Apply constraints >>>> from PM QoS" instead. >>> >>> I agree the new comment with 'Apply constraints ... '. >>> >>>> >>>>> I prefer to use following comments: >>>>> >>>>> /* Constraint minimum/maximum frequency from PM QoS constraints */ >>>>> >>>>>> + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>>> + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>>> + *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); >>>>>> + *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); >>>>>> + >>>>>> /* constraints from sysfs */ >>>>>> *min_freq = max(*min_freq, devfreq->min_freq); >>>>>> *max_freq = min(*max_freq, devfreq->max_freq); >>>>>> >>>>>> /* constraints from OPP interface */ >>>>>> @@ -606,10 +618,49 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, >>>>>> mutex_unlock(&devfreq->lock); >>>>>> >>>>>> return ret; >>>>>> } >>>>>> >>>>>> +/** >>>>>> + * qos_notifier_call() - Common handler for QoS constraints. >>>>>> + * @devfreq: the devfreq instance. >>>>>> + */ >>>>>> +static int qos_notifier_call(struct devfreq *devfreq) >>>>>> +{ >>>>>> + int err; >>>>>> + >>>>>> + mutex_lock(&devfreq->lock); >>>>>> + err = update_devfreq(devfreq); >>>>>> + mutex_unlock(&devfreq->lock); >>>>>> + if (err) >>>>>> + dev_err(devfreq->dev.parent, >>>>>> + "failed to update frequency for PM QoS constraints (%d)\n", >>>>> >>>>> Is it not over 80 char? >>>> >>>> Yes but coding style explicitly forbids breaking strings. >>>> >>>>>> + err); >>>>>> + >>>>>> + return NOTIFY_OK; >>>>>> +} >>>>>> + >>>>>> +/** >>>>>> + * qos_min_notifier_call() - Callback for QoS min_freq changes. >>>>>> + * @nb: Should be devfreq->nb_min >>>>>> + */ >>>>>> +static int qos_min_notifier_call(struct notifier_block *nb, >>>>>> + unsigned long val, void *ptr) >>>>>> +{ >>>>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_min)); >>>>>> +} >>>>>> + >>>>>> +/** >>>>>> + * qos_max_notifier_call() - Callback for QoS max_freq changes. >>>>>> + * @nb: Should be devfreq->nb_max >>>>>> + */ >>>>>> +static int qos_max_notifier_call(struct notifier_block *nb, >>>>>> + unsigned long val, void *ptr) >>>>>> +{ >>>>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_max)); >>>>>> +} >>>>>> + >>>>>> /** >>>>>> * devfreq_dev_release() - Callback for struct device to release the device. >>>>>> * @dev: the devfreq device >>>>>> * >>>>>> * Remove devfreq from the list and release its resources. >>>>>> @@ -620,10 +671,15 @@ static void devfreq_dev_release(struct device *dev) >>>>>> >>>>>> mutex_lock(&devfreq_list_lock); >>>>>> list_del(&devfreq->node); >>>>>> mutex_unlock(&devfreq_list_lock); >>>>>> >>>>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max, >>>>>> + DEV_PM_QOS_MAX_FREQUENCY); >>>>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min, >>>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>>> + >>>>> >>>>> Just print error with dev_err() without stopping the release step. >>>>> >>>>> I prefer to handle the return value if kernel API provides >>>>> the error code. >>> >>> How about? >> >> Modified to dev_warn. This also applies to PATCH 6 so I replied there. >> >>>>>> if (devfreq->profile->exit) >>>>>> devfreq->profile->exit(devfreq->dev.parent); >>>>>> >>>>>> kfree(devfreq->time_in_state); >>>>>> kfree(devfreq->trans_table); >>>>>> @@ -733,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>>>> if (err) { >>>>>> put_device(&devfreq->dev); >>>>>> goto err_out; >>>>>> } >>>>>> >>>>>> + /* >>>>>> + * Register notifiers for updates to min/max_freq after device is >>>>>> + * initialized (and we can handle notifications) but before the >>>>>> + * governor is started (which should do an initial enforcement of >>>>>> + * constraints). >>>>>> + */ >>>>> >>>>> My previous comment is not enough why I prefer to remove it. Sorry. >>>>> Actually, until now, the devfreq_add_device() don't have the detailed >>>>> comments because the line code is not too long. But, at the present time, >>>>> devfreq_add_device() is too long. It means that the detailed comment >>>>> are necessary. >>>>> >>>>> So, I'll add the detailed comment for each step of devfreq_add_device() >>>>> on separate patch to keep the same style. I'll send the patch to you >>>>> for the review. >>>> >>>> This is very likely to result in merge conflicts, maybe wait for my >>>> series to go in first? >>> >>> I'll send the separate patch after finished the review of these patches. >>> So, if you agree, please remove this comment on this patch. >>> >>> You can review the detailed comments on separate patch when I send. >> This patch already contains comments and they explain the code being >> added. Doesn't it make more sense for comments and code to go in together? >> >> I think the comment is a resonable explanation as to why notifiers are >> registered at that specific step in the initialization sequence. > > If you add this comment on this patch, OK. just I have some comments. > > /* > * Register notifiers for updates to min/max_freq after device is > * initialized (and we can handle notifications) but before the > > I think that 'device is initialized' is not needed. > It is always true, it don't need to add the additional comments. > because dev_pm_qos_add_notifier() must need the 'devfreq->dev'. > The this code prove the call sequence between them. In theory if a notifier is registered too soon then it could crash on a NULL pointer. But looking at the code it first checks that "governor != NULL) so it would be harmless. > > About 'us', don't use 'we'. The subject is 'devfreq' or other device instead of us. > > * governor is started (which should do an initial enforcement of > * constraints). > */ > > Actually, it doesn't matter the initialization step between governor > and PM_QOS registration. In theory PM QoS constraints could be modified between governor startup and notifier registration and that update would be lost (until the next one). > If start governor and then register PM_QOS, > the governor can decide the new frequency. And then PM_QOS request > the new constraints for min/max frequency. On request time of PM_QOS, > devfreq can detect this time and then apply the constraints from PM_QOS. > The user of devfreq's PM_QOS must need the phandle or device instance > of devfreq deivce. It means that user of devfreq's PM_QOS can request > the any constraints of PM_QOS_MIN/MAX through PM_QOS interface. > > So, it doesn't need to add the following comments > because following comment is not always mandatory. > " governor is started (which should do an initial enforcement of constraints)." > > Also, you don't need to use parentheses on comments. > > Actually, I think that following comments are enough. > > /* Register PM QoS notifiers for updating to min/max freq */ I thought that explaning why it's done at this particular step but I'll just remove it, devfreq_add_device can get more comments later. >>>>>> + devfreq->nb_min.notifier_call = qos_min_notifier_call; >>>>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min, >>>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>>> + if (err) >>>>>> + goto err_devfreq; >>>>>> + >>>>>> + devfreq->nb_max.notifier_call = qos_max_notifier_call; >>>>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max, >>>>>> + DEV_PM_QOS_MAX_FREQUENCY); >>>>>> + if (err) >>>>>> + goto err_devfreq; >>>>>> + >>>>>> mutex_lock(&devfreq_list_lock); >>>>>> >>>>>> governor = try_then_request_governor(devfreq->governor_name); >>>>>> if (IS_ERR(governor)) { >>>>>> dev_err(dev, "%s: Unable to find governor for the device\n", >>>>>> @@ -760,10 +834,11 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>>>> >>>>>> return devfreq; >>>>>> >>>>>> err_init: >>>>>> mutex_unlock(&devfreq_list_lock); >>>>>> +err_devfreq: >>>>>> devfreq_remove_device(devfreq); >>>>>> return ERR_PTR(err); >>>>>> >>>>>> err_dev: >>>>>> /* >>>>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >>>>>> index c3cbc15fdf08..dac0dffeabb4 100644 >>>>>> --- a/include/linux/devfreq.h >>>>>> +++ b/include/linux/devfreq.h >>>>>> @@ -134,10 +134,12 @@ struct devfreq_dev_profile { >>>>>> * @total_trans: Number of devfreq transitions >>>>>> * @trans_table: Statistics of devfreq transitions >>>>>> * @time_in_state: Statistics of devfreq states >>>>>> * @last_stat_updated: The last time stat updated >>>>>> * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier >>>>>> + * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY >>>>>> + * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY >>>>>> * >>>>>> * This structure stores the devfreq information for a give device. >>>>>> * >>>>>> * Note that when a governor accesses entries in struct devfreq in its >>>>>> * functions except for the context of callbacks defined in struct >>>>>> @@ -176,10 +178,13 @@ struct devfreq { >>>>>> unsigned int *trans_table; >>>>>> unsigned long *time_in_state; >>>>>> unsigned long last_stat_updated; >>>>>> >>>>>> struct srcu_notifier_head transition_notifier_list; >>>>>> + >>>>>> + struct notifier_block nb_min; >>>>>> + struct notifier_block nb_max; >>>>>> }; >>>>>> >>>>>> struct devfreq_freqs { >>>>>> unsigned long old; >>>>>> unsigned long new; > >
On 19. 9. 30. 오후 9:43, Leonard Crestez wrote: > On 2019-09-26 4:14 AM, Chanwoo Choi wrote: >> On 19. 9. 26. 오전 6:18, Leonard Crestez wrote: >>> On 25.09.2019 05:11, Chanwoo Choi wrote: >>>> On 19. 9. 24. 오후 7:11, Leonard Crestez wrote: >>>>> Register notifiers with the PM QoS framework in order to respond to >>>>> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY. >>>>> >>>>> No notifiers are added by this patch but PM QoS constraints can be >>>>> imposed externally (for example from other devices). >>>>> >>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >>>>> --- >>>>> drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++ >>>>> include/linux/devfreq.h | 5 +++ >>>>> 2 files changed, 80 insertions(+) >>>>> >>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>>> index eee403e70c84..784f3e40536a 100644 >>>>> --- a/drivers/devfreq/devfreq.c >>>>> +++ b/drivers/devfreq/devfreq.c >>>>> @@ -22,15 +22,18 @@ >>>>> #include <linux/platform_device.h> >>>>> #include <linux/list.h> >>>>> #include <linux/printk.h> >>>>> #include <linux/hrtimer.h> >>>>> #include <linux/of.h> >>>>> +#include <linux/pm_qos.h> >>>>> #include "governor.h" >>>>> >>>>> #define CREATE_TRACE_POINTS >>>>> #include <trace/events/devfreq.h> >>>>> >>>>> +#define HZ_PER_KHZ 1000 >>>>> + >>>>> static struct class *devfreq_class; >>>>> >>>>> /* >>>>> * devfreq core provides delayed work based load monitoring helper >>>>> * functions. Governors can use these or can implement their own >>>>> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) >>>>> static void get_freq_range(struct devfreq *devfreq, >>>>> unsigned long *min_freq, >>>>> unsigned long *max_freq) >>>>> { >>>>> unsigned long *freq_table = devfreq->profile->freq_table; >>>>> + unsigned long qos_min_freq, qos_max_freq; >>>>> >>>>> lockdep_assert_held(&devfreq->lock); >>>>> >>>>> /* >>>>> * Init min/max frequency from freq table. >>>>> @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq, >>>>> } else { >>>>> *min_freq = freq_table[devfreq->profile->max_state - 1]; >>>>> *max_freq = freq_table[0]; >>>>> } >>>>> >>>>> + /* constraints from PM QoS */ >>>> >>>> As I commented on patch4, >>>> 'constraints' -> 'Constraint' because first verb have to be used >>>> as the sigular verbs. >>> >>> Already discussed for another patch; I will modify to "Apply constraints >>> from PM QoS" instead. >>> >>>> I prefer to use following comments: >>>> >>>> /* Constraint minimum/maximum frequency from PM QoS constraints */ >>>> >>>>> + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>> + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>> + *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); >>>>> + *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); >>>>> + >>>>> /* constraints from sysfs */ >>>>> *min_freq = max(*min_freq, devfreq->min_freq); >>>>> *max_freq = min(*max_freq, devfreq->max_freq); >>>>> >>>>> /* constraints from OPP interface */ >>>>> @@ -606,10 +618,49 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, >>>>> mutex_unlock(&devfreq->lock); >>>>> >>>>> return ret; >>>>> } >>>>> >>>>> +/** >>>>> + * qos_notifier_call() - Common handler for QoS constraints. >>>>> + * @devfreq: the devfreq instance. >>>>> + */ >>>>> +static int qos_notifier_call(struct devfreq *devfreq) >>>>> +{ >>>>> + int err; >>>>> + >>>>> + mutex_lock(&devfreq->lock); >>>>> + err = update_devfreq(devfreq); >>>>> + mutex_unlock(&devfreq->lock); >>>>> + if (err) >>>>> + dev_err(devfreq->dev.parent, >>>>> + "failed to update frequency for PM QoS constraints (%d)\n", >>>> >>>> Is it not over 80 char? >>> >>> Yes but coding style explicitly forbids breaking strings. >> >> I want to make it within 80 char. How about following comment? >> >> dev_err(devfreq->dev.parent, >> "failed to update frequency from PM QoS (%d)\n", > > Yes, shrinking the comment and aligning with open parenthesis can keep > it under 80 chars. Maybe it could be shrunk further to > > "failed update for PM QoS" I think that we need to specify what do update something like 'frequency'. So, it is more proper as following: "failed to update frequency from PM QoS\n", > >>>>> + err); >>>>> + >>>>> + return NOTIFY_OK; >>>>> +} >>>>> + >>>>> +/** >>>>> + * qos_min_notifier_call() - Callback for QoS min_freq changes. >>>>> + * @nb: Should be devfreq->nb_min >>>>> + */ >>>>> +static int qos_min_notifier_call(struct notifier_block *nb, >>>>> + unsigned long val, void *ptr) >>>>> +{ >>>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_min)); >>>>> +} >>>>> + >>>>> +/** >>>>> + * qos_max_notifier_call() - Callback for QoS max_freq changes. >>>>> + * @nb: Should be devfreq->nb_max >>>>> + */ >>>>> +static int qos_max_notifier_call(struct notifier_block *nb, >>>>> + unsigned long val, void *ptr) >>>>> +{ >>>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_max)); >>>>> +} >>>>> + >>>>> /** >>>>> * devfreq_dev_release() - Callback for struct device to release the device. >>>>> * @dev: the devfreq device >>>>> * >>>>> * Remove devfreq from the list and release its resources. >>>>> @@ -620,10 +671,15 @@ static void devfreq_dev_release(struct device *dev) >>>>> >>>>> mutex_lock(&devfreq_list_lock); >>>>> list_del(&devfreq->node); >>>>> mutex_unlock(&devfreq_list_lock); >>>>> >>>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max, >>>>> + DEV_PM_QOS_MAX_FREQUENCY); >>>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min, >>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>> + >>>> >>>> Just print error with dev_err() without stopping the release step. >>>> >>>> I prefer to handle the return value if kernel API provides >>>> the error code. >>>> >>>>> if (devfreq->profile->exit) >>>>> devfreq->profile->exit(devfreq->dev.parent); >>>>> >>>>> kfree(devfreq->time_in_state); >>>>> kfree(devfreq->trans_table); >>>>> @@ -733,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>>> if (err) { >>>>> put_device(&devfreq->dev); >>>>> goto err_out; >>>>> } >>>>> >>>>> + /* >>>>> + * Register notifiers for updates to min/max_freq after device is >>>>> + * initialized (and we can handle notifications) but before the >>>>> + * governor is started (which should do an initial enforcement of >>>>> + * constraints). >>>>> + */ >>>> >>>> My previous comment is not enough why I prefer to remove it. Sorry. >>>> Actually, until now, the devfreq_add_device() don't have the detailed >>>> comments because the line code is not too long. But, at the present time, >>>> devfreq_add_device() is too long. It means that the detailed comment >>>> are necessary. >>>> >>>> So, I'll add the detailed comment for each step of devfreq_add_device() >>>> on separate patch to keep the same style. I'll send the patch to you >>>> for the review. >>> >>> This is very likely to result in merge conflicts, maybe wait for my >>> series to go in first? >>> >>>>> + devfreq->nb_min.notifier_call = qos_min_notifier_call; >>>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min, >>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>> + if (err) >>>>> + goto err_devfreq; >>>>> + >>>>> + devfreq->nb_max.notifier_call = qos_max_notifier_call; >>>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max, >>>>> + DEV_PM_QOS_MAX_FREQUENCY); >>>>> + if (err) >>>>> + goto err_devfreq; >>>>> + >>>>> mutex_lock(&devfreq_list_lock); >>>>> >>>>> governor = try_then_request_governor(devfreq->governor_name); >>>>> if (IS_ERR(governor)) { >>>>> dev_err(dev, "%s: Unable to find governor for the device\n", >>>>> @@ -760,10 +834,11 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>>> >>>>> return devfreq; >>>>> >>>>> err_init: >>>>> mutex_unlock(&devfreq_list_lock); >>>>> +err_devfreq: >>>>> devfreq_remove_device(devfreq); >>>>> return ERR_PTR(err); >>>>> >>>>> err_dev: >>>>> /* >>>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >>>>> index c3cbc15fdf08..dac0dffeabb4 100644 >>>>> --- a/include/linux/devfreq.h >>>>> +++ b/include/linux/devfreq.h >>>>> @@ -134,10 +134,12 @@ struct devfreq_dev_profile { >>>>> * @total_trans: Number of devfreq transitions >>>>> * @trans_table: Statistics of devfreq transitions >>>>> * @time_in_state: Statistics of devfreq states >>>>> * @last_stat_updated: The last time stat updated >>>>> * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier >>>>> + * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY >>>>> + * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY >>>>> * >>>>> * This structure stores the devfreq information for a give device. >>>>> * >>>>> * Note that when a governor accesses entries in struct devfreq in its >>>>> * functions except for the context of callbacks defined in struct >>>>> @@ -176,10 +178,13 @@ struct devfreq { >>>>> unsigned int *trans_table; >>>>> unsigned long *time_in_state; >>>>> unsigned long last_stat_updated; >>>>> >>>>> struct srcu_notifier_head transition_notifier_list; >>>>> + >>>>> + struct notifier_block nb_min; >>>>> + struct notifier_block nb_max; >>>>> }; >>>>> >>>>> struct devfreq_freqs { >>>>> unsigned long old; >>>>> unsigned long new; >>>>> >>>> >>>> >>> >> >> >
Hi, On 19. 9. 30. 오후 10:16, Leonard Crestez wrote: > On 2019-09-27 4:45 AM, Chanwoo Choi wrote: >> On 19. 9. 26. 오후 10:43, Leonard Crestez wrote: >>> On 2019-09-26 4:07 AM, Chanwoo Choi wrote: >>>> On 19. 9. 26. 오전 6:18, Leonard Crestez wrote: >>>>> On 25.09.2019 05:11, Chanwoo Choi wrote: >>>>>> On 19. 9. 24. 오후 7:11, Leonard Crestez wrote: >>>>>>> Register notifiers with the PM QoS framework in order to respond to >>>>>>> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY. >>>>>>> >>>>>>> No notifiers are added by this patch but PM QoS constraints can be >>>>>>> imposed externally (for example from other devices). >>>>>>> >>>>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>>>>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >>>>>>> --- >>>>>>> drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++ >>>>>>> include/linux/devfreq.h | 5 +++ >>>>>>> 2 files changed, 80 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>>>>> index eee403e70c84..784f3e40536a 100644 >>>>>>> --- a/drivers/devfreq/devfreq.c >>>>>>> +++ b/drivers/devfreq/devfreq.c >>>>>>> @@ -22,15 +22,18 @@ >>>>>>> #include <linux/platform_device.h> >>>>>>> #include <linux/list.h> >>>>>>> #include <linux/printk.h> >>>>>>> #include <linux/hrtimer.h> >>>>>>> #include <linux/of.h> >>>>>>> +#include <linux/pm_qos.h> >>>>>>> #include "governor.h" >>>>>>> >>>>>>> #define CREATE_TRACE_POINTS >>>>>>> #include <trace/events/devfreq.h> >>>>>>> >>>>>>> +#define HZ_PER_KHZ 1000 >>>>>>> + >>>>>>> static struct class *devfreq_class; >>>>>>> >>>>>>> /* >>>>>>> * devfreq core provides delayed work based load monitoring helper >>>>>>> * functions. Governors can use these or can implement their own >>>>>>> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) >>>>>>> static void get_freq_range(struct devfreq *devfreq, >>>>>>> unsigned long *min_freq, >>>>>>> unsigned long *max_freq) >>>>>>> { >>>>>>> unsigned long *freq_table = devfreq->profile->freq_table; >>>>>>> + unsigned long qos_min_freq, qos_max_freq; >>>>>>> >>>>>>> lockdep_assert_held(&devfreq->lock); >>>>>>> >>>>>>> /* >>>>>>> * Init min/max frequency from freq table. >>>>>>> @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq, >>>>>>> } else { >>>>>>> *min_freq = freq_table[devfreq->profile->max_state - 1]; >>>>>>> *max_freq = freq_table[0]; >>>>>>> } >>>>>>> >>>>>>> + /* constraints from PM QoS */ >>>>>> >>>>>> As I commented on patch4, >>>>>> 'constraints' -> 'Constraint' because first verb have to be used >>>>>> as the sigular verbs. >>>>> >>>>> Already discussed for another patch; I will modify to "Apply constraints >>>>> from PM QoS" instead. >>>> >>>> I agree the new comment with 'Apply constraints ... '. >>>> >>>>> >>>>>> I prefer to use following comments: >>>>>> >>>>>> /* Constraint minimum/maximum frequency from PM QoS constraints */ >>>>>> >>>>>>> + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>>>> + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>>>> + *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); >>>>>>> + *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); >>>>>>> + >>>>>>> /* constraints from sysfs */ >>>>>>> *min_freq = max(*min_freq, devfreq->min_freq); >>>>>>> *max_freq = min(*max_freq, devfreq->max_freq); >>>>>>> >>>>>>> /* constraints from OPP interface */ >>>>>>> @@ -606,10 +618,49 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, >>>>>>> mutex_unlock(&devfreq->lock); >>>>>>> >>>>>>> return ret; >>>>>>> } >>>>>>> >>>>>>> +/** >>>>>>> + * qos_notifier_call() - Common handler for QoS constraints. >>>>>>> + * @devfreq: the devfreq instance. >>>>>>> + */ >>>>>>> +static int qos_notifier_call(struct devfreq *devfreq) >>>>>>> +{ >>>>>>> + int err; >>>>>>> + >>>>>>> + mutex_lock(&devfreq->lock); >>>>>>> + err = update_devfreq(devfreq); >>>>>>> + mutex_unlock(&devfreq->lock); >>>>>>> + if (err) >>>>>>> + dev_err(devfreq->dev.parent, >>>>>>> + "failed to update frequency for PM QoS constraints (%d)\n", >>>>>> >>>>>> Is it not over 80 char? >>>>> >>>>> Yes but coding style explicitly forbids breaking strings. >>>>> >>>>>>> + err); >>>>>>> + >>>>>>> + return NOTIFY_OK; >>>>>>> +} >>>>>>> + >>>>>>> +/** >>>>>>> + * qos_min_notifier_call() - Callback for QoS min_freq changes. >>>>>>> + * @nb: Should be devfreq->nb_min >>>>>>> + */ >>>>>>> +static int qos_min_notifier_call(struct notifier_block *nb, >>>>>>> + unsigned long val, void *ptr) >>>>>>> +{ >>>>>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_min)); >>>>>>> +} >>>>>>> + >>>>>>> +/** >>>>>>> + * qos_max_notifier_call() - Callback for QoS max_freq changes. >>>>>>> + * @nb: Should be devfreq->nb_max >>>>>>> + */ >>>>>>> +static int qos_max_notifier_call(struct notifier_block *nb, >>>>>>> + unsigned long val, void *ptr) >>>>>>> +{ >>>>>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_max)); >>>>>>> +} >>>>>>> + >>>>>>> /** >>>>>>> * devfreq_dev_release() - Callback for struct device to release the device. >>>>>>> * @dev: the devfreq device >>>>>>> * >>>>>>> * Remove devfreq from the list and release its resources. >>>>>>> @@ -620,10 +671,15 @@ static void devfreq_dev_release(struct device *dev) >>>>>>> >>>>>>> mutex_lock(&devfreq_list_lock); >>>>>>> list_del(&devfreq->node); >>>>>>> mutex_unlock(&devfreq_list_lock); >>>>>>> >>>>>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max, >>>>>>> + DEV_PM_QOS_MAX_FREQUENCY); >>>>>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min, >>>>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>>>> + >>>>>> >>>>>> Just print error with dev_err() without stopping the release step. >>>>>> >>>>>> I prefer to handle the return value if kernel API provides >>>>>> the error code. >>>> >>>> How about? >>> >>> Modified to dev_warn. This also applies to PATCH 6 so I replied there. >>> >>>>>>> if (devfreq->profile->exit) >>>>>>> devfreq->profile->exit(devfreq->dev.parent); >>>>>>> >>>>>>> kfree(devfreq->time_in_state); >>>>>>> kfree(devfreq->trans_table); >>>>>>> @@ -733,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>>>>> if (err) { >>>>>>> put_device(&devfreq->dev); >>>>>>> goto err_out; >>>>>>> } >>>>>>> >>>>>>> + /* >>>>>>> + * Register notifiers for updates to min/max_freq after device is >>>>>>> + * initialized (and we can handle notifications) but before the >>>>>>> + * governor is started (which should do an initial enforcement of >>>>>>> + * constraints). >>>>>>> + */ >>>>>> >>>>>> My previous comment is not enough why I prefer to remove it. Sorry. >>>>>> Actually, until now, the devfreq_add_device() don't have the detailed >>>>>> comments because the line code is not too long. But, at the present time, >>>>>> devfreq_add_device() is too long. It means that the detailed comment >>>>>> are necessary. >>>>>> >>>>>> So, I'll add the detailed comment for each step of devfreq_add_device() >>>>>> on separate patch to keep the same style. I'll send the patch to you >>>>>> for the review. >>>>> >>>>> This is very likely to result in merge conflicts, maybe wait for my >>>>> series to go in first? >>>> >>>> I'll send the separate patch after finished the review of these patches. >>>> So, if you agree, please remove this comment on this patch. >>>> >>>> You can review the detailed comments on separate patch when I send. >>> This patch already contains comments and they explain the code being >>> added. Doesn't it make more sense for comments and code to go in together? >>> >>> I think the comment is a resonable explanation as to why notifiers are >>> registered at that specific step in the initialization sequence. >> >> If you add this comment on this patch, OK. just I have some comments. >> >> /* >> * Register notifiers for updates to min/max_freq after device is >> * initialized (and we can handle notifications) but before the >> >> I think that 'device is initialized' is not needed. >> It is always true, it don't need to add the additional comments. >> because dev_pm_qos_add_notifier() must need the 'devfreq->dev'. >> The this code prove the call sequence between them. > > In theory if a notifier is registered too soon then it could crash on a > NULL pointer. But looking at the code it first checks that "governor != > NULL) so it would be harmless. > >> >> About 'us', don't use 'we'. The subject is 'devfreq' or other device instead of us. >> >> * governor is started (which should do an initial enforcement of >> * constraints). >> */ >> >> Actually, it doesn't matter the initialization step between governor >> and PM_QOS registration. > > In theory PM QoS constraints could be modified between governor startup > and notifier registration and that update would be lost (until the next > one). Don't lose the any of PM QoS request. User can request the any frequency through PMQoS at the any time and then devfreq consider the constraints from PM QoS. Also, after finished the registration of devfreq device, the user (other device driver required the min/max freq) can request the PM QoS on real use-case. It is impossible to get the devfreq instance before finished the devfreq_add_device() because the user can access the devfreq instance devfreq_get_devfreq_by_phandle() which use 'devfreq_list'. If devfreq_add_device() is not finished, the user cannot get the any devfreq device from 'devfreq_list'. > >> If start governor and then register PM_QOS, >> the governor can decide the new frequency. And then PM_QOS request >> the new constraints for min/max frequency. On request time of PM_QOS, >> devfreq can detect this time and then apply the constraints from PM_QOS. >> The user of devfreq's PM_QOS must need the phandle or device instance >> of devfreq deivce. It means that user of devfreq's PM_QOS can request >> the any constraints of PM_QOS_MIN/MAX through PM_QOS interface. >> >> So, it doesn't need to add the following comments >> because following comment is not always mandatory. >> " governor is started (which should do an initial enforcement of constraints)." >> >> Also, you don't need to use parentheses on comments. >> >> Actually, I think that following comments are enough. >> >> /* Register PM QoS notifiers for updating to min/max freq */ > > I thought that explaning why it's done at this particular step but I'll > just remove it, devfreq_add_device can get more comments later. OK. Thanks. I think that if the comment contains what is meaning of the code, it is enough. Although change the sequence between dev_pm_qos_add_notifier() and try_then_request_governor(), in the real-case, the any issue would not happen. Because as I commented above, the other device-driver cannot get the devfreq instance before finished the devfreq_add_device. It means that the user device driver cannot request the any PM QoS constraints. > >>>>>>> + devfreq->nb_min.notifier_call = qos_min_notifier_call; >>>>>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min, >>>>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>>>> + if (err) >>>>>>> + goto err_devfreq; >>>>>>> + >>>>>>> + devfreq->nb_max.notifier_call = qos_max_notifier_call; >>>>>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max, >>>>>>> + DEV_PM_QOS_MAX_FREQUENCY); >>>>>>> + if (err) >>>>>>> + goto err_devfreq; >>>>>>> + >>>>>>> mutex_lock(&devfreq_list_lock); >>>>>>> >>>>>>> governor = try_then_request_governor(devfreq->governor_name); >>>>>>> if (IS_ERR(governor)) { >>>>>>> dev_err(dev, "%s: Unable to find governor for the device\n", >>>>>>> @@ -760,10 +834,11 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>>>>> >>>>>>> return devfreq; >>>>>>> >>>>>>> err_init: >>>>>>> mutex_unlock(&devfreq_list_lock); >>>>>>> +err_devfreq: >>>>>>> devfreq_remove_device(devfreq); >>>>>>> return ERR_PTR(err); >>>>>>> >>>>>>> err_dev: >>>>>>> /* >>>>>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >>>>>>> index c3cbc15fdf08..dac0dffeabb4 100644 >>>>>>> --- a/include/linux/devfreq.h >>>>>>> +++ b/include/linux/devfreq.h >>>>>>> @@ -134,10 +134,12 @@ struct devfreq_dev_profile { >>>>>>> * @total_trans: Number of devfreq transitions >>>>>>> * @trans_table: Statistics of devfreq transitions >>>>>>> * @time_in_state: Statistics of devfreq states >>>>>>> * @last_stat_updated: The last time stat updated >>>>>>> * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier >>>>>>> + * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY >>>>>>> + * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY >>>>>>> * >>>>>>> * This structure stores the devfreq information for a give device. >>>>>>> * >>>>>>> * Note that when a governor accesses entries in struct devfreq in its >>>>>>> * functions except for the context of callbacks defined in struct >>>>>>> @@ -176,10 +178,13 @@ struct devfreq { >>>>>>> unsigned int *trans_table; >>>>>>> unsigned long *time_in_state; >>>>>>> unsigned long last_stat_updated; >>>>>>> >>>>>>> struct srcu_notifier_head transition_notifier_list; >>>>>>> + >>>>>>> + struct notifier_block nb_min; >>>>>>> + struct notifier_block nb_max; >>>>>>> }; >>>>>>> >>>>>>> struct devfreq_freqs { >>>>>>> unsigned long old; >>>>>>> unsigned long new; >> >> >
On 2019-10-01 12:37 AM, Chanwoo Choi wrote: > Hi, > > On 19. 9. 30. 오후 10:16, Leonard Crestez wrote: >> On 2019-09-27 4:45 AM, Chanwoo Choi wrote: >>> On 19. 9. 26. 오후 10:43, Leonard Crestez wrote: >>>> On 2019-09-26 4:07 AM, Chanwoo Choi wrote: >>>>> On 19. 9. 26. 오전 6:18, Leonard Crestez wrote: >>>>>> On 25.09.2019 05:11, Chanwoo Choi wrote: >>>>>>> On 19. 9. 24. 오후 7:11, Leonard Crestez wrote: >>>>>>>> Register notifiers with the PM QoS framework in order to respond to >>>>>>>> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY. >>>>>>>> >>>>>>>> No notifiers are added by this patch but PM QoS constraints can be >>>>>>>> imposed externally (for example from other devices). >>>>>>>> >>>>>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>>>>>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >>>>>>>> --- >>>>>>>> drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++ >>>>>>>> include/linux/devfreq.h | 5 +++ >>>>>>>> 2 files changed, 80 insertions(+) >>>>>>>> >>>>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>>>>>> index eee403e70c84..784f3e40536a 100644 >>>>>>>> --- a/drivers/devfreq/devfreq.c >>>>>>>> +++ b/drivers/devfreq/devfreq.c >>>>>>>> @@ -22,15 +22,18 @@ >>>>>>>> #include <linux/platform_device.h> >>>>>>>> #include <linux/list.h> >>>>>>>> #include <linux/printk.h> >>>>>>>> #include <linux/hrtimer.h> >>>>>>>> #include <linux/of.h> >>>>>>>> +#include <linux/pm_qos.h> >>>>>>>> #include "governor.h" >>>>>>>> >>>>>>>> #define CREATE_TRACE_POINTS >>>>>>>> #include <trace/events/devfreq.h> >>>>>>>> >>>>>>>> +#define HZ_PER_KHZ 1000 >>>>>>>> + >>>>>>>> static struct class *devfreq_class; >>>>>>>> >>>>>>>> /* >>>>>>>> * devfreq core provides delayed work based load monitoring helper >>>>>>>> * functions. Governors can use these or can implement their own >>>>>>>> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) >>>>>>>> static void get_freq_range(struct devfreq *devfreq, >>>>>>>> unsigned long *min_freq, >>>>>>>> unsigned long *max_freq) >>>>>>>> { >>>>>>>> unsigned long *freq_table = devfreq->profile->freq_table; >>>>>>>> + unsigned long qos_min_freq, qos_max_freq; >>>>>>>> >>>>>>>> lockdep_assert_held(&devfreq->lock); >>>>>>>> >>>>>>>> /* >>>>>>>> * Init min/max frequency from freq table. >>>>>>>> @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq, >>>>>>>> } else { >>>>>>>> *min_freq = freq_table[devfreq->profile->max_state - 1]; >>>>>>>> *max_freq = freq_table[0]; >>>>>>>> } >>>>>>>> >>>>>>>> + /* constraints from PM QoS */ >>>>>>> >>>>>>> As I commented on patch4, >>>>>>> 'constraints' -> 'Constraint' because first verb have to be used >>>>>>> as the sigular verbs. >>>>>> >>>>>> Already discussed for another patch; I will modify to "Apply constraints >>>>>> from PM QoS" instead. >>>>> >>>>> I agree the new comment with 'Apply constraints ... '. >>>>> >>>>>> >>>>>>> I prefer to use following comments: >>>>>>> >>>>>>> /* Constraint minimum/maximum frequency from PM QoS constraints */ >>>>>>> >>>>>>>> + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>>>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>>>>> + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>>>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>>>>> + *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); >>>>>>>> + *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); >>>>>>>> + >>>>>>>> /* constraints from sysfs */ >>>>>>>> *min_freq = max(*min_freq, devfreq->min_freq); >>>>>>>> *max_freq = min(*max_freq, devfreq->max_freq); >>>>>>>> >>>>>>>> /* constraints from OPP interface */ >>>>>>>> @@ -606,10 +618,49 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, >>>>>>>> mutex_unlock(&devfreq->lock); >>>>>>>> >>>>>>>> return ret; >>>>>>>> } >>>>>>>> >>>>>>>> +/** >>>>>>>> + * qos_notifier_call() - Common handler for QoS constraints. >>>>>>>> + * @devfreq: the devfreq instance. >>>>>>>> + */ >>>>>>>> +static int qos_notifier_call(struct devfreq *devfreq) >>>>>>>> +{ >>>>>>>> + int err; >>>>>>>> + >>>>>>>> + mutex_lock(&devfreq->lock); >>>>>>>> + err = update_devfreq(devfreq); >>>>>>>> + mutex_unlock(&devfreq->lock); >>>>>>>> + if (err) >>>>>>>> + dev_err(devfreq->dev.parent, >>>>>>>> + "failed to update frequency for PM QoS constraints (%d)\n", >>>>>>> >>>>>>> Is it not over 80 char? >>>>>> >>>>>> Yes but coding style explicitly forbids breaking strings. >>>>>> >>>>>>>> + err); >>>>>>>> + >>>>>>>> + return NOTIFY_OK; >>>>>>>> +} >>>>>>>> + >>>>>>>> +/** >>>>>>>> + * qos_min_notifier_call() - Callback for QoS min_freq changes. >>>>>>>> + * @nb: Should be devfreq->nb_min >>>>>>>> + */ >>>>>>>> +static int qos_min_notifier_call(struct notifier_block *nb, >>>>>>>> + unsigned long val, void *ptr) >>>>>>>> +{ >>>>>>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_min)); >>>>>>>> +} >>>>>>>> + >>>>>>>> +/** >>>>>>>> + * qos_max_notifier_call() - Callback for QoS max_freq changes. >>>>>>>> + * @nb: Should be devfreq->nb_max >>>>>>>> + */ >>>>>>>> +static int qos_max_notifier_call(struct notifier_block *nb, >>>>>>>> + unsigned long val, void *ptr) >>>>>>>> +{ >>>>>>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_max)); >>>>>>>> +} >>>>>>>> + >>>>>>>> /** >>>>>>>> * devfreq_dev_release() - Callback for struct device to release the device. >>>>>>>> * @dev: the devfreq device >>>>>>>> * >>>>>>>> * Remove devfreq from the list and release its resources. >>>>>>>> @@ -620,10 +671,15 @@ static void devfreq_dev_release(struct device *dev) >>>>>>>> >>>>>>>> mutex_lock(&devfreq_list_lock); >>>>>>>> list_del(&devfreq->node); >>>>>>>> mutex_unlock(&devfreq_list_lock); >>>>>>>> >>>>>>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max, >>>>>>>> + DEV_PM_QOS_MAX_FREQUENCY); >>>>>>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min, >>>>>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>>>>> + >>>>>>> >>>>>>> Just print error with dev_err() without stopping the release step. >>>>>>> >>>>>>> I prefer to handle the return value if kernel API provides >>>>>>> the error code. >>>>> >>>>> How about? >>>> >>>> Modified to dev_warn. This also applies to PATCH 6 so I replied there. >>>> >>>>>>>> if (devfreq->profile->exit) >>>>>>>> devfreq->profile->exit(devfreq->dev.parent); >>>>>>>> >>>>>>>> kfree(devfreq->time_in_state); >>>>>>>> kfree(devfreq->trans_table); >>>>>>>> @@ -733,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>>>>>> if (err) { >>>>>>>> put_device(&devfreq->dev); >>>>>>>> goto err_out; >>>>>>>> } >>>>>>>> >>>>>>>> + /* >>>>>>>> + * Register notifiers for updates to min/max_freq after device is >>>>>>>> + * initialized (and we can handle notifications) but before the >>>>>>>> + * governor is started (which should do an initial enforcement of >>>>>>>> + * constraints). >>>>>>>> + */ >>>>>>> >>>>>>> My previous comment is not enough why I prefer to remove it. Sorry. >>>>>>> Actually, until now, the devfreq_add_device() don't have the detailed >>>>>>> comments because the line code is not too long. But, at the present time, >>>>>>> devfreq_add_device() is too long. It means that the detailed comment >>>>>>> are necessary. >>>>>>> >>>>>>> So, I'll add the detailed comment for each step of devfreq_add_device() >>>>>>> on separate patch to keep the same style. I'll send the patch to you >>>>>>> for the review. >>>>>> >>>>>> This is very likely to result in merge conflicts, maybe wait for my >>>>>> series to go in first? >>>>> >>>>> I'll send the separate patch after finished the review of these patches. >>>>> So, if you agree, please remove this comment on this patch. >>>>> >>>>> You can review the detailed comments on separate patch when I send. >>>> This patch already contains comments and they explain the code being >>>> added. Doesn't it make more sense for comments and code to go in together? >>>> >>>> I think the comment is a resonable explanation as to why notifiers are >>>> registered at that specific step in the initialization sequence. >>> >>> If you add this comment on this patch, OK. just I have some comments. >>> >>> /* >>> * Register notifiers for updates to min/max_freq after device is >>> * initialized (and we can handle notifications) but before the >>> >>> I think that 'device is initialized' is not needed. >>> It is always true, it don't need to add the additional comments. >>> because dev_pm_qos_add_notifier() must need the 'devfreq->dev'. >>> The this code prove the call sequence between them. >> >> In theory if a notifier is registered too soon then it could crash on a >> NULL pointer. But looking at the code it first checks that "governor != >> NULL) so it would be harmless. >> >>> >>> About 'us', don't use 'we'. The subject is 'devfreq' or other device instead of us. >>> >>> * governor is started (which should do an initial enforcement of >>> * constraints). >>> */ >>> >>> Actually, it doesn't matter the initialization step between governor >>> and PM_QOS registration. >> >> In theory PM QoS constraints could be modified between governor startup >> and notifier registration and that update would be lost (until the next >> one). > > Don't lose the any of PM QoS request. User can request the any frequency > through PMQoS at the any time and then devfreq consider the constraints > from PM QoS. > > Also, after finished the registration of devfreq device, > the user (other device driver required the min/max freq) > can request the PM QoS on real use-case. > > It is impossible to get the devfreq instance before finished > the devfreq_add_device() because the user can access the devfreq instance > devfreq_get_devfreq_by_phandle() which use 'devfreq_list'. > > If devfreq_add_device() is not finished, the user cannot get > the any devfreq device from 'devfreq_list'. PM QoS constraints are not on devfreq->dev but on the parent so in theory it's possible to add constraints during devfreq initialization. Making PM QoS requests doesn't require fetching devfreq instances, I think you could call of_find_device_by_node with a phandle and register constraints that way. >>> If start governor and then register PM_QOS, >>> the governor can decide the new frequency. And then PM_QOS request >>> the new constraints for min/max frequency. On request time of PM_QOS, >>> devfreq can detect this time and then apply the constraints from PM_QOS. >>> The user of devfreq's PM_QOS must need the phandle or device instance >>> of devfreq deivce. It means that user of devfreq's PM_QOS can request >>> the any constraints of PM_QOS_MIN/MAX through PM_QOS interface. >>> >>> So, it doesn't need to add the following comments >>> because following comment is not always mandatory. >>> " governor is started (which should do an initial enforcement of constraints)." >>> >>> Also, you don't need to use parentheses on comments. >>> >>> Actually, I think that following comments are enough. >>> >>> /* Register PM QoS notifiers for updating to min/max freq */ >> >> I thought that explaning why it's done at this particular step but I'll >> just remove it, devfreq_add_device can get more comments later. > > OK. Thanks. > I think that if the comment contains what is meaning of the code, > it is enough. > > Although change the sequence between dev_pm_qos_add_notifier() and > try_then_request_governor(), in the real-case, the any issue > would not happen. Because as I commented above, the other device-driver > cannot get the devfreq instance before finished the devfreq_add_device. > It means that the user device driver cannot request the any PM QoS > constraints. > >> >>>>>>>> + devfreq->nb_min.notifier_call = qos_min_notifier_call; >>>>>>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min, >>>>>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>>>>> + if (err) >>>>>>>> + goto err_devfreq; >>>>>>>> + >>>>>>>> + devfreq->nb_max.notifier_call = qos_max_notifier_call; >>>>>>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max, >>>>>>>> + DEV_PM_QOS_MAX_FREQUENCY); >>>>>>>> + if (err) >>>>>>>> + goto err_devfreq; >>>>>>>> + >>>>>>>> mutex_lock(&devfreq_list_lock); >>>>>>>> >>>>>>>> governor = try_then_request_governor(devfreq->governor_name); >>>>>>>> if (IS_ERR(governor)) { >>>>>>>> dev_err(dev, "%s: Unable to find governor for the device\n", >>>>>>>> @@ -760,10 +834,11 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>>>>>> >>>>>>>> return devfreq; >>>>>>>> >>>>>>>> err_init: >>>>>>>> mutex_unlock(&devfreq_list_lock); >>>>>>>> +err_devfreq: >>>>>>>> devfreq_remove_device(devfreq); >>>>>>>> return ERR_PTR(err); >>>>>>>> >>>>>>>> err_dev: >>>>>>>> /* >>>>>>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >>>>>>>> index c3cbc15fdf08..dac0dffeabb4 100644 >>>>>>>> --- a/include/linux/devfreq.h >>>>>>>> +++ b/include/linux/devfreq.h >>>>>>>> @@ -134,10 +134,12 @@ struct devfreq_dev_profile { >>>>>>>> * @total_trans: Number of devfreq transitions >>>>>>>> * @trans_table: Statistics of devfreq transitions >>>>>>>> * @time_in_state: Statistics of devfreq states >>>>>>>> * @last_stat_updated: The last time stat updated >>>>>>>> * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier >>>>>>>> + * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY >>>>>>>> + * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY >>>>>>>> * >>>>>>>> * This structure stores the devfreq information for a give device. >>>>>>>> * >>>>>>>> * Note that when a governor accesses entries in struct devfreq in its >>>>>>>> * functions except for the context of callbacks defined in struct >>>>>>>> @@ -176,10 +178,13 @@ struct devfreq { >>>>>>>> unsigned int *trans_table; >>>>>>>> unsigned long *time_in_state; >>>>>>>> unsigned long last_stat_updated; >>>>>>>> >>>>>>>> struct srcu_notifier_head transition_notifier_list; >>>>>>>> + >>>>>>>> + struct notifier_block nb_min; >>>>>>>> + struct notifier_block nb_max; >>>>>>>> }; >>>>>>>> >>>>>>>> struct devfreq_freqs { >>>>>>>> unsigned long old; >>>>>>>> unsigned long new; >>> >>> >> > >
On 19. 10. 1. 오후 6:39, Leonard Crestez wrote: > On 2019-10-01 12:37 AM, Chanwoo Choi wrote: >> Hi, >> >> On 19. 9. 30. 오후 10:16, Leonard Crestez wrote: >>> On 2019-09-27 4:45 AM, Chanwoo Choi wrote: >>>> On 19. 9. 26. 오후 10:43, Leonard Crestez wrote: >>>>> On 2019-09-26 4:07 AM, Chanwoo Choi wrote: >>>>>> On 19. 9. 26. 오전 6:18, Leonard Crestez wrote: >>>>>>> On 25.09.2019 05:11, Chanwoo Choi wrote: >>>>>>>> On 19. 9. 24. 오후 7:11, Leonard Crestez wrote: >>>>>>>>> Register notifiers with the PM QoS framework in order to respond to >>>>>>>>> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY. >>>>>>>>> >>>>>>>>> No notifiers are added by this patch but PM QoS constraints can be >>>>>>>>> imposed externally (for example from other devices). >>>>>>>>> >>>>>>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>>>>>>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >>>>>>>>> --- >>>>>>>>> drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++ >>>>>>>>> include/linux/devfreq.h | 5 +++ >>>>>>>>> 2 files changed, 80 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>>>>>>> index eee403e70c84..784f3e40536a 100644 >>>>>>>>> --- a/drivers/devfreq/devfreq.c >>>>>>>>> +++ b/drivers/devfreq/devfreq.c >>>>>>>>> @@ -22,15 +22,18 @@ >>>>>>>>> #include <linux/platform_device.h> >>>>>>>>> #include <linux/list.h> >>>>>>>>> #include <linux/printk.h> >>>>>>>>> #include <linux/hrtimer.h> >>>>>>>>> #include <linux/of.h> >>>>>>>>> +#include <linux/pm_qos.h> >>>>>>>>> #include "governor.h" >>>>>>>>> >>>>>>>>> #define CREATE_TRACE_POINTS >>>>>>>>> #include <trace/events/devfreq.h> >>>>>>>>> >>>>>>>>> +#define HZ_PER_KHZ 1000 >>>>>>>>> + >>>>>>>>> static struct class *devfreq_class; >>>>>>>>> >>>>>>>>> /* >>>>>>>>> * devfreq core provides delayed work based load monitoring helper >>>>>>>>> * functions. Governors can use these or can implement their own >>>>>>>>> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) >>>>>>>>> static void get_freq_range(struct devfreq *devfreq, >>>>>>>>> unsigned long *min_freq, >>>>>>>>> unsigned long *max_freq) >>>>>>>>> { >>>>>>>>> unsigned long *freq_table = devfreq->profile->freq_table; >>>>>>>>> + unsigned long qos_min_freq, qos_max_freq; >>>>>>>>> >>>>>>>>> lockdep_assert_held(&devfreq->lock); >>>>>>>>> >>>>>>>>> /* >>>>>>>>> * Init min/max frequency from freq table. >>>>>>>>> @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq, >>>>>>>>> } else { >>>>>>>>> *min_freq = freq_table[devfreq->profile->max_state - 1]; >>>>>>>>> *max_freq = freq_table[0]; >>>>>>>>> } >>>>>>>>> >>>>>>>>> + /* constraints from PM QoS */ >>>>>>>> >>>>>>>> As I commented on patch4, >>>>>>>> 'constraints' -> 'Constraint' because first verb have to be used >>>>>>>> as the sigular verbs. >>>>>>> >>>>>>> Already discussed for another patch; I will modify to "Apply constraints >>>>>>> from PM QoS" instead. >>>>>> >>>>>> I agree the new comment with 'Apply constraints ... '. >>>>>> >>>>>>> >>>>>>>> I prefer to use following comments: >>>>>>>> >>>>>>>> /* Constraint minimum/maximum frequency from PM QoS constraints */ >>>>>>>> >>>>>>>>> + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>>>>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>>>>>> + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>>>>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>>>>>> + *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); >>>>>>>>> + *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); >>>>>>>>> + >>>>>>>>> /* constraints from sysfs */ >>>>>>>>> *min_freq = max(*min_freq, devfreq->min_freq); >>>>>>>>> *max_freq = min(*max_freq, devfreq->max_freq); >>>>>>>>> >>>>>>>>> /* constraints from OPP interface */ >>>>>>>>> @@ -606,10 +618,49 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, >>>>>>>>> mutex_unlock(&devfreq->lock); >>>>>>>>> >>>>>>>>> return ret; >>>>>>>>> } >>>>>>>>> >>>>>>>>> +/** >>>>>>>>> + * qos_notifier_call() - Common handler for QoS constraints. >>>>>>>>> + * @devfreq: the devfreq instance. >>>>>>>>> + */ >>>>>>>>> +static int qos_notifier_call(struct devfreq *devfreq) >>>>>>>>> +{ >>>>>>>>> + int err; >>>>>>>>> + >>>>>>>>> + mutex_lock(&devfreq->lock); >>>>>>>>> + err = update_devfreq(devfreq); >>>>>>>>> + mutex_unlock(&devfreq->lock); >>>>>>>>> + if (err) >>>>>>>>> + dev_err(devfreq->dev.parent, >>>>>>>>> + "failed to update frequency for PM QoS constraints (%d)\n", >>>>>>>> >>>>>>>> Is it not over 80 char? >>>>>>> >>>>>>> Yes but coding style explicitly forbids breaking strings. >>>>>>> >>>>>>>>> + err); >>>>>>>>> + >>>>>>>>> + return NOTIFY_OK; >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +/** >>>>>>>>> + * qos_min_notifier_call() - Callback for QoS min_freq changes. >>>>>>>>> + * @nb: Should be devfreq->nb_min >>>>>>>>> + */ >>>>>>>>> +static int qos_min_notifier_call(struct notifier_block *nb, >>>>>>>>> + unsigned long val, void *ptr) >>>>>>>>> +{ >>>>>>>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_min)); >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +/** >>>>>>>>> + * qos_max_notifier_call() - Callback for QoS max_freq changes. >>>>>>>>> + * @nb: Should be devfreq->nb_max >>>>>>>>> + */ >>>>>>>>> +static int qos_max_notifier_call(struct notifier_block *nb, >>>>>>>>> + unsigned long val, void *ptr) >>>>>>>>> +{ >>>>>>>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_max)); >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> /** >>>>>>>>> * devfreq_dev_release() - Callback for struct device to release the device. >>>>>>>>> * @dev: the devfreq device >>>>>>>>> * >>>>>>>>> * Remove devfreq from the list and release its resources. >>>>>>>>> @@ -620,10 +671,15 @@ static void devfreq_dev_release(struct device *dev) >>>>>>>>> >>>>>>>>> mutex_lock(&devfreq_list_lock); >>>>>>>>> list_del(&devfreq->node); >>>>>>>>> mutex_unlock(&devfreq_list_lock); >>>>>>>>> >>>>>>>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max, >>>>>>>>> + DEV_PM_QOS_MAX_FREQUENCY); >>>>>>>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min, >>>>>>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>>>>>> + >>>>>>>> >>>>>>>> Just print error with dev_err() without stopping the release step. >>>>>>>> >>>>>>>> I prefer to handle the return value if kernel API provides >>>>>>>> the error code. >>>>>> >>>>>> How about? >>>>> >>>>> Modified to dev_warn. This also applies to PATCH 6 so I replied there. >>>>> >>>>>>>>> if (devfreq->profile->exit) >>>>>>>>> devfreq->profile->exit(devfreq->dev.parent); >>>>>>>>> >>>>>>>>> kfree(devfreq->time_in_state); >>>>>>>>> kfree(devfreq->trans_table); >>>>>>>>> @@ -733,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>>>>>>> if (err) { >>>>>>>>> put_device(&devfreq->dev); >>>>>>>>> goto err_out; >>>>>>>>> } >>>>>>>>> >>>>>>>>> + /* >>>>>>>>> + * Register notifiers for updates to min/max_freq after device is >>>>>>>>> + * initialized (and we can handle notifications) but before the >>>>>>>>> + * governor is started (which should do an initial enforcement of >>>>>>>>> + * constraints). >>>>>>>>> + */ >>>>>>>> >>>>>>>> My previous comment is not enough why I prefer to remove it. Sorry. >>>>>>>> Actually, until now, the devfreq_add_device() don't have the detailed >>>>>>>> comments because the line code is not too long. But, at the present time, >>>>>>>> devfreq_add_device() is too long. It means that the detailed comment >>>>>>>> are necessary. >>>>>>>> >>>>>>>> So, I'll add the detailed comment for each step of devfreq_add_device() >>>>>>>> on separate patch to keep the same style. I'll send the patch to you >>>>>>>> for the review. >>>>>>> >>>>>>> This is very likely to result in merge conflicts, maybe wait for my >>>>>>> series to go in first? >>>>>> >>>>>> I'll send the separate patch after finished the review of these patches. >>>>>> So, if you agree, please remove this comment on this patch. >>>>>> >>>>>> You can review the detailed comments on separate patch when I send. >>>>> This patch already contains comments and they explain the code being >>>>> added. Doesn't it make more sense for comments and code to go in together? >>>>> >>>>> I think the comment is a resonable explanation as to why notifiers are >>>>> registered at that specific step in the initialization sequence. >>>> >>>> If you add this comment on this patch, OK. just I have some comments. >>>> >>>> /* >>>> * Register notifiers for updates to min/max_freq after device is >>>> * initialized (and we can handle notifications) but before the >>>> >>>> I think that 'device is initialized' is not needed. >>>> It is always true, it don't need to add the additional comments. >>>> because dev_pm_qos_add_notifier() must need the 'devfreq->dev'. >>>> The this code prove the call sequence between them. >>> >>> In theory if a notifier is registered too soon then it could crash on a >>> NULL pointer. But looking at the code it first checks that "governor != >>> NULL) so it would be harmless. >>> >>>> >>>> About 'us', don't use 'we'. The subject is 'devfreq' or other device instead of us. >>>> >>>> * governor is started (which should do an initial enforcement of >>>> * constraints). >>>> */ >>>> >>>> Actually, it doesn't matter the initialization step between governor >>>> and PM_QOS registration. >>> >>> In theory PM QoS constraints could be modified between governor startup >>> and notifier registration and that update would be lost (until the next >>> one). >> >> Don't lose the any of PM QoS request. User can request the any frequency >> through PMQoS at the any time and then devfreq consider the constraints >> from PM QoS. >> >> Also, after finished the registration of devfreq device, >> the user (other device driver required the min/max freq) >> can request the PM QoS on real use-case. >> >> It is impossible to get the devfreq instance before finished >> the devfreq_add_device() because the user can access the devfreq instance >> devfreq_get_devfreq_by_phandle() which use 'devfreq_list'. >> >> If devfreq_add_device() is not finished, the user cannot get >> the any devfreq device from 'devfreq_list'. > > PM QoS constraints are not on devfreq->dev but on the parent so in > theory it's possible to add constraints during devfreq initialization. > > Making PM QoS requests doesn't require fetching devfreq instances, I > think you could call of_find_device_by_node with a phandle and register > constraints that way. You're right. My comment was not enough. I mean that PM QoS depends on the devfreq->dev.parent. If probe() of devfreq device requires the PMQoS request, it is possible to register PM QoS. On the other hand, the other device device regardless the devfreq device driver, they can request PM QoS constraints after registering the parent device (devfreq->dev.parent). Your patch already registered the PM QoS notifier before governor start. It is enough to show the your intention. I don't any objection of this code. Just, the too more detailed comment make the devfreq core complicated. As I commented, I think t hat the simple comment is enough for devfreq user. Thanks for your explanation. > >>>> If start governor and then register PM_QOS, >>>> the governor can decide the new frequency. And then PM_QOS request >>>> the new constraints for min/max frequency. On request time of PM_QOS, >>>> devfreq can detect this time and then apply the constraints from PM_QOS. >>>> The user of devfreq's PM_QOS must need the phandle or device instance >>>> of devfreq deivce. It means that user of devfreq's PM_QOS can request >>>> the any constraints of PM_QOS_MIN/MAX through PM_QOS interface. >>>> >>>> So, it doesn't need to add the following comments >>>> because following comment is not always mandatory. >>>> " governor is started (which should do an initial enforcement of constraints)." >>>> >>>> Also, you don't need to use parentheses on comments. >>>> >>>> Actually, I think that following comments are enough. >>>> >>>> /* Register PM QoS notifiers for updating to min/max freq */ >>> >>> I thought that explaning why it's done at this particular step but I'll >>> just remove it, devfreq_add_device can get more comments later. >> >> OK. Thanks. >> I think that if the comment contains what is meaning of the code, >> it is enough. >> >> Although change the sequence between dev_pm_qos_add_notifier() and >> try_then_request_governor(), in the real-case, the any issue >> would not happen. Because as I commented above, the other device-driver >> cannot get the devfreq instance before finished the devfreq_add_device. >> It means that the user device driver cannot request the any PM QoS >> constraints. >> >>> >>>>>>>>> + devfreq->nb_min.notifier_call = qos_min_notifier_call; >>>>>>>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min, >>>>>>>>> + DEV_PM_QOS_MIN_FREQUENCY); >>>>>>>>> + if (err) >>>>>>>>> + goto err_devfreq; >>>>>>>>> + >>>>>>>>> + devfreq->nb_max.notifier_call = qos_max_notifier_call; >>>>>>>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max, >>>>>>>>> + DEV_PM_QOS_MAX_FREQUENCY); >>>>>>>>> + if (err) >>>>>>>>> + goto err_devfreq; >>>>>>>>> + >>>>>>>>> mutex_lock(&devfreq_list_lock); >>>>>>>>> >>>>>>>>> governor = try_then_request_governor(devfreq->governor_name); >>>>>>>>> if (IS_ERR(governor)) { >>>>>>>>> dev_err(dev, "%s: Unable to find governor for the device\n", >>>>>>>>> @@ -760,10 +834,11 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>>>>>>> >>>>>>>>> return devfreq; >>>>>>>>> >>>>>>>>> err_init: >>>>>>>>> mutex_unlock(&devfreq_list_lock); >>>>>>>>> +err_devfreq: >>>>>>>>> devfreq_remove_device(devfreq); >>>>>>>>> return ERR_PTR(err); >>>>>>>>> >>>>>>>>> err_dev: >>>>>>>>> /* >>>>>>>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >>>>>>>>> index c3cbc15fdf08..dac0dffeabb4 100644 >>>>>>>>> --- a/include/linux/devfreq.h >>>>>>>>> +++ b/include/linux/devfreq.h >>>>>>>>> @@ -134,10 +134,12 @@ struct devfreq_dev_profile { >>>>>>>>> * @total_trans: Number of devfreq transitions >>>>>>>>> * @trans_table: Statistics of devfreq transitions >>>>>>>>> * @time_in_state: Statistics of devfreq states >>>>>>>>> * @last_stat_updated: The last time stat updated >>>>>>>>> * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier >>>>>>>>> + * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY >>>>>>>>> + * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY >>>>>>>>> * >>>>>>>>> * This structure stores the devfreq information for a give device. >>>>>>>>> * >>>>>>>>> * Note that when a governor accesses entries in struct devfreq in its >>>>>>>>> * functions except for the context of callbacks defined in struct >>>>>>>>> @@ -176,10 +178,13 @@ struct devfreq { >>>>>>>>> unsigned int *trans_table; >>>>>>>>> unsigned long *time_in_state; >>>>>>>>> unsigned long last_stat_updated; >>>>>>>>> >>>>>>>>> struct srcu_notifier_head transition_notifier_list; >>>>>>>>> + >>>>>>>>> + struct notifier_block nb_min; >>>>>>>>> + struct notifier_block nb_max; >>>>>>>>> }; >>>>>>>>> >>>>>>>>> struct devfreq_freqs { >>>>>>>>> unsigned long old; >>>>>>>>> unsigned long new; >>>> >>>> >>> >> >> >
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index eee403e70c84..784f3e40536a 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -22,15 +22,18 @@ #include <linux/platform_device.h> #include <linux/list.h> #include <linux/printk.h> #include <linux/hrtimer.h> #include <linux/of.h> +#include <linux/pm_qos.h> #include "governor.h" #define CREATE_TRACE_POINTS #include <trace/events/devfreq.h> +#define HZ_PER_KHZ 1000 + static struct class *devfreq_class; /* * devfreq core provides delayed work based load monitoring helper * functions. Governors can use these or can implement their own @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) static void get_freq_range(struct devfreq *devfreq, unsigned long *min_freq, unsigned long *max_freq) { unsigned long *freq_table = devfreq->profile->freq_table; + unsigned long qos_min_freq, qos_max_freq; lockdep_assert_held(&devfreq->lock); /* * Init min/max frequency from freq table. @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq, } else { *min_freq = freq_table[devfreq->profile->max_state - 1]; *max_freq = freq_table[0]; } + /* constraints from PM QoS */ + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent, + DEV_PM_QOS_MIN_FREQUENCY); + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, + DEV_PM_QOS_MIN_FREQUENCY); + *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); + *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); + /* constraints from sysfs */ *min_freq = max(*min_freq, devfreq->min_freq); *max_freq = min(*max_freq, devfreq->max_freq); /* constraints from OPP interface */ @@ -606,10 +618,49 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, mutex_unlock(&devfreq->lock); return ret; } +/** + * qos_notifier_call() - Common handler for QoS constraints. + * @devfreq: the devfreq instance. + */ +static int qos_notifier_call(struct devfreq *devfreq) +{ + int err; + + mutex_lock(&devfreq->lock); + err = update_devfreq(devfreq); + mutex_unlock(&devfreq->lock); + if (err) + dev_err(devfreq->dev.parent, + "failed to update frequency for PM QoS constraints (%d)\n", + err); + + return NOTIFY_OK; +} + +/** + * qos_min_notifier_call() - Callback for QoS min_freq changes. + * @nb: Should be devfreq->nb_min + */ +static int qos_min_notifier_call(struct notifier_block *nb, + unsigned long val, void *ptr) +{ + return qos_notifier_call(container_of(nb, struct devfreq, nb_min)); +} + +/** + * qos_max_notifier_call() - Callback for QoS max_freq changes. + * @nb: Should be devfreq->nb_max + */ +static int qos_max_notifier_call(struct notifier_block *nb, + unsigned long val, void *ptr) +{ + return qos_notifier_call(container_of(nb, struct devfreq, nb_max)); +} + /** * devfreq_dev_release() - Callback for struct device to release the device. * @dev: the devfreq device * * Remove devfreq from the list and release its resources. @@ -620,10 +671,15 @@ static void devfreq_dev_release(struct device *dev) mutex_lock(&devfreq_list_lock); list_del(&devfreq->node); mutex_unlock(&devfreq_list_lock); + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max, + DEV_PM_QOS_MAX_FREQUENCY); + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min, + DEV_PM_QOS_MIN_FREQUENCY); + if (devfreq->profile->exit) devfreq->profile->exit(devfreq->dev.parent); kfree(devfreq->time_in_state); kfree(devfreq->trans_table); @@ -733,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev, if (err) { put_device(&devfreq->dev); goto err_out; } + /* + * Register notifiers for updates to min/max_freq after device is + * initialized (and we can handle notifications) but before the + * governor is started (which should do an initial enforcement of + * constraints). + */ + devfreq->nb_min.notifier_call = qos_min_notifier_call; + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min, + DEV_PM_QOS_MIN_FREQUENCY); + if (err) + goto err_devfreq; + + devfreq->nb_max.notifier_call = qos_max_notifier_call; + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max, + DEV_PM_QOS_MAX_FREQUENCY); + if (err) + goto err_devfreq; + mutex_lock(&devfreq_list_lock); governor = try_then_request_governor(devfreq->governor_name); if (IS_ERR(governor)) { dev_err(dev, "%s: Unable to find governor for the device\n", @@ -760,10 +834,11 @@ struct devfreq *devfreq_add_device(struct device *dev, return devfreq; err_init: mutex_unlock(&devfreq_list_lock); +err_devfreq: devfreq_remove_device(devfreq); return ERR_PTR(err); err_dev: /* diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h index c3cbc15fdf08..dac0dffeabb4 100644 --- a/include/linux/devfreq.h +++ b/include/linux/devfreq.h @@ -134,10 +134,12 @@ struct devfreq_dev_profile { * @total_trans: Number of devfreq transitions * @trans_table: Statistics of devfreq transitions * @time_in_state: Statistics of devfreq states * @last_stat_updated: The last time stat updated * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier + * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY + * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY * * This structure stores the devfreq information for a give device. * * Note that when a governor accesses entries in struct devfreq in its * functions except for the context of callbacks defined in struct @@ -176,10 +178,13 @@ struct devfreq { unsigned int *trans_table; unsigned long *time_in_state; unsigned long last_stat_updated; struct srcu_notifier_head transition_notifier_list; + + struct notifier_block nb_min; + struct notifier_block nb_max; }; struct devfreq_freqs { unsigned long old; unsigned long new;