Hi Kamil, The 'freq_table' and 'max_state' in the devfreq_dev_profile were used in the ARM Mali device driver[1][2][3]. Although ARM Mali device driver was posted to mainline kernel, they used them for a long time. It means that this patch break the compatibility. The ARM Mali drivers are very important devfreq device driver. [1] https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/bifrost-kernel# [2] https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/midgard-kernel [3] https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/utgard-kernel Also, the devfreq device driver specifies their own information and data into devfreq_dev_profile structure before registering the devfreq device with devfreq_add_device(). This patch breaks the basic usage rule of devfreq_dev_profile structure. So, I can't agree this patch. Not ack. Regards, Chanwoo Choi On 11/13/19 6:13 PM, Kamil Konieczny wrote: > Count time and transitions between devfreq frequencies in separate struct > for improved code readability and maintenance. > > Signed-off-by: Kamil Konieczny <k.konieczny@xxxxxxxxxxx> > --- > drivers/devfreq/devfreq.c | 156 ++++++++++++++++------------- > drivers/devfreq/exynos-bus.c | 6 +- > drivers/devfreq/governor_passive.c | 26 +++-- > include/linux/devfreq.h | 43 ++++---- > 4 files changed, 129 insertions(+), 102 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index d79412b0de59..d85867a91230 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -105,10 +105,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) > */ > static int devfreq_get_freq_level(struct devfreq *devfreq, unsigned long freq) > { > + struct devfreq_stats *stats = devfreq->profile->stats; > int lev; > > - for (lev = 0; lev < devfreq->profile->max_state; lev++) > - if (freq == devfreq->profile->freq_table[lev]) > + for (lev = 0; lev < stats->max_state; lev++) > + if (freq == stats->freq_table[lev]) > return lev; > > return -EINVAL; > @@ -117,56 +118,64 @@ static int devfreq_get_freq_level(struct devfreq *devfreq, unsigned long freq) > static int set_freq_table(struct devfreq *devfreq) > { > struct devfreq_dev_profile *profile = devfreq->profile; > + struct devfreq_stats *stats; > struct dev_pm_opp *opp; > unsigned long freq; > - int i, count; > + int i, count, err = -ENOMEM; > > /* Initialize the freq_table from OPP table */ > count = dev_pm_opp_get_opp_count(devfreq->dev.parent); > if (count <= 0) > return -EINVAL; > > - profile->max_state = count; > - profile->freq_table = devm_kcalloc(devfreq->dev.parent, > - count, > - sizeof(*profile->freq_table), > - GFP_KERNEL); > - if (!profile->freq_table) { > - profile->max_state = 0; > + stats = devm_kzalloc(devfreq->dev.parent, > + sizeof(struct devfreq_stats), GFP_KERNEL); > + if (!stats) > return -ENOMEM; > - } > > - for (i = 0, freq = 0; i < profile->max_state; i++, freq++) { > + profile->stats = stats; > + stats->max_state = count; > + stats->freq_table = devm_kcalloc(devfreq->dev.parent, > + count, > + sizeof(*stats->freq_table), > + GFP_KERNEL); > + if (!stats->freq_table) > + goto err_no_mem; > + > + for (i = 0, freq = 0; i < count; i++, freq++) { > opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &freq); > if (IS_ERR(opp)) { > - devm_kfree(devfreq->dev.parent, profile->freq_table); > - profile->max_state = 0; > - return PTR_ERR(opp); > + devm_kfree(devfreq->dev.parent, stats->freq_table); > + stats->max_state = 0; > + err = PTR_ERR(opp); > + goto err_no_mem; > } > dev_pm_opp_put(opp); > - profile->freq_table[i] = freq; > + stats->freq_table[i] = freq; > } > > - profile->trans_table = devm_kzalloc(devfreq->dev.parent, > - array3_size(sizeof(unsigned int), > - count, count), > - GFP_KERNEL); > - if (!profile->trans_table) > + stats->trans_table = devm_kzalloc(devfreq->dev.parent, > + array3_size(sizeof(unsigned int), > + count, count), > + GFP_KERNEL); > + if (!stats->trans_table) > goto err_no_mem; > > - profile->time_in_state = devm_kcalloc(devfreq->dev.parent, count, > - sizeof(*profile->time_in_state), > - GFP_KERNEL); > - if (!profile->time_in_state) > + stats->time_in_state = devm_kcalloc(devfreq->dev.parent, count, > + sizeof(*stats->time_in_state), > + GFP_KERNEL); > + if (!stats->time_in_state) > goto err_no_mem; > > - profile->last_time = get_jiffies_64(); > - spin_lock_init(&profile->stats_lock); > + stats->last_time = get_jiffies_64(); > + spin_lock_init(&stats->stats_lock); > > return 0; > err_no_mem: > - profile->max_state = 0; > - return -ENOMEM; > + stats->max_state = 0; > + devm_kfree(devfreq->dev.parent, profile->stats); > + profile->stats = NULL; > + return err; > } > > /** > @@ -176,7 +185,7 @@ static int set_freq_table(struct devfreq *devfreq) > */ > int devfreq_update_status(struct devfreq *devfreq, unsigned long freq) > { > - struct devfreq_dev_profile *profile = devfreq->profile; > + struct devfreq_stats *stats = devfreq->profile->stats; > unsigned long long cur_time; > int lev, prev_lev, ret = 0; > > @@ -184,22 +193,21 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq) > > /* Immediately exit if previous_freq is not initialized yet. */ > if (!devfreq->previous_freq) { > - spin_lock(&profile->stats_lock); > - profile->last_time = cur_time; > - spin_unlock(&profile->stats_lock); > + spin_lock(&stats->stats_lock); > + stats->last_time = cur_time; > + spin_unlock(&stats->stats_lock); > return 0; > } > > prev_lev = devfreq_get_freq_level(devfreq, devfreq->previous_freq); > > - spin_lock(&profile->stats_lock); > + spin_lock(&stats->stats_lock); > if (prev_lev < 0) { > ret = prev_lev; > goto out; > } > > - profile->time_in_state[prev_lev] += > - cur_time - profile->last_time; > + stats->time_in_state[prev_lev] += cur_time - stats->last_time; > lev = devfreq_get_freq_level(devfreq, freq); > if (lev < 0) { > ret = lev; > @@ -207,14 +215,14 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq) > } > > if (lev != prev_lev) { > - profile->trans_table[(prev_lev * > - profile->max_state) + lev]++; > - profile->total_trans++; > + stats->trans_table[(prev_lev * > + stats->max_state) + lev]++; > + stats->total_trans++; > } > > out: > - profile->last_time = cur_time; > - spin_unlock(&profile->stats_lock); > + stats->last_time = cur_time; > + spin_unlock(&stats->stats_lock); > return ret; > } > EXPORT_SYMBOL(devfreq_update_status); > @@ -504,9 +512,9 @@ void devfreq_monitor_resume(struct devfreq *devfreq) > queue_delayed_work(devfreq_wq, &devfreq->work, > msecs_to_jiffies(profile->polling_ms)); > > - spin_lock(&profile->stats_lock); > - profile->last_time = get_jiffies_64(); > - spin_unlock(&profile->stats_lock); > + spin_lock(&profile->stats->stats_lock); > + profile->stats->last_time = get_jiffies_64(); > + spin_unlock(&profile->stats->stats_lock); > devfreq->stop_polling = false; > > if (profile->get_cur_freq && > @@ -677,7 +685,7 @@ struct devfreq *devfreq_add_device(struct device *dev, > devfreq->data = data; > devfreq->nb.notifier_call = devfreq_notifier_call; > > - if (!profile->max_state && !profile->freq_table) { > + if (!profile->stats) { > mutex_unlock(&devfreq->lock); > err = set_freq_table(devfreq); > if (err < 0) > @@ -1282,6 +1290,7 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, > const char *buf, size_t count) > { > struct devfreq *df = to_devfreq(dev); > + struct devfreq_stats *stats = df->profile->stats; > unsigned long value; > int ret; > > @@ -1297,13 +1306,13 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, > goto unlock; > } > } else { > - unsigned long *freq_table = df->profile->freq_table; > + unsigned long *freq_table = stats->freq_table; > > /* Get minimum frequency according to sorting order */ > - if (freq_table[0] < freq_table[df->profile->max_state - 1]) > + if (freq_table[0] < freq_table[stats->max_state - 1]) > value = freq_table[0]; > else > - value = freq_table[df->profile->max_state - 1]; > + value = freq_table[stats->max_state - 1]; > } > > df->min_freq = value; > @@ -1326,6 +1335,7 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, > const char *buf, size_t count) > { > struct devfreq *df = to_devfreq(dev); > + struct devfreq_stats *stats = df->profile->stats; > unsigned long value; > int ret; > > @@ -1341,11 +1351,11 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, > goto unlock; > } > } else { > - unsigned long *freq_table = df->profile->freq_table; > + unsigned long *freq_table = stats->freq_table; > > /* Get maximum frequency according to sorting order */ > - if (freq_table[0] < freq_table[df->profile->max_state - 1]) > - value = freq_table[df->profile->max_state - 1]; > + if (freq_table[0] < freq_table[stats->max_state - 1]) > + value = freq_table[stats->max_state - 1]; > else > value = freq_table[0]; > } > @@ -1373,14 +1383,15 @@ static ssize_t available_frequencies_show(struct device *d, > char *buf) > { > struct devfreq *df = to_devfreq(d); > + struct devfreq_stats *stats = df->profile->stats; > ssize_t count = 0; > int i; > > mutex_lock(&df->lock); > > - for (i = 0; i < df->profile->max_state; i++) > + for (i = 0; i < stats->max_state; i++) > count += scnprintf(&buf[count], (PAGE_SIZE - count - 2), > - "%lu ", df->profile->freq_table[i]); > + "%lu ", stats->freq_table[i]); > > mutex_unlock(&df->lock); > /* Truncate the trailing space */ > @@ -1398,9 +1409,10 @@ static ssize_t trans_stat_show(struct device *dev, > { > struct devfreq *devfreq = to_devfreq(dev); > struct devfreq_dev_profile *profile = devfreq->profile; > + struct devfreq_stats *stats = profile->stats; > + unsigned int max_state = stats->max_state; > ssize_t len; > int i, j; > - unsigned int max_state = profile->max_state; > > if (!devfreq->stop_polling && > devfreq_update_status(devfreq, devfreq->previous_freq)) > @@ -1411,45 +1423,45 @@ static ssize_t trans_stat_show(struct device *dev, > len = sprintf(buf, " From : To\n"); > len += sprintf(buf + len, " :"); > > - spin_lock(&profile->stats_lock); > + spin_lock(&stats->stats_lock); > for (i = 0; i < max_state; i++) > len += sprintf(buf + len, "%10lu", > - profile->freq_table[i]); > + stats->freq_table[i]); > > len += sprintf(buf + len, " time(ms)\n"); > > for (i = 0; i < max_state; i++) { > - if (profile->freq_table[i] == devfreq->previous_freq) > + if (stats->freq_table[i] == devfreq->previous_freq) > len += sprintf(buf + len, "*"); > else > len += sprintf(buf + len, " "); > > len += sprintf(buf + len, "%10lu:", > - profile->freq_table[i]); > + stats->freq_table[i]); > for (j = 0; j < max_state; j++) > len += sprintf(buf + len, "%10u", > - profile->trans_table[(i * max_state) + j]); > + stats->trans_table[(i * max_state) + j]); > len += sprintf(buf + len, "%10llu\n", (u64) > - jiffies64_to_msecs(profile->time_in_state[i])); > + jiffies64_to_msecs(stats->time_in_state[i])); > } > > len += sprintf(buf + len, "Total transition : %u\n", > - profile->total_trans); > - spin_unlock(&profile->stats_lock); > + stats->total_trans); > + spin_unlock(&stats->stats_lock); > return len; > } > static DEVICE_ATTR_RO(trans_stat); > > -static void defvreq_stats_clear_table(struct devfreq_dev_profile *profile) > +static void defvreq_stats_clear_table(struct devfreq_stats *stats) > { > - unsigned int count = profile->max_state; > - > - spin_lock(&profile->stats_lock); > - memset(profile->time_in_state, 0, count * sizeof(u64)); > - memset(profile->trans_table, 0, count * count * sizeof(int)); > - profile->last_time = get_jiffies_64(); > - profile->total_trans = 0; > - spin_unlock(&profile->stats_lock); > + unsigned int count = stats->max_state; > + > + spin_lock(&stats->stats_lock); > + memset(stats->time_in_state, 0, count * sizeof(u64)); > + memset(stats->trans_table, 0, count * count * sizeof(int)); > + stats->last_time = get_jiffies_64(); > + stats->total_trans = 0; > + spin_unlock(&stats->stats_lock); > } > > static ssize_t trans_reset_store(struct device *dev, > @@ -1459,7 +1471,7 @@ static ssize_t trans_reset_store(struct device *dev, > { > struct devfreq *devfreq = to_devfreq(dev); > > - defvreq_stats_clear_table(devfreq->profile); > + defvreq_stats_clear_table(devfreq->profile->stats); > > return count; > } > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c > index d9f377912c10..b212aae2bb3e 100644 > --- a/drivers/devfreq/exynos-bus.c > +++ b/drivers/devfreq/exynos-bus.c > @@ -496,9 +496,9 @@ static int exynos_bus_probe(struct platform_device *pdev) > } > > out: > - max_state = bus->devfreq->profile->max_state; > - min_freq = (bus->devfreq->profile->freq_table[0] / 1000); > - max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000); > + max_state = profile->stats->max_state; > + min_freq = (profile->stats->freq_table[0] / 1000); > + max_freq = (profile->stats->freq_table[max_state - 1] / 1000); > pr_info("exynos-bus: new bus device registered: %s (%6ld KHz ~ %6ld KHz)\n", > dev_name(dev), min_freq, max_freq); > > diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c > index 58308948b863..b2d87a88335c 100644 > --- a/drivers/devfreq/governor_passive.c > +++ b/drivers/devfreq/governor_passive.c > @@ -18,6 +18,8 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq, > struct devfreq_passive_data *p_data > = (struct devfreq_passive_data *)devfreq->data; > struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent; > + struct devfreq_stats *parent_stats = parent_devfreq->profile->stats; > + struct devfreq_stats *stats; > unsigned long child_freq = ULONG_MAX; > struct dev_pm_opp *opp; > int i, count, ret = 0; > @@ -47,10 +49,14 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq, > * device. And then the index is used for getting the suitable > * new frequency for passive devfreq device. > */ > - if (!devfreq->profile || !devfreq->profile->freq_table > - || devfreq->profile->max_state <= 0) > + if (!devfreq->profile || !devfreq->profile->stats || > + devfreq->profile->stats->max_state <= 0 || > + !parent_devfreq->profile || !parent_devfreq->profile->stats || > + parent_devfreq->profile->stats->max_state <= 0) > return -EINVAL; > > + stats = devfreq->profile->stats; > + parent_stats = parent_devfreq->profile->stats; > /* > * The passive governor have to get the correct frequency from OPP > * list of parent device. Because in this case, *freq is temporary > @@ -68,21 +74,21 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq, > * Get the OPP table's index of decided freqeuncy by governor > * of parent device. > */ > - for (i = 0; i < parent_devfreq->profile->max_state; i++) > - if (parent_devfreq->profile->freq_table[i] == *freq) > + for (i = 0; i < parent_stats->max_state; i++) > + if (parent_stats->freq_table[i] == *freq) > break; > > - if (i == parent_devfreq->profile->max_state) { > + if (i == parent_stats->max_state) { > ret = -EINVAL; > goto out; > } > > /* Get the suitable frequency by using index of parent device. */ > - if (i < devfreq->profile->max_state) { > - child_freq = devfreq->profile->freq_table[i]; > + if (i < stats->max_state) { > + child_freq = stats->freq_table[i]; > } else { > - count = devfreq->profile->max_state; > - child_freq = devfreq->profile->freq_table[count - 1]; > + count = stats->max_state; > + child_freq = stats->freq_table[count - 1]; > } > > /* Return the suitable frequency for passive device. */ > @@ -109,7 +115,7 @@ static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq) > if (ret < 0) > goto out; > > - if (devfreq->profile->freq_table > + if (devfreq->profile->stats > && (devfreq_update_status(devfreq, freq))) > dev_err(&devfreq->dev, > "Couldn't update frequency transition information.\n"); > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h > index 4ceb2a517a9c..8459af1a1583 100644 > --- a/include/linux/devfreq.h > +++ b/include/linux/devfreq.h > @@ -64,6 +64,30 @@ struct devfreq_dev_status { > */ > #define DEVFREQ_FLAG_LEAST_UPPER_BOUND 0x1 > > +/** > + * struct devfreq_stats - Devfreq's transitions stats counters > + * @freq_table: Optional list of frequencies to support statistics > + * and freq_table must be generated in ascending order. > + * @max_state: The size of freq_table. > + * @total_trans: Number of devfreq transitions > + * @trans_table: Statistics of devfreq transitions > + * @time_in_state: Statistics of devfreq states > + * @last_time: The last time stats were updated > + * @stats_lock: Lock protecting trans_table, time_in_state, > + * last_time and total_trans used for statistics > + */ > +struct devfreq_stats { > + unsigned long *freq_table; > + unsigned int max_state; > + > + /* information for device frequency transition */ > + unsigned int total_trans; > + unsigned int *trans_table; > + u64 *time_in_state; > + unsigned long long last_time; > + spinlock_t stats_lock; > +}; > + > /** > * struct devfreq_dev_profile - Devfreq's user device profile > * @initial_freq: The operating frequency when devfreq_add_device() is > @@ -88,15 +112,7 @@ struct devfreq_dev_status { > * from devfreq_remove_device() call. If the user > * has registered devfreq->nb at a notifier-head, > * this is the time to unregister it. > - * @freq_table: Optional list of frequencies to support statistics > - * and freq_table must be generated in ascending order. > - * @max_state: The size of freq_table. > - * @total_trans: Number of devfreq transitions > - * @trans_table: Statistics of devfreq transitions > - * @time_in_state: Statistics of devfreq states > - * @last_time: The last time stats were updated > - * @stats_lock: Lock protecting trans_table, time_in_state, > - * last_time and total_trans used for statistics > + * @stats: Statistics of devfreq states and state transitions > */ > struct devfreq_dev_profile { > unsigned long initial_freq; > @@ -108,14 +124,7 @@ struct devfreq_dev_profile { > int (*get_cur_freq)(struct device *dev, unsigned long *freq); > void (*exit)(struct device *dev); > > - unsigned long *freq_table; > - unsigned int max_state; > - /* information for device frequency transition */ > - unsigned int total_trans; > - unsigned int *trans_table; > - u64 *time_in_state; > - unsigned long long last_time; > - spinlock_t stats_lock; > + struct devfreq_stats *stats; > }; > > /** >
