| Message ID | 714675448e7fbf3c930b0dca6fbe54fa5f87211b.1569256001.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PM / devfreq: Lock devfreq in trans_stat_show | expand |
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>
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.
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>
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>
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
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>
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/
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]);
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/