[7/7] devfreq: move statistics to separate struct
diff mbox series

Message ID 20191113091336.5218-8-k.konieczny@samsung.com
State New
Headers show
Series
  • Untitled series #202217
Related show

Commit Message

Kamil Konieczny Nov. 13, 2019, 9:13 a.m. UTC
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(-)

Comments

Chanwoo Choi Nov. 14, 2019, 1:52 a.m. UTC | #1
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;
>  };
>  
>  /**
>
Bartlomiej Zolnierkiewicz Nov. 14, 2019, 6:01 p.m. UTC | #2
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;
>>  };
>>  
>>  /**
>>
Chanwoo Choi Nov. 15, 2019, 3:25 a.m. UTC | #3
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;
>>>  };
>>>  
>>>  /**
>>>
> 
>
Chanwoo Choi Nov. 15, 2019, 6:21 a.m. UTC | #4
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;
>>>>  };
>>>>  
>>>>  /**
>>>>
>>
>>
> 
>
Bartlomiej Zolnierkiewicz Nov. 15, 2019, 12:40 p.m. UTC | #5
[ 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;
>>>>>  };
>>>>>  
>>>>>  /**
>>>>>

Patch
diff mbox series

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;
 };
 
 /**