* [PATCH 0/7] devfreq: improve devfreq statistics counting
[not found] <CGME20191113091350eucas1p2545166dfa1dc3b85aee375e353d7a604@eucas1p2.samsung.com>
@ 2019-11-13 9:13 ` Kamil Konieczny
[not found] ` <CGME20191113091354eucas1p265de4985d167814f5080fbdf21b75a0a@eucas1p2.samsung.com>
0 siblings, 1 reply; 11+ messages in thread
From: Kamil Konieczny @ 2019-11-13 9:13 UTC (permalink / raw)
To: k.konieczny
Cc: linux-samsung-soc, Bartlomiej Zolnierkiewicz, linux-pm,
linux-kernel, Krzysztof Kozlowski, Chanwoo Choi, Kyungmin Park,
Kukjin Kim, MyungJoo Ham, Kamil Konieczny, linux-arm-kernel,
Marek Szyprowski
Hi,
this patch series improves devfreq statistics:
- do conversion to use 64-bit jiffies for storing elapsed time and prevent counters
overflow,
- add ability to reset statistics using sysfs,
- move statistics data to separate structure for improved code
readability and maintenance,
- make devfreq statistics code more similar to cpufreq statistics
code for improved long-term maintainability
The first four patches fix time stats to use 64-bits, add spinlock for protecting data
access, add new function in sysfs for clearing statistics counters and change var name
used in time counters. Remaining patches make steps to moving stats into separate
structure devfreq_stats.
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
Kamil Konieczny (7):
devfreq: change time stats to 64-bit
devfreq: protect devfreq stats data with spinlock
devfreq: add clearing transitions stats in sysfs
devfreq: change var name used in time statistics
devfreq: move transition statistics to devfreq profile structure
devfreq: move transition statistics allocations to set_freq_stats()
devfreq: move statistics to separate struct
drivers/devfreq/devfreq.c | 199 ++++++++++++++++++-----------
drivers/devfreq/exynos-bus.c | 6 +-
drivers/devfreq/governor_passive.c | 26 ++--
include/linux/devfreq.h | 41 +++---
4 files changed, 167 insertions(+), 105 deletions(-)
--
2.24.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 7/7] devfreq: move statistics to separate struct
[not found] ` <CGME20191113091354eucas1p265de4985d167814f5080fbdf21b75a0a@eucas1p2.samsung.com>
@ 2019-11-13 9:13 ` Kamil Konieczny
2019-11-14 1:52 ` Chanwoo Choi
0 siblings, 1 reply; 11+ messages in thread
From: Kamil Konieczny @ 2019-11-13 9:13 UTC (permalink / raw)
To: k.konieczny
Cc: linux-samsung-soc, Bartlomiej Zolnierkiewicz, linux-pm,
linux-kernel, Krzysztof Kozlowski, Chanwoo Choi, Kyungmin Park,
Kukjin Kim, MyungJoo Ham, Kamil Konieczny, linux-arm-kernel,
Marek Szyprowski
Count time and transitions between devfreq frequencies in separate struct
for improved code readability and maintenance.
Signed-off-by: Kamil Konieczny <k.konieczny@samsung.com>
---
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;
};
/**
--
2.24.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 7/7] devfreq: move statistics to separate struct
2019-11-13 9:13 ` [PATCH 7/7] devfreq: move statistics to separate struct Kamil Konieczny
@ 2019-11-14 1:52 ` Chanwoo Choi
2019-11-14 18:01 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 11+ messages in thread
From: Chanwoo Choi @ 2019-11-14 1:52 UTC (permalink / raw)
To: Kamil Konieczny
Cc: linux-samsung-soc, Bartlomiej Zolnierkiewicz, linux-pm,
linux-kernel, Krzysztof Kozlowski, Kyungmin Park, Kukjin Kim,
MyungJoo Ham, Kamil Konieczny, linux-arm-kernel,
Marek Szyprowski
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@samsung.com>
> ---
> 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;
> };
>
> /**
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 7/7] devfreq: move statistics to separate struct
2019-11-14 1:52 ` Chanwoo Choi
@ 2019-11-14 18:01 ` Bartlomiej Zolnierkiewicz
2019-11-15 3:25 ` Chanwoo Choi
0 siblings, 1 reply; 11+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2019-11-14 18:01 UTC (permalink / raw)
To: Chanwoo Choi
Cc: linux-samsung-soc, linux-pm, Kamil Konieczny, linux-kernel,
Krzysztof Kozlowski, Kyungmin Park, Kukjin Kim, MyungJoo Ham,
Kamil Konieczny, linux-arm-kernel, Marek Szyprowski
Hi Chanwoo,
On 11/14/19 2:52 AM, Chanwoo Choi wrote:
> 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.
This argument is not a a technical one and the official upstream
kernel policy is to not depend on out-of-tree drivers.
Besides the ARM Mali drivers are full of code like:
#if LINUX_VERSION_CODE >= KERNEL_VERSION(x, y, z)
...
#else
...
#endif
so few more instances of similar code won't do any harm.. ;-)
> [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
I took a look at ARM Mali drivers source code anyway and I fail to
see a rationale behind their behavior of doing 'freq_table' and
'max_state' initialization in the driver itself (instead of leaving
it up to the devfreq core code, like all in-kernel drivers are doing
already).
Could you please explain rationale behind ARM Mali drivers' special
needs?
[ Both ARM Mali and devfreq core code are using generic PM OPP code
these days to do 'freq_table' and 'max_state' initialization, the
only difference seems to be that ARM Mali creates the frequency
table in the descending order (but there also seems to be no real
need for it). ]
Maybe this is an opportunity to simplify also the ARM Mali driver?
> 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.
Well, 'struct devfreq_stats *stats' can be trivially moved out of
'struct devfreq_profile' to 'struct devfreq' if you prefer it that
way..
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
> 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@samsung.com>
>> ---
>> 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;
>> };
>>
>> /**
>>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 7/7] devfreq: move statistics to separate struct
2019-11-14 18:01 ` Bartlomiej Zolnierkiewicz
@ 2019-11-15 3:25 ` Chanwoo Choi
2019-11-15 6:21 ` Chanwoo Choi
0 siblings, 1 reply; 11+ messages in thread
From: Chanwoo Choi @ 2019-11-15 3:25 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: linux-samsung-soc, linux-pm, Kamil Konieczny, linux-kernel,
Krzysztof Kozlowski, Kyungmin Park, Kukjin Kim, MyungJoo Ham,
linux-arm-kernel, Marek Szyprowski
Hi Bartlomiej,
On 11/15/19 3:01 AM, Bartlomiej Zolnierkiewicz wrote:
>
> Hi Chanwoo,
>
> On 11/14/19 2:52 AM, Chanwoo Choi wrote:
>> 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.
>
> This argument is not a a technical one and the official upstream
> kernel policy is to not depend on out-of-tree drivers.
>
> Besides the ARM Mali drivers are full of code like:
>
> #if LINUX_VERSION_CODE >= KERNEL_VERSION(x, y, z)
> ...
> #else
> ...
> #endif
>
> so few more instances of similar code won't do any harm.. ;-)
>
>> [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
>
> I took a look at ARM Mali drivers source code anyway and I fail to
> see a rationale behind their behavior of doing 'freq_table' and
> 'max_state' initialization in the driver itself (instead of leaving
> it up to the devfreq core code, like all in-kernel drivers are doing
> already).
>
> Could you please explain rationale behind ARM Mali drivers' special
> needs?
>
> [ Both ARM Mali and devfreq core code are using generic PM OPP code
> these days to do 'freq_table' and 'max_state' initialization, the
> only difference seems to be that ARM Mali creates the frequency
> table in the descending order (but there also seems to be no real
> need for it). ]
>
> Maybe this is an opportunity to simplify also the ARM Mali driver?
OK. I agree to simplify them on this time.
For touching the 'freq_table' and 'max_state', need to fix
the descending order of freq_table.
The partition_enable_ops() in the drivers/thermal/devfreq_cooling.c
requires the descending order of freq_table. Have to change it by using
the ascending time or support both ascending and descending order for freq_table.
1. Move freq_table, max_state of devfreq_dev_profile to struct devfreq
2. Edit partition_enable_ops() in the drivers/thermal/devfreq_cooling.c
by using ascending order instead of descending order.
>
>> 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.
>
> Well, 'struct devfreq_stats *stats' can be trivially moved out of
> 'struct devfreq_profile' to 'struct devfreq' if you prefer it that
> way..
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>
>> 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@samsung.com>
>>> ---
>>> 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;
>>> };
>>>
>>> /**
>>>
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 7/7] devfreq: move statistics to separate struct
2019-11-15 3:25 ` Chanwoo Choi
@ 2019-11-15 6:21 ` Chanwoo Choi
2019-11-15 12:40 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 11+ messages in thread
From: Chanwoo Choi @ 2019-11-15 6:21 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: linux-samsung-soc, linux-pm, Kamil Konieczny, linux-kernel,
Krzysztof Kozlowski, Kyungmin Park, Kukjin Kim, MyungJoo Ham,
linux-arm-kernel, Marek Szyprowski
Hi Bartlomiej,
On 11/15/19 12:25 PM, Chanwoo Choi wrote:
> Hi Bartlomiej,
>
> On 11/15/19 3:01 AM, Bartlomiej Zolnierkiewicz wrote:
>>
>> Hi Chanwoo,
>>
>> On 11/14/19 2:52 AM, Chanwoo Choi wrote:
>>> 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.
>>
>> This argument is not a a technical one and the official upstream
>> kernel policy is to not depend on out-of-tree drivers.
>>
>> Besides the ARM Mali drivers are full of code like:
>>
>> #if LINUX_VERSION_CODE >= KERNEL_VERSION(x, y, z)
>> ...
>> #else
>> ...
>> #endif
>>
>> so few more instances of similar code won't do any harm.. ;-)
>>
>>> [1] https://protect2.fireeye.com/url?k=909caa5c-cd52abe8-909d2113-000babdfecba-812f16576c3614a3&u=https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/bifrost-kernel#
>>> [2] https://protect2.fireeye.com/url?k=33265f96-6ee85e22-3327d4d9-000babdfecba-44c2daec328712e6&u=https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/midgard-kernel
>>> [3] https://protect2.fireeye.com/url?k=69bdcab0-3473cb04-69bc41ff-000babdfecba-4b576facf85e0208&u=https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/utgard-kernel
>>
>> I took a look at ARM Mali drivers source code anyway and I fail to
>> see a rationale behind their behavior of doing 'freq_table' and
>> 'max_state' initialization in the driver itself (instead of leaving
>> it up to the devfreq core code, like all in-kernel drivers are doing
>> already).
>>
>> Could you please explain rationale behind ARM Mali drivers' special
>> needs?
>>
>> [ Both ARM Mali and devfreq core code are using generic PM OPP code
>> these days to do 'freq_table' and 'max_state' initialization, the
>> only difference seems to be that ARM Mali creates the frequency
>> table in the descending order (but there also seems to be no real
>> need for it). ]
>>
>> Maybe this is an opportunity to simplify also the ARM Mali driver?
>
> OK. I agree to simplify them on this time.
> For touching the 'freq_table' and 'max_state', need to fix
> the descending order of freq_table.
>
> The partition_enable_ops() in the drivers/thermal/devfreq_cooling.c
> requires the descending order of freq_table. Have to change it by using
> the ascending time or support both ascending and descending order for freq_table.
>
> 1. Move freq_table, max_state of devfreq_dev_profile to struct devfreq
> 2. Edit partition_enable_ops() in the drivers/thermal/devfreq_cooling.c
> by using ascending order instead of descending order.
>
After changed about 'freq_table' and 'max_state', the build error
will happen on ARM mail driver because the initialization code of
'freq_table' in ARM mali driver, didn't check the kernel version.
The first devfreq patch provided the 'freq_table' as optional variable
in the 'struct devfreq_dev_profile'. Even if ARM mali driver is out of mainline tree,
this change seems that break the usage rule of 'freq_table' in 'struct devfreq_dev_profile'.
So, if there are no any beneficial reason, I just keep the current status of 'freq_table'
in order to keep the previous usage rule of 'freq_table' in 'struct devfreq_dev_profile'.
Frankly, I'm note sure that it is necessary. I don't want to make
the side-effect without any useful reason.
But,
Separately, have to fix the ordering issue of partition_enable_ops()
in the drivers/thermal/devfreq_cooling.c.
>>
>>> 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.
>>
>> Well, 'struct devfreq_stats *stats' can be trivially moved out of
>> 'struct devfreq_profile' to 'struct devfreq' if you prefer it that
>> way..
>>
>> Best regards,
>> --
>> Bartlomiej Zolnierkiewicz
>> Samsung R&D Institute Poland
>> Samsung Electronics
>>
>>> 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@samsung.com>
>>>> ---
>>>> 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;
>>>> };
>>>>
>>>> /**
>>>>
>>
>>
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 7/7] devfreq: move statistics to separate struct
2019-11-15 6:21 ` Chanwoo Choi
@ 2019-11-15 12:40 ` Bartlomiej Zolnierkiewicz
2019-12-16 13:01 ` Lukasz Luba
0 siblings, 1 reply; 11+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2019-11-15 12:40 UTC (permalink / raw)
To: Chanwoo Choi
Cc: Javi Merino, linux-samsung-soc, linux-pm, Kamil Konieczny,
linux-kernel, Krzysztof Kozlowski, Eduardo Valentin,
Kyungmin Park, Kukjin Kim, MyungJoo Ham, Zhang Rui,
Ørjan Eide, linux-arm-kernel, Marek Szyprowski
[ added Zhang, Eduardo, Ørjan and Javi to Cc: ]
On 11/15/19 7:21 AM, Chanwoo Choi wrote:
> Hi Bartlomiej,
>
> On 11/15/19 12:25 PM, Chanwoo Choi wrote:
>> Hi Bartlomiej,
>>
>> On 11/15/19 3:01 AM, Bartlomiej Zolnierkiewicz wrote:
>>>
>>> Hi Chanwoo,
>>>
>>> On 11/14/19 2:52 AM, Chanwoo Choi wrote:
>>>> 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.
>>>
>>> This argument is not a a technical one and the official upstream
>>> kernel policy is to not depend on out-of-tree drivers.
>>>
>>> Besides the ARM Mali drivers are full of code like:
>>>
>>> #if LINUX_VERSION_CODE >= KERNEL_VERSION(x, y, z)
>>> ...
>>> #else
>>> ...
>>> #endif
>>>
>>> so few more instances of similar code won't do any harm.. ;-)
>>>
>>>> [1] https://protect2.fireeye.com/url?k=909caa5c-cd52abe8-909d2113-000babdfecba-812f16576c3614a3&u=https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/bifrost-kernel#
>>>> [2] https://protect2.fireeye.com/url?k=33265f96-6ee85e22-3327d4d9-000babdfecba-44c2daec328712e6&u=https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/midgard-kernel
>>>> [3] https://protect2.fireeye.com/url?k=69bdcab0-3473cb04-69bc41ff-000babdfecba-4b576facf85e0208&u=https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/utgard-kernel
>>>
>>> I took a look at ARM Mali drivers source code anyway and I fail to
>>> see a rationale behind their behavior of doing 'freq_table' and
>>> 'max_state' initialization in the driver itself (instead of leaving
>>> it up to the devfreq core code, like all in-kernel drivers are doing
>>> already).
>>>
>>> Could you please explain rationale behind ARM Mali drivers' special
>>> needs?
>>>
>>> [ Both ARM Mali and devfreq core code are using generic PM OPP code
>>> these days to do 'freq_table' and 'max_state' initialization, the
>>> only difference seems to be that ARM Mali creates the frequency
>>> table in the descending order (but there also seems to be no real
>>> need for it). ]
>>>
>>> Maybe this is an opportunity to simplify also the ARM Mali driver?
>>
>> OK. I agree to simplify them on this time.
>> For touching the 'freq_table' and 'max_state', need to fix
>> the descending order of freq_table.
>>
>> The partition_enable_ops() in the drivers/thermal/devfreq_cooling.c
>> requires the descending order of freq_table. Have to change it by using
>> the ascending time or support both ascending and descending order for freq_table.
>>
>> 1. Move freq_table, max_state of devfreq_dev_profile to struct devfreq
>> 2. Edit partition_enable_ops() in the drivers/thermal/devfreq_cooling.c
>> by using ascending order instead of descending order.
>>
>
> After changed about 'freq_table' and 'max_state', the build error
> will happen on ARM mail driver because the initialization code of
> 'freq_table' in ARM mali driver, didn't check the kernel version.
>
> The first devfreq patch provided the 'freq_table' as optional variable
> in the 'struct devfreq_dev_profile'. Even if ARM mali driver is out of mainline tree,
> this change seems that break the usage rule of 'freq_table' in 'struct devfreq_dev_profile'.
>
> So, if there are no any beneficial reason, I just keep the current status of 'freq_table'
> in order to keep the previous usage rule of 'freq_table' in 'struct devfreq_dev_profile'.
>
> Frankly, I'm note sure that it is necessary. I don't want to make
> the side-effect without any useful reason.
>
> But,
> Separately, have to fix the ordering issue of partition_enable_ops()
> in the drivers/thermal/devfreq_cooling.c.
Hmmm.. fixing partition_enable_opps() should be trivial but I wonder
why we are carrying devfreq_cooling.c code in upstream kernel at all?
It has been merged in the following commit:
commit a76caf55e5b356ba20a5a43ac4d9f7a04b20941d
Author: Ørjan Eide <orjan.eide@arm.com>
Date: Thu Sep 10 18:09:30 2015 +0100
thermal: Add devfreq cooling
Add a generic thermal cooling device for devfreq, that is similar to
cpu_cooling.
The device must use devfreq. In order to use the power extension of the
cooling device, it must have registered its OPPs using the OPP library.
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>
Signed-off-by: Ørjan Eide <orjan.eide@arm.com>
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
...
but 4 years later there is still no single in-kernel user for this code?
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
>>>
>>>> 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.
>>>
>>> Well, 'struct devfreq_stats *stats' can be trivially moved out of
>>> 'struct devfreq_profile' to 'struct devfreq' if you prefer it that
>>> way..
>>>
>>> Best regards,
>>> --
>>> Bartlomiej Zolnierkiewicz
>>> Samsung R&D Institute Poland
>>> Samsung Electronics
>>>
>>>> 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@samsung.com>
>>>>> ---
>>>>> 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;
>>>>> };
>>>>>
>>>>> /**
>>>>>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Re: [PATCH 7/7] devfreq: move statistics to separate struct
2019-11-15 12:40 ` Bartlomiej Zolnierkiewicz
@ 2019-12-16 13:01 ` Lukasz Luba
2019-12-17 0:07 ` Chanwoo Choi
2020-01-15 15:56 ` Bartlomiej Zolnierkiewicz
0 siblings, 2 replies; 11+ messages in thread
From: Lukasz Luba @ 2019-12-16 13:01 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: a.hajda, Javi Merino, linux-samsung-soc, linux-pm,
Kamil Konieczny, linux-kernel, Krzysztof Kozlowski,
Dietmar Eggemann, Eduardo Valentin, Chanwoo Choi, Kyungmin Park,
Kukjin Kim, MyungJoo Ham, Zhang Rui, robin.murphy,
Ørjan Eide, linux-arm-kernel, Marek Szyprowski
Hi Bartek,
[added Dietmar, Robin, Andrzej (for upcoming DRM drm-misc-next)]
On 11/15/19 12:40 PM, Bartlomiej Zolnierkiewicz wrote:
>
> [ added Zhang, Eduardo, Ørjan and Javi to Cc: ]
>
> On 11/15/19 7:21 AM, Chanwoo Choi wrote:
>> Hi Bartlomiej,
>>
>> On 11/15/19 12:25 PM, Chanwoo Choi wrote:
>>> Hi Bartlomiej,
>>>
>>> On 11/15/19 3:01 AM, Bartlomiej Zolnierkiewicz wrote:
>>>>
>>>> Hi Chanwoo,
>>>>
>>>> On 11/14/19 2:52 AM, Chanwoo Choi wrote:
>>>>> 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.
>>>>
>>>> This argument is not a a technical one and the official upstream
>>>> kernel policy is to not depend on out-of-tree drivers.
>>>>
>>>> Besides the ARM Mali drivers are full of code like:
>>>>
>>>> #if LINUX_VERSION_CODE >= KERNEL_VERSION(x, y, z)
>>>> ...
>>>> #else
>>>> ...
>>>> #endif
>>>>
>>>> so few more instances of similar code won't do any harm.. ;-)
>>>>
>>>>> [1] https://protect2.fireeye.com/url?k=909caa5c-cd52abe8-909d2113-000babdfecba-812f16576c3614a3&u=https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/bifrost-kernel#
>>>>> [2] https://protect2.fireeye.com/url?k=33265f96-6ee85e22-3327d4d9-000babdfecba-44c2daec328712e6&u=https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/midgard-kernel
>>>>> [3] https://protect2.fireeye.com/url?k=69bdcab0-3473cb04-69bc41ff-000babdfecba-4b576facf85e0208&u=https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/utgard-kernel
>>>>
>>>> I took a look at ARM Mali drivers source code anyway and I fail to
>>>> see a rationale behind their behavior of doing 'freq_table' and
>>>> 'max_state' initialization in the driver itself (instead of leaving
>>>> it up to the devfreq core code, like all in-kernel drivers are doing
>>>> already).
>>>>
>>>> Could you please explain rationale behind ARM Mali drivers' special
>>>> needs?
>>>>
>>>> [ Both ARM Mali and devfreq core code are using generic PM OPP code
>>>> these days to do 'freq_table' and 'max_state' initialization, the
>>>> only difference seems to be that ARM Mali creates the frequency
>>>> table in the descending order (but there also seems to be no real
>>>> need for it). ]
>>>>
>>>> Maybe this is an opportunity to simplify also the ARM Mali driver?
>>>
>>> OK. I agree to simplify them on this time.
>>> For touching the 'freq_table' and 'max_state', need to fix
>>> the descending order of freq_table.
>>>
>>> The partition_enable_ops() in the drivers/thermal/devfreq_cooling.c
>>> requires the descending order of freq_table. Have to change it by using
>>> the ascending time or support both ascending and descending order for freq_table.
>>>
>>> 1. Move freq_table, max_state of devfreq_dev_profile to struct devfreq
>>> 2. Edit partition_enable_ops() in the drivers/thermal/devfreq_cooling.c
>>> by using ascending order instead of descending order.
>>>
>>
>> After changed about 'freq_table' and 'max_state', the build error
>> will happen on ARM mail driver because the initialization code of
>> 'freq_table' in ARM mali driver, didn't check the kernel version.
>>
>> The first devfreq patch provided the 'freq_table' as optional variable
>> in the 'struct devfreq_dev_profile'. Even if ARM mali driver is out of mainline tree,
>> this change seems that break the usage rule of 'freq_table' in 'struct devfreq_dev_profile'.
>>
>> So, if there are no any beneficial reason, I just keep the current status of 'freq_table'
>> in order to keep the previous usage rule of 'freq_table' in 'struct devfreq_dev_profile'.
>>
>> Frankly, I'm note sure that it is necessary. I don't want to make
>> the side-effect without any useful reason.
>>
>> But,
>> Separately, have to fix the ordering issue of partition_enable_ops()
>> in the drivers/thermal/devfreq_cooling.c.
>
> Hmmm.. fixing partition_enable_opps() should be trivial but I wonder
> why we are carrying devfreq_cooling.c code in upstream kernel at all?
Well, the devfreq_cooling.c is going to have a client in mainline:
the GPU driver - Panfrost.
It is already in DRM branch 'drm-misc-next':
https://patchwork.freedesktop.org/patch/342848/
Regarding the devfreq_cooling.c code structure.
I am currently working on cleaning up the devfreq cooling code and
adding Energy Model instead for private freq, power tables. It will be
in similar fashion as it is done in cpufreq_cooling. The model will
be also simplified so hopefully more clients would come.
It is under internal review and will be posted shortly.
>
> It has been merged in the following commit:
>
> commit a76caf55e5b356ba20a5a43ac4d9f7a04b20941d
> Author: Ørjan Eide <orjan.eide@arm.com>
> Date: Thu Sep 10 18:09:30 2015 +0100
>
> thermal: Add devfreq cooling
>
> Add a generic thermal cooling device for devfreq, that is similar to
> cpu_cooling.
>
> The device must use devfreq. In order to use the power extension of the
> cooling device, it must have registered its OPPs using the OPP library.
>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> Signed-off-by: Ørjan Eide <orjan.eide@arm.com>
> Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
> ...
>
> but 4 years later there is still no single in-kernel user for this code?
There will be, via DRM tree.
Regards,
Lukasz
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>
>>>>
>>>>> 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.
>>>>
>>>> Well, 'struct devfreq_stats *stats' can be trivially moved out of
>>>> 'struct devfreq_profile' to 'struct devfreq' if you prefer it that
>>>> way..
>>>>
>>>> Best regards,
>>>> --
>>>> Bartlomiej Zolnierkiewicz
>>>> Samsung R&D Institute Poland
>>>> Samsung Electronics
>>>>
>>>>> 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@samsung.com>
>>>>>> ---
>>>>>> 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;
>>>>>> };
>>>>>>
>>>>>> /**
>>>>>>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 7/7] devfreq: move statistics to separate struct
2019-12-16 13:01 ` Lukasz Luba
@ 2019-12-17 0:07 ` Chanwoo Choi
2019-12-17 9:10 ` Lukasz Luba
2020-01-15 15:56 ` Bartlomiej Zolnierkiewicz
1 sibling, 1 reply; 11+ messages in thread
From: Chanwoo Choi @ 2019-12-17 0:07 UTC (permalink / raw)
To: Lukasz Luba, Bartlomiej Zolnierkiewicz
Cc: Javi Merino, linux-samsung-soc, linux-pm, Kamil Konieczny,
linux-kernel, Krzysztof Kozlowski, Dietmar Eggemann,
Eduardo Valentin, a.hajda, Kyungmin Park, Kukjin Kim,
MyungJoo Ham, Zhang Rui, robin.murphy, Ørjan Eide,
linux-arm-kernel, Marek Szyprowski
Hi Lukaz,
On 12/16/19 10:01 PM, Lukasz Luba wrote:
> Hi Bartek,
>
> [added Dietmar, Robin, Andrzej (for upcoming DRM drm-misc-next)]
>
> On 11/15/19 12:40 PM, Bartlomiej Zolnierkiewicz wrote:
>>
>> [ added Zhang, Eduardo, Ørjan and Javi to Cc: ]
>>
>> On 11/15/19 7:21 AM, Chanwoo Choi wrote:
>>> Hi Bartlomiej,
>>>
>>> On 11/15/19 12:25 PM, Chanwoo Choi wrote:
>>>> Hi Bartlomiej,
>>>>
>>>> On 11/15/19 3:01 AM, Bartlomiej Zolnierkiewicz wrote:
>>>>>
>>>>> Hi Chanwoo,
>>>>>
>>>>> On 11/14/19 2:52 AM, Chanwoo Choi wrote:
>>>>>> 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.
>>>>>
>>>>> This argument is not a a technical one and the official upstream
>>>>> kernel policy is to not depend on out-of-tree drivers.
>>>>>
>>>>> Besides the ARM Mali drivers are full of code like:
>>>>>
>>>>> #if LINUX_VERSION_CODE >= KERNEL_VERSION(x, y, z)
>>>>> ...
>>>>> #else
>>>>> ...
>>>>> #endif
>>>>>
>>>>> so few more instances of similar code won't do any harm.. ;-)
>>>>>
>>>>>> [1] https://protect2.fireeye.com/url?k=909caa5c-cd52abe8-909d2113-000babdfecba-812f16576c3614a3&u=https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/bifrost-kernel#
>>>>>> [2] https://protect2.fireeye.com/url?k=33265f96-6ee85e22-3327d4d9-000babdfecba-44c2daec328712e6&u=https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/midgard-kernel
>>>>>> [3] https://protect2.fireeye.com/url?k=69bdcab0-3473cb04-69bc41ff-000babdfecba-4b576facf85e0208&u=https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/utgard-kernel
>>>>>
>>>>> I took a look at ARM Mali drivers source code anyway and I fail to
>>>>> see a rationale behind their behavior of doing 'freq_table' and
>>>>> 'max_state' initialization in the driver itself (instead of leaving
>>>>> it up to the devfreq core code, like all in-kernel drivers are doing
>>>>> already).
>>>>>
>>>>> Could you please explain rationale behind ARM Mali drivers' special
>>>>> needs?
>>>>>
>>>>> [ Both ARM Mali and devfreq core code are using generic PM OPP code
>>>>> these days to do 'freq_table' and 'max_state' initialization, the
>>>>> only difference seems to be that ARM Mali creates the frequency
>>>>> table in the descending order (but there also seems to be no real
>>>>> need for it). ]
>>>>>
>>>>> Maybe this is an opportunity to simplify also the ARM Mali driver?
>>>>
>>>> OK. I agree to simplify them on this time.
>>>> For touching the 'freq_table' and 'max_state', need to fix
>>>> the descending order of freq_table.
>>>>
>>>> The partition_enable_ops() in the drivers/thermal/devfreq_cooling.c
>>>> requires the descending order of freq_table. Have to change it by using
>>>> the ascending time or support both ascending and descending order for freq_table.
>>>>
>>>> 1. Move freq_table, max_state of devfreq_dev_profile to struct devfreq
>>>> 2. Edit partition_enable_ops() in the drivers/thermal/devfreq_cooling.c
>>>> by using ascending order instead of descending order.
>>>>
>>>
>>> After changed about 'freq_table' and 'max_state', the build error
>>> will happen on ARM mail driver because the initialization code of
>>> 'freq_table' in ARM mali driver, didn't check the kernel version.
>>>
>>> The first devfreq patch provided the 'freq_table' as optional variable
>>> in the 'struct devfreq_dev_profile'. Even if ARM mali driver is out of mainline tree,
>>> this change seems that break the usage rule of 'freq_table' in 'struct devfreq_dev_profile'.
>>>
>>> So, if there are no any beneficial reason, I just keep the current status of 'freq_table'
>>> in order to keep the previous usage rule of 'freq_table' in 'struct devfreq_dev_profile'.
>>>
>>> Frankly, I'm note sure that it is necessary. I don't want to make
>>> the side-effect without any useful reason.
>>>
>>> But,
>>> Separately, have to fix the ordering issue of partition_enable_ops()
>>> in the drivers/thermal/devfreq_cooling.c.
>>
>> Hmmm.. fixing partition_enable_opps() should be trivial but I wonder
>> why we are carrying devfreq_cooling.c code in upstream kernel at all?
>
> Well, the devfreq_cooling.c is going to have a client in mainline:
> the GPU driver - Panfrost.
>
> It is already in DRM branch 'drm-misc-next':
> https://protect2.fireeye.com/url?k=75a0e087-283b1ce4-75a16bc8-0cc47a31cdbc-4953aa9e0574f6dc&u=https://patchwork.freedesktop.org/patch/342848/
>
> Regarding the devfreq_cooling.c code structure.
> I am currently working on cleaning up the devfreq cooling code and
> adding Energy Model instead for private freq, power tables. It will be
> in similar fashion as it is done in cpufreq_cooling. The model will
> be also simplified so hopefully more clients would come.
> It is under internal review and will be posted shortly.
Good news about Energy Model. When you send the patch related to Energy model,
please add me to Cc list.
>
>>
>> It has been merged in the following commit:
>>
>> commit a76caf55e5b356ba20a5a43ac4d9f7a04b20941d
>> Author: Ørjan Eide <orjan.eide@arm.com>
>> Date: Thu Sep 10 18:09:30 2015 +0100
>>
>> thermal: Add devfreq cooling
>> Add a generic thermal cooling device for devfreq, that is similar to
>> cpu_cooling.
>> The device must use devfreq. In order to use the power extension of the
>> cooling device, it must have registered its OPPs using the OPP library.
>> Cc: Zhang Rui <rui.zhang@intel.com>
>> Cc: Eduardo Valentin <edubezval@gmail.com>
>> Signed-off-by: Javi Merino <javi.merino@arm.com>
>> Signed-off-by: Ørjan Eide <orjan.eide@arm.com>
>> Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
>> ...
>>
>> but 4 years later there is still no single in-kernel user for this code?
>
> There will be, via DRM tree.
>
> Regards,
> Lukasz
>
>>
>> Best regards,
>> --
>> Bartlomiej Zolnierkiewicz
>> Samsung R&D Institute Poland
>> Samsung Electronics
>>
>>>>>
>>>>>> 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.
>>>>>
>>>>> Well, 'struct devfreq_stats *stats' can be trivially moved out of
>>>>> 'struct devfreq_profile' to 'struct devfreq' if you prefer it that
>>>>> way..
>>>>>
>>>>> Best regards,
>>>>> --
>>>>> Bartlomiej Zolnierkiewicz
>>>>> Samsung R&D Institute Poland
>>>>> Samsung Electronics
>>>>>
>>>>>> 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@samsung.com>
>>>>>>> ---
>>>>>>> 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;
>>>>>>> };
>>>>>>> /**
>>>>>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> https://protect2.fireeye.com/url?k=856913bb-d8f2efd8-856898f4-0cc47a31cdbc-5cb40ce5f31ed8ed&u=http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 7/7] devfreq: move statistics to separate struct
2019-12-17 0:07 ` Chanwoo Choi
@ 2019-12-17 9:10 ` Lukasz Luba
0 siblings, 0 replies; 11+ messages in thread
From: Lukasz Luba @ 2019-12-17 9:10 UTC (permalink / raw)
To: Chanwoo Choi
Cc: Javi Merino, linux-samsung-soc, linux-pm, Kamil Konieczny,
Bartlomiej Zolnierkiewicz, linux-kernel, Krzysztof Kozlowski,
Dietmar Eggemann, Eduardo Valentin, a.hajda, Kyungmin Park,
Kukjin Kim, MyungJoo Ham, Zhang Rui, robin.murphy,
Ørjan Eide, linux-arm-kernel, Marek Szyprowski
Hi Chanwoo,
On 12/17/19 12:07 AM, Chanwoo Choi wrote:
> Hi Lukaz,
>
> On 12/16/19 10:01 PM, Lukasz Luba wrote:
>> Hi Bartek,
>>
>> [added Dietmar, Robin, Andrzej (for upcoming DRM drm-misc-next)]
>>
>> On 11/15/19 12:40 PM, Bartlomiej Zolnierkiewicz wrote:
>>>
>>> [ added Zhang, Eduardo, Ørjan and Javi to Cc: ]
>>>
>>> On 11/15/19 7:21 AM, Chanwoo Choi wrote:
>>>> Hi Bartlomiej,
>>>>
>>>> On 11/15/19 12:25 PM, Chanwoo Choi wrote:
>>>>> Hi Bartlomiej,
>>>>>
>>>>> On 11/15/19 3:01 AM, Bartlomiej Zolnierkiewicz wrote:
>>>>>>
>>>>>> Hi Chanwoo,
>>>>>>
>>>>>> On 11/14/19 2:52 AM, Chanwoo Choi wrote:
>>>>>>> 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.
>>>>>>
>>>>>> This argument is not a a technical one and the official upstream
>>>>>> kernel policy is to not depend on out-of-tree drivers.
>>>>>>
>>>>>> Besides the ARM Mali drivers are full of code like:
>>>>>>
>>>>>> #if LINUX_VERSION_CODE >= KERNEL_VERSION(x, y, z)
>>>>>> ...
>>>>>> #else
>>>>>> ...
>>>>>> #endif
>>>>>>
>>>>>> so few more instances of similar code won't do any harm.. ;-)
>>>>>>
>>>>>>> [1] https://protect2.fireeye.com/url?k=909caa5c-cd52abe8-909d2113-000babdfecba-812f16576c3614a3&u=https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/bifrost-kernel#
>>>>>>> [2] https://protect2.fireeye.com/url?k=33265f96-6ee85e22-3327d4d9-000babdfecba-44c2daec328712e6&u=https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/midgard-kernel
>>>>>>> [3] https://protect2.fireeye.com/url?k=69bdcab0-3473cb04-69bc41ff-000babdfecba-4b576facf85e0208&u=https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/utgard-kernel
>>>>>>
>>>>>> I took a look at ARM Mali drivers source code anyway and I fail to
>>>>>> see a rationale behind their behavior of doing 'freq_table' and
>>>>>> 'max_state' initialization in the driver itself (instead of leaving
>>>>>> it up to the devfreq core code, like all in-kernel drivers are doing
>>>>>> already).
>>>>>>
>>>>>> Could you please explain rationale behind ARM Mali drivers' special
>>>>>> needs?
>>>>>>
>>>>>> [ Both ARM Mali and devfreq core code are using generic PM OPP code
>>>>>> these days to do 'freq_table' and 'max_state' initialization, the
>>>>>> only difference seems to be that ARM Mali creates the frequency
>>>>>> table in the descending order (but there also seems to be no real
>>>>>> need for it). ]
>>>>>>
>>>>>> Maybe this is an opportunity to simplify also the ARM Mali driver?
>>>>>
>>>>> OK. I agree to simplify them on this time.
>>>>> For touching the 'freq_table' and 'max_state', need to fix
>>>>> the descending order of freq_table.
>>>>>
>>>>> The partition_enable_ops() in the drivers/thermal/devfreq_cooling.c
>>>>> requires the descending order of freq_table. Have to change it by using
>>>>> the ascending time or support both ascending and descending order for freq_table.
>>>>>
>>>>> 1. Move freq_table, max_state of devfreq_dev_profile to struct devfreq
>>>>> 2. Edit partition_enable_ops() in the drivers/thermal/devfreq_cooling.c
>>>>> by using ascending order instead of descending order.
>>>>>
>>>>
>>>> After changed about 'freq_table' and 'max_state', the build error
>>>> will happen on ARM mail driver because the initialization code of
>>>> 'freq_table' in ARM mali driver, didn't check the kernel version.
>>>>
>>>> The first devfreq patch provided the 'freq_table' as optional variable
>>>> in the 'struct devfreq_dev_profile'. Even if ARM mali driver is out of mainline tree,
>>>> this change seems that break the usage rule of 'freq_table' in 'struct devfreq_dev_profile'.
>>>>
>>>> So, if there are no any beneficial reason, I just keep the current status of 'freq_table'
>>>> in order to keep the previous usage rule of 'freq_table' in 'struct devfreq_dev_profile'.
>>>>
>>>> Frankly, I'm note sure that it is necessary. I don't want to make
>>>> the side-effect without any useful reason.
>>>>
>>>> But,
>>>> Separately, have to fix the ordering issue of partition_enable_ops()
>>>> in the drivers/thermal/devfreq_cooling.c.
>>>
>>> Hmmm.. fixing partition_enable_opps() should be trivial but I wonder
>>> why we are carrying devfreq_cooling.c code in upstream kernel at all?
>>
>> Well, the devfreq_cooling.c is going to have a client in mainline:
>> the GPU driver - Panfrost.
>>
>> It is already in DRM branch 'drm-misc-next':
>> https://protect2.fireeye.com/url?k=75a0e087-283b1ce4-75a16bc8-0cc47a31cdbc-4953aa9e0574f6dc&u=https://patchwork.freedesktop.org/patch/342848/
>>
>> Regarding the devfreq_cooling.c code structure.
>> I am currently working on cleaning up the devfreq cooling code and
>> adding Energy Model instead for private freq, power tables. It will be
>> in similar fashion as it is done in cpufreq_cooling. The model will
>> be also simplified so hopefully more clients would come.
>> It is under internal review and will be posted shortly.
>
> Good news about Energy Model. When you send the patch related to Energy model,
> please add me to Cc list.
I will add you, thanks. More eyeballs in capturing bugs are more than
welcome.
Regards,
Lukasz
>
>>
>>>
>>> It has been merged in the following commit:
>>>
>>> commit a76caf55e5b356ba20a5a43ac4d9f7a04b20941d
>>> Author: Ørjan Eide <orjan.eide@arm.com>
>>> Date: Thu Sep 10 18:09:30 2015 +0100
>>>
>>> thermal: Add devfreq cooling
>>> Add a generic thermal cooling device for devfreq, that is similar to
>>> cpu_cooling.
>>> The device must use devfreq. In order to use the power extension of the
>>> cooling device, it must have registered its OPPs using the OPP library.
>>> Cc: Zhang Rui <rui.zhang@intel.com>
>>> Cc: Eduardo Valentin <edubezval@gmail.com>
>>> Signed-off-by: Javi Merino <javi.merino@arm.com>
>>> Signed-off-by: Ørjan Eide <orjan.eide@arm.com>
>>> Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
>>> ...
>>>
>>> but 4 years later there is still no single in-kernel user for this code?
>>
>> There will be, via DRM tree.
>>
>> Regards,
>> Lukasz
>>
>>>
>>> Best regards,
>>> --
>>> Bartlomiej Zolnierkiewicz
>>> Samsung R&D Institute Poland
>>> Samsung Electronics
>>>
>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> Well, 'struct devfreq_stats *stats' can be trivially moved out of
>>>>>> 'struct devfreq_profile' to 'struct devfreq' if you prefer it that
>>>>>> way..
>>>>>>
>>>>>> Best regards,
>>>>>> --
>>>>>> Bartlomiej Zolnierkiewicz
>>>>>> Samsung R&D Institute Poland
>>>>>> Samsung Electronics
>>>>>>
>>>>>>> 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@samsung.com>
>>>>>>>> ---
>>>>>>>> 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;
>>>>>>>> };
>>>>>>>> /**
>>>>>>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> https://protect2.fireeye.com/url?k=856913bb-d8f2efd8-856898f4-0cc47a31cdbc-5cb40ce5f31ed8ed&u=http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>
>>
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 7/7] devfreq: move statistics to separate struct
2019-12-16 13:01 ` Lukasz Luba
2019-12-17 0:07 ` Chanwoo Choi
@ 2020-01-15 15:56 ` Bartlomiej Zolnierkiewicz
1 sibling, 0 replies; 11+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2020-01-15 15:56 UTC (permalink / raw)
To: Lukasz Luba
Cc: a.hajda, Javi Merino, linux-samsung-soc, linux-pm,
Kamil Konieczny, linux-kernel, Krzysztof Kozlowski,
Dietmar Eggemann, Eduardo Valentin, Chanwoo Choi, Kyungmin Park,
Kukjin Kim, MyungJoo Ham, Zhang Rui, robin.murphy,
Ørjan Eide, linux-arm-kernel, Marek Szyprowski
On 12/16/19 2:01 PM, Lukasz Luba wrote:
> Hi Bartek,
>
> [added Dietmar, Robin, Andrzej (for upcoming DRM drm-misc-next)]
>
> On 11/15/19 12:40 PM, Bartlomiej Zolnierkiewicz wrote:
[...]
>> Hmmm.. fixing partition_enable_opps() should be trivial but I wonder
>> why we are carrying devfreq_cooling.c code in upstream kernel at all?
>
> Well, the devfreq_cooling.c is going to have a client in mainline:
> the GPU driver - Panfrost.
>
> It is already in DRM branch 'drm-misc-next':
> https://patchwork.freedesktop.org/patch/342848/
OK, thanks for explaining this.
> Regarding the devfreq_cooling.c code structure.
> I am currently working on cleaning up the devfreq cooling code and
> adding Energy Model instead for private freq, power tables. It will be
> in similar fashion as it is done in cpufreq_cooling. The model will
> be also simplified so hopefully more clients would come.
> It is under internal review and will be posted shortly.
Great to hear this and thank you for working on it.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-01-15 15:56 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20191113091350eucas1p2545166dfa1dc3b85aee375e353d7a604@eucas1p2.samsung.com>
2019-11-13 9:13 ` [PATCH 0/7] devfreq: improve devfreq statistics counting Kamil Konieczny
[not found] ` <CGME20191113091354eucas1p265de4985d167814f5080fbdf21b75a0a@eucas1p2.samsung.com>
2019-11-13 9:13 ` [PATCH 7/7] devfreq: move statistics to separate struct Kamil Konieczny
2019-11-14 1:52 ` Chanwoo Choi
2019-11-14 18:01 ` Bartlomiej Zolnierkiewicz
2019-11-15 3:25 ` Chanwoo Choi
2019-11-15 6:21 ` Chanwoo Choi
2019-11-15 12:40 ` Bartlomiej Zolnierkiewicz
2019-12-16 13:01 ` Lukasz Luba
2019-12-17 0:07 ` Chanwoo Choi
2019-12-17 9:10 ` Lukasz Luba
2020-01-15 15:56 ` Bartlomiej Zolnierkiewicz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).