* [PATCH] PM / devfreq: Lock devfreq in trans_stat_show
@ 2019-09-23 16:27 ` Leonard Crestez
0 siblings, 0 replies; 16+ messages in thread
From: Leonard Crestez @ 2019-09-23 16:27 UTC (permalink / raw)
To: Matthias Kaehlcke, MyungJoo Ham, Kyungmin Park
Cc: Artur Świgoń,
linux-pm, Krzysztof Kozlowski, Lukasz Luba, Chanwoo Choi,
NXP Linux Team, Georgi Djakov, linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show
2019-09-23 16:27 ` Leonard Crestez
@ 2019-09-23 18:54 ` Matthias Kaehlcke
-1 siblings, 0 replies; 16+ messages in thread
From: Matthias Kaehlcke @ 2019-09-23 18:54 UTC (permalink / raw)
To: Leonard Crestez
Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Artur Świgoń,
Krzysztof Kozlowski, Georgi Djakov, Lukasz Luba, NXP Linux Team,
linux-pm, linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show
@ 2019-09-23 18:54 ` Matthias Kaehlcke
0 siblings, 0 replies; 16+ messages in thread
From: Matthias Kaehlcke @ 2019-09-23 18:54 UTC (permalink / raw)
To: Leonard Crestez
Cc: Artur Świgoń,
linux-pm, Krzysztof Kozlowski, Lukasz Luba, Chanwoo Choi,
Kyungmin Park, MyungJoo Ham, NXP Linux Team, Georgi Djakov,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show
2019-09-23 16:27 ` Leonard Crestez
@ 2019-09-23 23:34 ` Matthias Kaehlcke
-1 siblings, 0 replies; 16+ messages in thread
From: Matthias Kaehlcke @ 2019-09-23 23:34 UTC (permalink / raw)
To: Leonard Crestez
Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Artur Świgoń,
Krzysztof Kozlowski, Georgi Djakov, Lukasz Luba, NXP Linux Team,
linux-pm, linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show
@ 2019-09-23 23:34 ` Matthias Kaehlcke
0 siblings, 0 replies; 16+ messages in thread
From: Matthias Kaehlcke @ 2019-09-23 23:34 UTC (permalink / raw)
To: Leonard Crestez
Cc: Artur Świgoń,
linux-pm, Krzysztof Kozlowski, Lukasz Luba, Chanwoo Choi,
Kyungmin Park, MyungJoo Ham, NXP Linux Team, Georgi Djakov,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show
2019-09-23 16:27 ` Leonard Crestez
@ 2019-09-24 2:11 ` Chanwoo Choi
-1 siblings, 0 replies; 16+ messages in thread
From: Chanwoo Choi @ 2019-09-24 2:11 UTC (permalink / raw)
To: Leonard Crestez, Matthias Kaehlcke, MyungJoo Ham, Kyungmin Park
Cc: Artur Świgoń,
Krzysztof Kozlowski, Georgi Djakov, Lukasz Luba, NXP Linux Team,
linux-pm, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show
@ 2019-09-24 2:11 ` Chanwoo Choi
0 siblings, 0 replies; 16+ messages in thread
From: Chanwoo Choi @ 2019-09-24 2:11 UTC (permalink / raw)
To: Leonard Crestez, Matthias Kaehlcke, MyungJoo Ham, Kyungmin Park
Cc: Artur Świgoń,
linux-pm, Krzysztof Kozlowski, Lukasz Luba, NXP Linux Team,
Georgi Djakov, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show
2019-09-24 2:11 ` Chanwoo Choi
@ 2019-09-24 7:44 ` Leonard Crestez
-1 siblings, 0 replies; 16+ messages in thread
From: Leonard Crestez @ 2019-09-24 7:44 UTC (permalink / raw)
To: Chanwoo Choi, Matthias Kaehlcke
Cc: MyungJoo Ham, Kyungmin Park, Artur Świgoń,
Krzysztof Kozlowski, Georgi Djakov, Lukasz Luba, dl-linux-imx,
linux-pm, linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show
@ 2019-09-24 7:44 ` Leonard Crestez
0 siblings, 0 replies; 16+ messages in thread
From: Leonard Crestez @ 2019-09-24 7:44 UTC (permalink / raw)
To: Chanwoo Choi, Matthias Kaehlcke
Cc: Artur Świgoń,
linux-pm, Krzysztof Kozlowski, Lukasz Luba, Kyungmin Park,
MyungJoo Ham, dl-linux-imx, Georgi Djakov, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show
2019-09-24 7:44 ` Leonard Crestez
@ 2019-09-24 15:44 ` Matthias Kaehlcke
-1 siblings, 0 replies; 16+ messages in thread
From: Matthias Kaehlcke @ 2019-09-24 15:44 UTC (permalink / raw)
To: Leonard Crestez
Cc: Chanwoo Choi, MyungJoo Ham, Kyungmin Park, Artur Świgoń,
Krzysztof Kozlowski, Georgi Djakov, Lukasz Luba, dl-linux-imx,
linux-pm, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show
@ 2019-09-24 15:44 ` Matthias Kaehlcke
0 siblings, 0 replies; 16+ messages in thread
From: Matthias Kaehlcke @ 2019-09-24 15:44 UTC (permalink / raw)
To: Leonard Crestez
Cc: Artur Świgoń,
linux-pm, Krzysztof Kozlowski, Lukasz Luba, Chanwoo Choi,
Kyungmin Park, MyungJoo Ham, dl-linux-imx, Georgi Djakov,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show
2019-09-24 7:44 ` Leonard Crestez
@ 2019-09-25 1:21 ` Chanwoo Choi
-1 siblings, 0 replies; 16+ messages in thread
From: Chanwoo Choi @ 2019-09-25 1:21 UTC (permalink / raw)
To: Leonard Crestez, Matthias Kaehlcke
Cc: MyungJoo Ham, Kyungmin Park, Artur Świgoń,
Krzysztof Kozlowski, Georgi Djakov, Lukasz Luba, dl-linux-imx,
linux-pm, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show
@ 2019-09-25 1:21 ` Chanwoo Choi
0 siblings, 0 replies; 16+ messages in thread
From: Chanwoo Choi @ 2019-09-25 1:21 UTC (permalink / raw)
To: Leonard Crestez, Matthias Kaehlcke
Cc: Artur Świgoń,
linux-pm, Krzysztof Kozlowski, Lukasz Luba, Kyungmin Park,
MyungJoo Ham, dl-linux-imx, Georgi Djakov, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show
2019-09-25 1:21 ` Chanwoo Choi
@ 2019-09-25 19:36 ` Leonard Crestez
-1 siblings, 0 replies; 16+ messages in thread
From: Leonard Crestez @ 2019-09-25 19:36 UTC (permalink / raw)
To: Chanwoo Choi, Matthias Kaehlcke, MyungJoo Ham
Cc: Kyungmin Park, Artur Świgoń,
Krzysztof Kozlowski, Georgi Djakov, Lukasz Luba, dl-linux-imx,
linux-pm, linux-arm-kernel
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/
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] PM / devfreq: Lock devfreq in trans_stat_show
@ 2019-09-25 19:36 ` Leonard Crestez
0 siblings, 0 replies; 16+ messages in thread
From: Leonard Crestez @ 2019-09-25 19:36 UTC (permalink / raw)
To: Chanwoo Choi, Matthias Kaehlcke, MyungJoo Ham
Cc: Artur Świgoń,
linux-pm, Krzysztof Kozlowski, Lukasz Luba, Kyungmin Park,
dl-linux-imx, Georgi Djakov, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread