[PATCH] PM / devfreq: Lock devfreq in trans_stat_show

[PATCH] PM / devfreq: Lock devfreq in trans_stat_show

[Leonard Crestez]

There is no locking in this sysfs show function so stats printing can
race with a devfreq_update_status called as part of freq switching or
with initialization.

Also add an assert in devfreq_update_status to make it clear that lock
must be held by caller.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/devfreq/devfreq.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Changes since v1:
* Split from series: low-priority bugfix not strictly required for PM QoS
* Only keep lock during update, release before sprintf
Link to v1: https://patchwork.kernel.org/patch/11149493/

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 4c58fbf7d4e4..00fc23fea5b2 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -206,10 +206,11 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
 {
 	int lev, prev_lev, ret = 0;
 	unsigned long cur_time;
 
 	cur_time = jiffies;
+	lockdep_assert_held(&devfreq->lock);
 
 	/* Immediately exit if previous_freq is not initialized yet. */
 	if (!devfreq->previous_freq)
 		goto out;
 
@@ -1507,16 +1508,22 @@ static ssize_t trans_stat_show(struct device *dev,
 	struct devfreq *devfreq = to_devfreq(dev);
 	ssize_t len;
 	int i, j;
 	unsigned int max_state = devfreq->profile->max_state;
 
-	if (!devfreq->stop_polling &&
-			devfreq_update_status(devfreq, devfreq->previous_freq))
-		return 0;
 	if (max_state == 0)
 		return sprintf(buf, "Not Supported.\n");
 
+	/* lock and update */
+	mutex_lock(&devfreq->lock);
+	if (!devfreq->stop_polling &&
+			devfreq_update_status(devfreq, devfreq->previous_freq)) {
+		mutex_unlock(&devfreq->lock);
+		return 0;
+	}
+	mutex_unlock(&devfreq->lock);
+
 	len = sprintf(buf, "     From  :   To\n");
 	len += sprintf(buf + len, "           :");
 	for (i = 0; i < max_state; i++)
 		len += sprintf(buf + len, "%10lu",
 				devfreq->profile->freq_table[i]);
-- 
2.17.1

[PATCH] PM / devfreq: Lock devfreq in trans_stat_show

[Leonard Crestez]

There is no locking in this sysfs show function so stats printing can
race with a devfreq_update_status called as part of freq switching or
with initialization.

Also add an assert in devfreq_update_status to make it clear that lock
must be held by caller.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/devfreq/devfreq.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Changes since v1:
* Split from series: low-priority bugfix not strictly required for PM QoS
* Only keep lock during update, release before sprintf
Link to v1: https://patchwork.kernel.org/patch/11149493/

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 4c58fbf7d4e4..00fc23fea5b2 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -206,10 +206,11 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
 {
 	int lev, prev_lev, ret = 0;
 	unsigned long cur_time;
 
 	cur_time = jiffies;
+	lockdep_assert_held(&devfreq->lock);
 
 	/* Immediately exit if previous_freq is not initialized yet. */
 	if (!devfreq->previous_freq)
 		goto out;
 
@@ -1507,16 +1508,22 @@ static ssize_t trans_stat_show(struct device *dev,
 	struct devfreq *devfreq = to_devfreq(dev);
 	ssize_t len;
 	int i, j;
 	unsigned int max_state = devfreq->profile->max_state;
 
-	if (!devfreq->stop_polling &&
-			devfreq_update_status(devfreq, devfreq->previous_freq))
-		return 0;
 	if (max_state == 0)
 		return sprintf(buf, "Not Supported.\n");
 
+	/* lock and update */
+	mutex_lock(&devfreq->lock);
+	if (!devfreq->stop_polling &&
+			devfreq_update_status(devfreq, devfreq->previous_freq)) {
+		mutex_unlock(&devfreq->lock);
+		return 0;
+	}
+	mutex_unlock(&devfreq->lock);
+
 	len = sprintf(buf, "     From  :   To\n");
 	len += sprintf(buf + len, "           :");
 	for (i = 0; i < max_state; i++)
 		len += sprintf(buf + len, "%10lu",
 				devfreq->profile->freq_table[i]);
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show

[Matthias Kaehlcke]

On Mon, Sep 23, 2019 at 07:27:27PM +0300, Leonard Crestez wrote:
> There is no locking in this sysfs show function so stats printing can
> race with a devfreq_update_status called as part of freq switching or
> with initialization.
> 
> Also add an assert in devfreq_update_status to make it clear that lock
> must be held by caller.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> Changes since v1:
> * Split from series: low-priority bugfix not strictly required for PM QoS
> * Only keep lock during update, release before sprintf
> Link to v1: https://patchwork.kernel.org/patch/11149493/
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 4c58fbf7d4e4..00fc23fea5b2 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -206,10 +206,11 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>  {
>  	int lev, prev_lev, ret = 0;
>  	unsigned long cur_time;
>  
>  	cur_time = jiffies;
> +	lockdep_assert_held(&devfreq->lock);
>  
>  	/* Immediately exit if previous_freq is not initialized yet. */
>  	if (!devfreq->previous_freq)
>  		goto out;
>  
> @@ -1507,16 +1508,22 @@ static ssize_t trans_stat_show(struct device *dev,
>  	struct devfreq *devfreq = to_devfreq(dev);
>  	ssize_t len;
>  	int i, j;
>  	unsigned int max_state = devfreq->profile->max_state;
>  
> -	if (!devfreq->stop_polling &&
> -			devfreq_update_status(devfreq, devfreq->previous_freq))
> -		return 0;
>  	if (max_state == 0)
>  		return sprintf(buf, "Not Supported.\n");
>  
> +	/* lock and update */
> +	mutex_lock(&devfreq->lock);
> +	if (!devfreq->stop_polling &&
> +			devfreq_update_status(devfreq, devfreq->previous_freq)) {
> +		mutex_unlock(&devfreq->lock);
> +		return 0;
> +	}
> +	mutex_unlock(&devfreq->lock);
> +
>  	len = sprintf(buf, "     From  :   To\n");
>  	len += sprintf(buf + len, "           :");
>  	for (i = 0; i < max_state; i++)
>  		len += sprintf(buf + len, "%10lu",
>  				devfreq->profile->freq_table[i]);

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show

[Matthias Kaehlcke]

On Mon, Sep 23, 2019 at 07:27:27PM +0300, Leonard Crestez wrote:
> There is no locking in this sysfs show function so stats printing can
> race with a devfreq_update_status called as part of freq switching or
> with initialization.
> 
> Also add an assert in devfreq_update_status to make it clear that lock
> must be held by caller.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> Changes since v1:
> * Split from series: low-priority bugfix not strictly required for PM QoS
> * Only keep lock during update, release before sprintf
> Link to v1: https://patchwork.kernel.org/patch/11149493/
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 4c58fbf7d4e4..00fc23fea5b2 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -206,10 +206,11 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>  {
>  	int lev, prev_lev, ret = 0;
>  	unsigned long cur_time;
>  
>  	cur_time = jiffies;
> +	lockdep_assert_held(&devfreq->lock);
>  
>  	/* Immediately exit if previous_freq is not initialized yet. */
>  	if (!devfreq->previous_freq)
>  		goto out;
>  
> @@ -1507,16 +1508,22 @@ static ssize_t trans_stat_show(struct device *dev,
>  	struct devfreq *devfreq = to_devfreq(dev);
>  	ssize_t len;
>  	int i, j;
>  	unsigned int max_state = devfreq->profile->max_state;
>  
> -	if (!devfreq->stop_polling &&
> -			devfreq_update_status(devfreq, devfreq->previous_freq))
> -		return 0;
>  	if (max_state == 0)
>  		return sprintf(buf, "Not Supported.\n");
>  
> +	/* lock and update */
> +	mutex_lock(&devfreq->lock);
> +	if (!devfreq->stop_polling &&
> +			devfreq_update_status(devfreq, devfreq->previous_freq)) {
> +		mutex_unlock(&devfreq->lock);
> +		return 0;
> +	}
> +	mutex_unlock(&devfreq->lock);
> +
>  	len = sprintf(buf, "     From  :   To\n");
>  	len += sprintf(buf + len, "           :");
>  	for (i = 0; i < max_state; i++)
>  		len += sprintf(buf + len, "%10lu",
>  				devfreq->profile->freq_table[i]);

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show

[Matthias Kaehlcke]

On Mon, Sep 23, 2019 at 07:27:27PM +0300, Leonard Crestez wrote:
> There is no locking in this sysfs show function so stats printing can
> race with a devfreq_update_status called as part of freq switching or
> with initialization.
> 
> Also add an assert in devfreq_update_status to make it clear that lock
> must be held by caller.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> Changes since v1:
> * Split from series: low-priority bugfix not strictly required for PM QoS
> * Only keep lock during update, release before sprintf
> Link to v1: https://patchwork.kernel.org/patch/11149493/
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 4c58fbf7d4e4..00fc23fea5b2 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -206,10 +206,11 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>  {
>  	int lev, prev_lev, ret = 0;
>  	unsigned long cur_time;
>  
>  	cur_time = jiffies;
> +	lockdep_assert_held(&devfreq->lock);
>  
>  	/* Immediately exit if previous_freq is not initialized yet. */
>  	if (!devfreq->previous_freq)
>  		goto out;
>  
> @@ -1507,16 +1508,22 @@ static ssize_t trans_stat_show(struct device *dev,
>  	struct devfreq *devfreq = to_devfreq(dev);
>  	ssize_t len;
>  	int i, j;
>  	unsigned int max_state = devfreq->profile->max_state;
>  
> -	if (!devfreq->stop_polling &&
> -			devfreq_update_status(devfreq, devfreq->previous_freq))
> -		return 0;
>  	if (max_state == 0)
>  		return sprintf(buf, "Not Supported.\n");
>  
> +	/* lock and update */

nit: the comment doesn't add much value, I'd suggest to remove it.

Forgot to comment on this earlier. My Reviewed-by tag still stands, up
to you if you want to re-spin.

Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show

[Matthias Kaehlcke]

On Mon, Sep 23, 2019 at 07:27:27PM +0300, Leonard Crestez wrote:
> There is no locking in this sysfs show function so stats printing can
> race with a devfreq_update_status called as part of freq switching or
> with initialization.
> 
> Also add an assert in devfreq_update_status to make it clear that lock
> must be held by caller.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> Changes since v1:
> * Split from series: low-priority bugfix not strictly required for PM QoS
> * Only keep lock during update, release before sprintf
> Link to v1: https://patchwork.kernel.org/patch/11149493/
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 4c58fbf7d4e4..00fc23fea5b2 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -206,10 +206,11 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>  {
>  	int lev, prev_lev, ret = 0;
>  	unsigned long cur_time;
>  
>  	cur_time = jiffies;
> +	lockdep_assert_held(&devfreq->lock);
>  
>  	/* Immediately exit if previous_freq is not initialized yet. */
>  	if (!devfreq->previous_freq)
>  		goto out;
>  
> @@ -1507,16 +1508,22 @@ static ssize_t trans_stat_show(struct device *dev,
>  	struct devfreq *devfreq = to_devfreq(dev);
>  	ssize_t len;
>  	int i, j;
>  	unsigned int max_state = devfreq->profile->max_state;
>  
> -	if (!devfreq->stop_polling &&
> -			devfreq_update_status(devfreq, devfreq->previous_freq))
> -		return 0;
>  	if (max_state == 0)
>  		return sprintf(buf, "Not Supported.\n");
>  
> +	/* lock and update */

nit: the comment doesn't add much value, I'd suggest to remove it.

Forgot to comment on this earlier. My Reviewed-by tag still stands, up
to you if you want to re-spin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show

[Chanwoo Choi]

Hi,

On 19. 9. 24. 오전 1:27, Leonard Crestez wrote:
> There is no locking in this sysfs show function so stats printing can
> race with a devfreq_update_status called as part of freq switching or
> with initialization.
> 
> Also add an assert in devfreq_update_status to make it clear that lock
> must be held by caller.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> Changes since v1:
> * Split from series: low-priority bugfix not strictly required for PM QoS
> * Only keep lock during update, release before sprintf
> Link to v1: https://patchwork.kernel.org/patch/11149493/
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 4c58fbf7d4e4..00fc23fea5b2 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -206,10 +206,11 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>  {
>  	int lev, prev_lev, ret = 0;
>  	unsigned long cur_time;
>  
>  	cur_time = jiffies;
> +	lockdep_assert_held(&devfreq->lock);

It better to move lock checking before 'cur_time = jiffies'
in order to reduce the redundant code execution.

>  
>  	/* Immediately exit if previous_freq is not initialized yet. */
>  	if (!devfreq->previous_freq)
>  		goto out;
>  
> @@ -1507,16 +1508,22 @@ static ssize_t trans_stat_show(struct device *dev,
>  	struct devfreq *devfreq = to_devfreq(dev);
>  	ssize_t len;
>  	int i, j;
>  	unsigned int max_state = devfreq->profile->max_state;
>  
> -	if (!devfreq->stop_polling &&
> -			devfreq_update_status(devfreq, devfreq->previous_freq))
> -		return 0;
>  	if (max_state == 0)
>  		return sprintf(buf, "Not Supported.\n");
>  
> +	/* lock and update */

It is not necessary. Anyone can know that this code is related to mutex lock/unlock.

> +	mutex_lock(&devfreq->lock);
> +	if (!devfreq->stop_polling &&
> +			devfreq_update_status(devfreq, devfreq->previous_freq)) {
> +		mutex_unlock(&devfreq->lock);
> +		return 0;
> +	}
> +	mutex_unlock(&devfreq->lock);
> +
>  	len = sprintf(buf, "     From  :   To\n");
>  	len += sprintf(buf + len, "           :");
>  	for (i = 0; i < max_state; i++)
>  		len += sprintf(buf + len, "%10lu",
>  				devfreq->profile->freq_table[i]);
> 

Basically, it is necessary. Please edit it by my comments.
Also, you have to add the following 'fixes' as following:
and send it stable mailing list.

Fixes: 39688ce6facd ("PM / devfreq: account suspend/resume for stats")

If you edit it by my comments, feel free to add my tag:
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show

[Chanwoo Choi]

Hi,

On 19. 9. 24. 오전 1:27, Leonard Crestez wrote:
> There is no locking in this sysfs show function so stats printing can
> race with a devfreq_update_status called as part of freq switching or
> with initialization.
> 
> Also add an assert in devfreq_update_status to make it clear that lock
> must be held by caller.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> Changes since v1:
> * Split from series: low-priority bugfix not strictly required for PM QoS
> * Only keep lock during update, release before sprintf
> Link to v1: https://patchwork.kernel.org/patch/11149493/
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 4c58fbf7d4e4..00fc23fea5b2 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -206,10 +206,11 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>  {
>  	int lev, prev_lev, ret = 0;
>  	unsigned long cur_time;
>  
>  	cur_time = jiffies;
> +	lockdep_assert_held(&devfreq->lock);

It better to move lock checking before 'cur_time = jiffies'
in order to reduce the redundant code execution.

>  
>  	/* Immediately exit if previous_freq is not initialized yet. */
>  	if (!devfreq->previous_freq)
>  		goto out;
>  
> @@ -1507,16 +1508,22 @@ static ssize_t trans_stat_show(struct device *dev,
>  	struct devfreq *devfreq = to_devfreq(dev);
>  	ssize_t len;
>  	int i, j;
>  	unsigned int max_state = devfreq->profile->max_state;
>  
> -	if (!devfreq->stop_polling &&
> -			devfreq_update_status(devfreq, devfreq->previous_freq))
> -		return 0;
>  	if (max_state == 0)
>  		return sprintf(buf, "Not Supported.\n");
>  
> +	/* lock and update */

It is not necessary. Anyone can know that this code is related to mutex lock/unlock.

> +	mutex_lock(&devfreq->lock);
> +	if (!devfreq->stop_polling &&
> +			devfreq_update_status(devfreq, devfreq->previous_freq)) {
> +		mutex_unlock(&devfreq->lock);
> +		return 0;
> +	}
> +	mutex_unlock(&devfreq->lock);
> +
>  	len = sprintf(buf, "     From  :   To\n");
>  	len += sprintf(buf + len, "           :");
>  	for (i = 0; i < max_state; i++)
>  		len += sprintf(buf + len, "%10lu",
>  				devfreq->profile->freq_table[i]);
> 

Basically, it is necessary. Please edit it by my comments.
Also, you have to add the following 'fixes' as following:
and send it stable mailing list.

Fixes: 39688ce6facd ("PM / devfreq: account suspend/resume for stats")

If you edit it by my comments, feel free to add my tag:
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show

[Leonard Crestez]

On 2019-09-24 5:07 AM, Chanwoo Choi wrote:
> Hi,
> 
> On 19. 9. 24. 오전 1:27, Leonard Crestez wrote:
>> There is no locking in this sysfs show function so stats printing can
>> race with a devfreq_update_status called as part of freq switching or
>> with initialization.
>>
>> Also add an assert in devfreq_update_status to make it clear that lock
>> must be held by caller.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>>   drivers/devfreq/devfreq.c | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> Changes since v1:
>> * Split from series: low-priority bugfix not strictly required for PM QoS
>> * Only keep lock during update, release before sprintf
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 4c58fbf7d4e4..00fc23fea5b2 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -206,10 +206,11 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>   {
>>   	int lev, prev_lev, ret = 0;
>>   	unsigned long cur_time;
>>   
>>   	cur_time = jiffies;
>> +	lockdep_assert_held(&devfreq->lock);
> 
> It better to move lock checking before 'cur_time = jiffies'
> in order to reduce the redundant code execution.

OK but I don't see how this makes a difference for an assert? It just 
prints a warning and carries on.

>>   	/* Immediately exit if previous_freq is not initialized yet. */
>>   	if (!devfreq->previous_freq)
>>   		goto out;
>>   
>> @@ -1507,16 +1508,22 @@ static ssize_t trans_stat_show(struct device *dev,
>>   	struct devfreq *devfreq = to_devfreq(dev);
>>   	ssize_t len;
>>   	int i, j;
>>   	unsigned int max_state = devfreq->profile->max_state;
>>   
>> -	if (!devfreq->stop_polling &&
>> -			devfreq_update_status(devfreq, devfreq->previous_freq))
>> -		return 0;
>>   	if (max_state == 0)
>>   		return sprintf(buf, "Not Supported.\n");
>>   
>> +	/* lock and update */
> 
> It is not necessary. Anyone can know that this code is related to mutex lock/unlock.

OK. You're the second person to mention this but it's quite strange to 
see objections raised against comments.

>> +	mutex_lock(&devfreq->lock);
>> +	if (!devfreq->stop_polling &&
>> +			devfreq_update_status(devfreq, devfreq->previous_freq)) {
>> +		mutex_unlock(&devfreq->lock);
>> +		return 0;
>> +	}
>> +	mutex_unlock(&devfreq->lock);
>> +
>>   	len = sprintf(buf, "     From  :   To\n");
>>   	len += sprintf(buf + len, "           :");
>>   	for (i = 0; i < max_state; i++)
>>   		len += sprintf(buf + len, "%10lu",
>>   				devfreq->profile->freq_table[i]);
>>
> 
> Basically, it is necessary. Please edit it by my comments.
> Also, you have to add the following 'fixes' as following:
> and send it stable mailing list.
> 
> Fixes: 39688ce6facd ("PM / devfreq: account suspend/resume for stats")
> 
> If you edit it by my comments, feel free to add my tag:
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show

[Leonard Crestez]

On 2019-09-24 5:07 AM, Chanwoo Choi wrote:
> Hi,
> 
> On 19. 9. 24. 오전 1:27, Leonard Crestez wrote:
>> There is no locking in this sysfs show function so stats printing can
>> race with a devfreq_update_status called as part of freq switching or
>> with initialization.
>>
>> Also add an assert in devfreq_update_status to make it clear that lock
>> must be held by caller.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>>   drivers/devfreq/devfreq.c | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> Changes since v1:
>> * Split from series: low-priority bugfix not strictly required for PM QoS
>> * Only keep lock during update, release before sprintf
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 4c58fbf7d4e4..00fc23fea5b2 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -206,10 +206,11 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>   {
>>   	int lev, prev_lev, ret = 0;
>>   	unsigned long cur_time;
>>   
>>   	cur_time = jiffies;
>> +	lockdep_assert_held(&devfreq->lock);
> 
> It better to move lock checking before 'cur_time = jiffies'
> in order to reduce the redundant code execution.

OK but I don't see how this makes a difference for an assert? It just 
prints a warning and carries on.

>>   	/* Immediately exit if previous_freq is not initialized yet. */
>>   	if (!devfreq->previous_freq)
>>   		goto out;
>>   
>> @@ -1507,16 +1508,22 @@ static ssize_t trans_stat_show(struct device *dev,
>>   	struct devfreq *devfreq = to_devfreq(dev);
>>   	ssize_t len;
>>   	int i, j;
>>   	unsigned int max_state = devfreq->profile->max_state;
>>   
>> -	if (!devfreq->stop_polling &&
>> -			devfreq_update_status(devfreq, devfreq->previous_freq))
>> -		return 0;
>>   	if (max_state == 0)
>>   		return sprintf(buf, "Not Supported.\n");
>>   
>> +	/* lock and update */
> 
> It is not necessary. Anyone can know that this code is related to mutex lock/unlock.

OK. You're the second person to mention this but it's quite strange to 
see objections raised against comments.

>> +	mutex_lock(&devfreq->lock);
>> +	if (!devfreq->stop_polling &&
>> +			devfreq_update_status(devfreq, devfreq->previous_freq)) {
>> +		mutex_unlock(&devfreq->lock);
>> +		return 0;
>> +	}
>> +	mutex_unlock(&devfreq->lock);
>> +
>>   	len = sprintf(buf, "     From  :   To\n");
>>   	len += sprintf(buf + len, "           :");
>>   	for (i = 0; i < max_state; i++)
>>   		len += sprintf(buf + len, "%10lu",
>>   				devfreq->profile->freq_table[i]);
>>
> 
> Basically, it is necessary. Please edit it by my comments.
> Also, you have to add the following 'fixes' as following:
> and send it stable mailing list.
> 
> Fixes: 39688ce6facd ("PM / devfreq: account suspend/resume for stats")
> 
> If you edit it by my comments, feel free to add my tag:
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show

[Matthias Kaehlcke]

On Tue, Sep 24, 2019 at 07:44:16AM +0000, Leonard Crestez wrote:
> On 2019-09-24 5:07 AM, Chanwoo Choi wrote:

> >> @@ -1507,16 +1508,22 @@ static ssize_t trans_stat_show(struct device *dev,
> >>   	struct devfreq *devfreq = to_devfreq(dev);
> >>   	ssize_t len;
> >>   	int i, j;
> >>   	unsigned int max_state = devfreq->profile->max_state;
> >>   
> >> -	if (!devfreq->stop_polling &&
> >> -			devfreq_update_status(devfreq, devfreq->previous_freq))
> >> -		return 0;
> >>   	if (max_state == 0)
> >>   		return sprintf(buf, "Not Supported.\n");
> >>   
> >> +	/* lock and update */
> > 
> > It is not necessary. Anyone can know that this code is related to mutex lock/unlock.
> 
> OK. You're the second person to mention this but it's quite strange to 
> see objections raised against comments.

Comments are great if they add value, in this case the comment is
stating the obvious, which IMO just adds noise to the code.

The coding style guidelines also briefly touch this topic:

  8) Commenting
  -------------

  Comments are good, but there is also a danger of over-commenting.

Documentation/process/coding-style.rst

Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show

[Matthias Kaehlcke]

On Tue, Sep 24, 2019 at 07:44:16AM +0000, Leonard Crestez wrote:
> On 2019-09-24 5:07 AM, Chanwoo Choi wrote:

> >> @@ -1507,16 +1508,22 @@ static ssize_t trans_stat_show(struct device *dev,
> >>   	struct devfreq *devfreq = to_devfreq(dev);
> >>   	ssize_t len;
> >>   	int i, j;
> >>   	unsigned int max_state = devfreq->profile->max_state;
> >>   
> >> -	if (!devfreq->stop_polling &&
> >> -			devfreq_update_status(devfreq, devfreq->previous_freq))
> >> -		return 0;
> >>   	if (max_state == 0)
> >>   		return sprintf(buf, "Not Supported.\n");
> >>   
> >> +	/* lock and update */
> > 
> > It is not necessary. Anyone can know that this code is related to mutex lock/unlock.
> 
> OK. You're the second person to mention this but it's quite strange to 
> see objections raised against comments.

Comments are great if they add value, in this case the comment is
stating the obvious, which IMO just adds noise to the code.

The coding style guidelines also briefly touch this topic:

  8) Commenting
  -------------

  Comments are good, but there is also a danger of over-commenting.

Documentation/process/coding-style.rst

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show

[Chanwoo Choi]

On 19. 9. 24. 오후 4:44, Leonard Crestez wrote:
> On 2019-09-24 5:07 AM, Chanwoo Choi wrote:
>> Hi,
>>
>> On 19. 9. 24. 오전 1:27, Leonard Crestez wrote:
>>> There is no locking in this sysfs show function so stats printing can
>>> race with a devfreq_update_status called as part of freq switching or
>>> with initialization.
>>>
>>> Also add an assert in devfreq_update_status to make it clear that lock
>>> must be held by caller.
>>>
>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>> ---
>>>   drivers/devfreq/devfreq.c | 13 ++++++++++---
>>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> Changes since v1:
>>> * Split from series: low-priority bugfix not strictly required for PM QoS
>>> * Only keep lock during update, release before sprintf
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index 4c58fbf7d4e4..00fc23fea5b2 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -206,10 +206,11 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>>   {
>>>   	int lev, prev_lev, ret = 0;
>>>   	unsigned long cur_time;
>>>   
>>>   	cur_time = jiffies;
>>> +	lockdep_assert_held(&devfreq->lock);
>>
>> It better to move lock checking before 'cur_time = jiffies'
>> in order to reduce the redundant code execution.
> 
> OK but I don't see how this makes a difference for an assert? It just 
> prints a warning and carries on.

According to the sequence between 'lockdep_assert_held' and 'cur_time = jiffies',
cur_time will be initialized with different jiffies because 'jiffies' is continuously
changed. In order to get the more correct time from 'jiffies',
we better to get 'jiffies' after releasing the lock.

> 
>>>   	/* Immediately exit if previous_freq is not initialized yet. */
>>>   	if (!devfreq->previous_freq)
>>>   		goto out;
>>>   
>>> @@ -1507,16 +1508,22 @@ static ssize_t trans_stat_show(struct device *dev,
>>>   	struct devfreq *devfreq = to_devfreq(dev);
>>>   	ssize_t len;
>>>   	int i, j;
>>>   	unsigned int max_state = devfreq->profile->max_state;
>>>   
>>> -	if (!devfreq->stop_polling &&
>>> -			devfreq_update_status(devfreq, devfreq->previous_freq))
>>> -		return 0;
>>>   	if (max_state == 0)
>>>   		return sprintf(buf, "Not Supported.\n");
>>>   
>>> +	/* lock and update */
>>
>> It is not necessary. Anyone can know that this code is related to mutex lock/unlock.
> 
> OK. You're the second person to mention this but it's quite strange to 
> see objections raised against comments.

The comment is very important to understand the code
for everyone. But, in this case, almost people understand
the usage of mutex_lock/mutex_unlock. It is no difficult
to understand the meaning of below codes.

Usually, we would add the comments if some codes are very difficult
without comments or some codes have depend on some call sequence and so on.

> 
>>> +	mutex_lock(&devfreq->lock);
>>> +	if (!devfreq->stop_polling &&
>>> +			devfreq_update_status(devfreq, devfreq->previous_freq)) {
>>> +		mutex_unlock(&devfreq->lock);
>>> +		return 0;
>>> +	}
>>> +	mutex_unlock(&devfreq->lock);
>>> +
>>>   	len = sprintf(buf, "     From  :   To\n");
>>>   	len += sprintf(buf + len, "           :");
>>>   	for (i = 0; i < max_state; i++)
>>>   		len += sprintf(buf + len, "%10lu",
>>>   				devfreq->profile->freq_table[i]);
>>>
>>
>> Basically, it is necessary. Please edit it by my comments.
>> Also, you have to add the following 'fixes' as following:
>> and send it stable mailing list.
>>
>> Fixes: 39688ce6facd ("PM / devfreq: account suspend/resume for stats")
>>
>> If you edit it by my comments, feel free to add my tag:
>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show

[Chanwoo Choi]

On 19. 9. 24. 오후 4:44, Leonard Crestez wrote:
> On 2019-09-24 5:07 AM, Chanwoo Choi wrote:
>> Hi,
>>
>> On 19. 9. 24. 오전 1:27, Leonard Crestez wrote:
>>> There is no locking in this sysfs show function so stats printing can
>>> race with a devfreq_update_status called as part of freq switching or
>>> with initialization.
>>>
>>> Also add an assert in devfreq_update_status to make it clear that lock
>>> must be held by caller.
>>>
>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>> ---
>>>   drivers/devfreq/devfreq.c | 13 ++++++++++---
>>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> Changes since v1:
>>> * Split from series: low-priority bugfix not strictly required for PM QoS
>>> * Only keep lock during update, release before sprintf
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index 4c58fbf7d4e4..00fc23fea5b2 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -206,10 +206,11 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>>   {
>>>   	int lev, prev_lev, ret = 0;
>>>   	unsigned long cur_time;
>>>   
>>>   	cur_time = jiffies;
>>> +	lockdep_assert_held(&devfreq->lock);
>>
>> It better to move lock checking before 'cur_time = jiffies'
>> in order to reduce the redundant code execution.
> 
> OK but I don't see how this makes a difference for an assert? It just 
> prints a warning and carries on.

According to the sequence between 'lockdep_assert_held' and 'cur_time = jiffies',
cur_time will be initialized with different jiffies because 'jiffies' is continuously
changed. In order to get the more correct time from 'jiffies',
we better to get 'jiffies' after releasing the lock.

> 
>>>   	/* Immediately exit if previous_freq is not initialized yet. */
>>>   	if (!devfreq->previous_freq)
>>>   		goto out;
>>>   
>>> @@ -1507,16 +1508,22 @@ static ssize_t trans_stat_show(struct device *dev,
>>>   	struct devfreq *devfreq = to_devfreq(dev);
>>>   	ssize_t len;
>>>   	int i, j;
>>>   	unsigned int max_state = devfreq->profile->max_state;
>>>   
>>> -	if (!devfreq->stop_polling &&
>>> -			devfreq_update_status(devfreq, devfreq->previous_freq))
>>> -		return 0;
>>>   	if (max_state == 0)
>>>   		return sprintf(buf, "Not Supported.\n");
>>>   
>>> +	/* lock and update */
>>
>> It is not necessary. Anyone can know that this code is related to mutex lock/unlock.
> 
> OK. You're the second person to mention this but it's quite strange to 
> see objections raised against comments.

The comment is very important to understand the code
for everyone. But, in this case, almost people understand
the usage of mutex_lock/mutex_unlock. It is no difficult
to understand the meaning of below codes.

Usually, we would add the comments if some codes are very difficult
without comments or some codes have depend on some call sequence and so on.

> 
>>> +	mutex_lock(&devfreq->lock);
>>> +	if (!devfreq->stop_polling &&
>>> +			devfreq_update_status(devfreq, devfreq->previous_freq)) {
>>> +		mutex_unlock(&devfreq->lock);
>>> +		return 0;
>>> +	}
>>> +	mutex_unlock(&devfreq->lock);
>>> +
>>>   	len = sprintf(buf, "     From  :   To\n");
>>>   	len += sprintf(buf + len, "           :");
>>>   	for (i = 0; i < max_state; i++)
>>>   		len += sprintf(buf + len, "%10lu",
>>>   				devfreq->profile->freq_table[i]);
>>>
>>
>> Basically, it is necessary. Please edit it by my comments.
>> Also, you have to add the following 'fixes' as following:
>> and send it stable mailing list.
>>
>> Fixes: 39688ce6facd ("PM / devfreq: account suspend/resume for stats")
>>
>> If you edit it by my comments, feel free to add my tag:
>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show

[Leonard Crestez]

On 25.09.2019 04:17, Chanwoo Choi wrote:
> On 19. 9. 24. 오후 4:44, Leonard Crestez wrote:
>> On 2019-09-24 5:07 AM, Chanwoo Choi wrote:
>>> Hi,
>>>
>>> On 19. 9. 24. 오전 1:27, Leonard Crestez wrote:
>>>> There is no locking in this sysfs show function so stats printing can
>>>> race with a devfreq_update_status called as part of freq switching or
>>>> with initialization.
>>>>
>>>> Also add an assert in devfreq_update_status to make it clear that lock
>>>> must be held by caller.
>>>>
>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>>> ---
>>>>    drivers/devfreq/devfreq.c | 13 ++++++++++---
>>>>    1 file changed, 10 insertions(+), 3 deletions(-)
>>>>
>>>> Changes since v1:
>>>> * Split from series: low-priority bugfix not strictly required for PM QoS
>>>> * Only keep lock during update, release before sprintf
>>>>
>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>> index 4c58fbf7d4e4..00fc23fea5b2 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -206,10 +206,11 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>>>    {
>>>>    	int lev, prev_lev, ret = 0;
>>>>    	unsigned long cur_time;
>>>>    
>>>>    	cur_time = jiffies;
>>>> +	lockdep_assert_held(&devfreq->lock);
>>>
>>> It better to move lock checking before 'cur_time = jiffies'
>>> in order to reduce the redundant code execution.
>>
>> OK but I don't see how this makes a difference for an assert? It just
>> prints a warning and carries on.
> 
> According to the sequence between 'lockdep_assert_held' and 'cur_time = jiffies',
> cur_time will be initialized with different jiffies because 'jiffies' is continuously
> changed. In order to get the more correct time from 'jiffies',
> we better to get 'jiffies' after releasing the lock.

That makes sense.

>>>>    	/* Immediately exit if previous_freq is not initialized yet. */
>>>>    	if (!devfreq->previous_freq)
>>>>    		goto out;
>>>>    
>>>> @@ -1507,16 +1508,22 @@ static ssize_t trans_stat_show(struct device *dev,
>>>>    	struct devfreq *devfreq = to_devfreq(dev);
>>>>    	ssize_t len;
>>>>    	int i, j;
>>>>    	unsigned int max_state = devfreq->profile->max_state;
>>>>    
>>>> -	if (!devfreq->stop_polling &&
>>>> -			devfreq_update_status(devfreq, devfreq->previous_freq))
>>>> -		return 0;
>>>>    	if (max_state == 0)
>>>>    		return sprintf(buf, "Not Supported.\n");
>>>>    
>>>> +	/* lock and update */
>>>
>>> It is not necessary. Anyone can know that this code is related to mutex lock/unlock.
>>
>> OK. You're the second person to mention this but it's quite strange to
>> see objections raised against comments.
> 
> The comment is very important to understand the code
> for everyone. But, in this case, almost people understand
> the usage of mutex_lock/mutex_unlock. It is no difficult
> to understand the meaning of below codes.
> 
> Usually, we would add the comments if some codes are very difficult
> without comments or some codes have depend on some call sequence and so on.

OK. Sometimes I add brief comments ahead of a paragraph of code so that 
I can read it faster.

>>>> +	mutex_lock(&devfreq->lock);
>>>> +	if (!devfreq->stop_polling &&
>>>> +			devfreq_update_status(devfreq, devfreq->previous_freq)) {
>>>> +		mutex_unlock(&devfreq->lock);
>>>> +		return 0;
>>>> +	}
>>>> +	mutex_unlock(&devfreq->lock);
>>>> +
>>>>    	len = sprintf(buf, "     From  :   To\n");
>>>>    	len += sprintf(buf + len, "           :");
>>>>    	for (i = 0; i < max_state; i++)
>>>>    		len += sprintf(buf + len, "%10lu",
>>>>    				devfreq->profile->freq_table[i]);
>>>>
>>>
>>> Basically, it is necessary. Please edit it by my comments.
>>> Also, you have to add the following 'fixes' as following:
>>> and send it stable mailing list.
>>>
>>> Fixes: 39688ce6facd ("PM / devfreq: account suspend/resume for stats")
>>>
>>> If you edit it by my comments, feel free to add my tag:
>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

Already posted v2 with all the requested changes:

https://patchwork.kernel.org/patch/11158225/

Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show

[Leonard Crestez]

On 25.09.2019 04:17, Chanwoo Choi wrote:
> On 19. 9. 24. 오후 4:44, Leonard Crestez wrote:
>> On 2019-09-24 5:07 AM, Chanwoo Choi wrote:
>>> Hi,
>>>
>>> On 19. 9. 24. 오전 1:27, Leonard Crestez wrote:
>>>> There is no locking in this sysfs show function so stats printing can
>>>> race with a devfreq_update_status called as part of freq switching or
>>>> with initialization.
>>>>
>>>> Also add an assert in devfreq_update_status to make it clear that lock
>>>> must be held by caller.
>>>>
>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>>> ---
>>>>    drivers/devfreq/devfreq.c | 13 ++++++++++---
>>>>    1 file changed, 10 insertions(+), 3 deletions(-)
>>>>
>>>> Changes since v1:
>>>> * Split from series: low-priority bugfix not strictly required for PM QoS
>>>> * Only keep lock during update, release before sprintf
>>>>
>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>> index 4c58fbf7d4e4..00fc23fea5b2 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -206,10 +206,11 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>>>    {
>>>>    	int lev, prev_lev, ret = 0;
>>>>    	unsigned long cur_time;
>>>>    
>>>>    	cur_time = jiffies;
>>>> +	lockdep_assert_held(&devfreq->lock);
>>>
>>> It better to move lock checking before 'cur_time = jiffies'
>>> in order to reduce the redundant code execution.
>>
>> OK but I don't see how this makes a difference for an assert? It just
>> prints a warning and carries on.
> 
> According to the sequence between 'lockdep_assert_held' and 'cur_time = jiffies',
> cur_time will be initialized with different jiffies because 'jiffies' is continuously
> changed. In order to get the more correct time from 'jiffies',
> we better to get 'jiffies' after releasing the lock.

That makes sense.

>>>>    	/* Immediately exit if previous_freq is not initialized yet. */
>>>>    	if (!devfreq->previous_freq)
>>>>    		goto out;
>>>>    
>>>> @@ -1507,16 +1508,22 @@ static ssize_t trans_stat_show(struct device *dev,
>>>>    	struct devfreq *devfreq = to_devfreq(dev);
>>>>    	ssize_t len;
>>>>    	int i, j;
>>>>    	unsigned int max_state = devfreq->profile->max_state;
>>>>    
>>>> -	if (!devfreq->stop_polling &&
>>>> -			devfreq_update_status(devfreq, devfreq->previous_freq))
>>>> -		return 0;
>>>>    	if (max_state == 0)
>>>>    		return sprintf(buf, "Not Supported.\n");
>>>>    
>>>> +	/* lock and update */
>>>
>>> It is not necessary. Anyone can know that this code is related to mutex lock/unlock.
>>
>> OK. You're the second person to mention this but it's quite strange to
>> see objections raised against comments.
> 
> The comment is very important to understand the code
> for everyone. But, in this case, almost people understand
> the usage of mutex_lock/mutex_unlock. It is no difficult
> to understand the meaning of below codes.
> 
> Usually, we would add the comments if some codes are very difficult
> without comments or some codes have depend on some call sequence and so on.

OK. Sometimes I add brief comments ahead of a paragraph of code so that 
I can read it faster.

>>>> +	mutex_lock(&devfreq->lock);
>>>> +	if (!devfreq->stop_polling &&
>>>> +			devfreq_update_status(devfreq, devfreq->previous_freq)) {
>>>> +		mutex_unlock(&devfreq->lock);
>>>> +		return 0;
>>>> +	}
>>>> +	mutex_unlock(&devfreq->lock);
>>>> +
>>>>    	len = sprintf(buf, "     From  :   To\n");
>>>>    	len += sprintf(buf + len, "           :");
>>>>    	for (i = 0; i < max_state; i++)
>>>>    		len += sprintf(buf + len, "%10lu",
>>>>    				devfreq->profile->freq_table[i]);
>>>>
>>>
>>> Basically, it is necessary. Please edit it by my comments.
>>> Also, you have to add the following 'fixes' as following:
>>> and send it stable mailing list.
>>>
>>> Fixes: 39688ce6facd ("PM / devfreq: account suspend/resume for stats")
>>>
>>> If you edit it by my comments, feel free to add my tag:
>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

Already posted v2 with all the requested changes:

https://patchwork.kernel.org/patch/11158225/
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel