| Message ID | 7d8f4d5c608d45ba19cdd52068fe6ffe30de67c1.1568764439.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | PM / devfreq: Add dev_pm_qos support | expand |
Hi Leonard, this series doesn't indicate the version, from the change history in the cover letter I suppose it is v5. On Wed, Sep 18, 2019 at 03:18:20AM +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. This and some other patches look like generic improvements and not directly related to the series "PM / devfreq: Add dev_pm_qos support". If there are no dependencies I think it is usually better to send the improvements separately, it keeps the series more focussed and might reduce version churn. Just my POV though ;-) > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > --- > drivers/devfreq/devfreq.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 2494ee16f502..665575228c4f 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -159,10 +159,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; > > @@ -1415,15 +1416,20 @@ 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; > > + mutex_lock(&devfreq->lock); > if (!devfreq->stop_polling && > - devfreq_update_status(devfreq, devfreq->previous_freq)) > - return 0; > - if (max_state == 0) > - return sprintf(buf, "Not Supported.\n"); > + devfreq_update_status(devfreq, devfreq->previous_freq)) { > + len = 0; you could assign 'len' in the declaration instead, but it's just another option, it'ss fine as is. > + goto out; > + } > + if (max_state == 0) { > + len = sprintf(buf, "Not Supported.\n"); > + goto out; > + } This leaves the general structure of the code as is, which is great, but since you are already touching this part you can consider to improve it: 'max_state' is constant after device creation, hence the check could be done at the beginning, which IMO would be clearer, it could also save an unnecessary devfreq_update_status() call and it wouldn't be necessary to hold the lock (one goto less). > len = sprintf(buf, " From : To\n"); > len += sprintf(buf + len, " :"); > for (i = 0; i < max_state; i++) > len += sprintf(buf + len, "%10lu", > @@ -1447,10 +1453,13 @@ static ssize_t trans_stat_show(struct device *dev, > jiffies_to_msecs(devfreq->time_in_state[i])); > } > > len += sprintf(buf + len, "Total transition : %u\n", > devfreq->total_trans); > + > +out: > + mutex_unlock(&devfreq->lock); > return len; > } > static DEVICE_ATTR_RO(trans_stat); > > static struct attribute *devfreq_attrs[] = { My only comments are possible improvements, but the change also looks good as is, so: Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
On 19.09.2019 00:28, Matthias Kaehlcke wrote: > Hi Leonard, > > this series doesn't indicate the version, from the change history in > the cover letter I suppose it is v5. Sorry about that, I forgot --subject-prefix. It is indeed v5 > On Wed, Sep 18, 2019 at 03:18:20AM +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. > > This and some other patches look like generic improvements and not > directly related to the series "PM / devfreq: Add dev_pm_qos > support". If there are no dependencies I think it is usually better to > send the improvements separately, it keeps the series more focussed > and might reduce version churn. Just my POV though ;-) The locking cleanups are required in order to initialize pm_qos request and notifiers without introducing lockdep warnings. pm_qos calls notifiers under dev_pm_qos_mtx and those notifiers needs to take &devfreq->lock. This means initializing pm_qos notifiers and requests must be done outside &devfreq->lock which needs some cleanups in devfreq_add_device. This particular patch is a more loosely related bugfix. Devfreq maintainers: would it help to post it separately? >> @@ -1415,15 +1416,20 @@ 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; >> >> + mutex_lock(&devfreq->lock); >> if (!devfreq->stop_polling && >> - devfreq_update_status(devfreq, devfreq->previous_freq)) >> - return 0; >> - if (max_state == 0) >> - return sprintf(buf, "Not Supported.\n"); >> + devfreq_update_status(devfreq, devfreq->previous_freq)) { >> + len = 0; > > you could assign 'len' in the declaration instead, but it's just > another option, it'ss fine as is >> + goto out; >> + } >> + if (max_state == 0) { >> + len = sprintf(buf, "Not Supported.\n"); >> + goto out; >> + } > > This leaves the general structure of the code as is, which is great, > but since you are already touching this part you can consider to > improve it: 'max_state' is constant after device creation, hence the > check could be done at the beginning, which IMO would be clearer, it > could also save an unnecessary devfreq_update_status() call and it > wouldn't be necessary to hold the lock (one goto less). Now that I look at this more closely &devfreq->lock only really needs to be held during the stats update, it can be released during sprintf. -- Regards, Leonard
On Thu, Sep 19, 2019 at 06:42:22PM +0000, Leonard Crestez wrote: > On 19.09.2019 00:28, Matthias Kaehlcke wrote: > > Hi Leonard, > > > > this series doesn't indicate the version, from the change history in > > the cover letter I suppose it is v5. > > Sorry about that, I forgot --subject-prefix. It is indeed v5 > > > On Wed, Sep 18, 2019 at 03:18:20AM +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. > > > > This and some other patches look like generic improvements and not > > directly related to the series "PM / devfreq: Add dev_pm_qos > > support". If there are no dependencies I think it is usually better to > > send the improvements separately, it keeps the series more focussed > > and might reduce version churn. Just my POV though ;-) > > The locking cleanups are required in order to initialize pm_qos request > and notifiers without introducing lockdep warnings. > > pm_qos calls notifiers under dev_pm_qos_mtx and those notifiers needs to > take &devfreq->lock. This means initializing pm_qos notifiers and > requests must be done outside &devfreq->lock which needs some cleanups > in devfreq_add_device. Thanks for the clarification! > This particular patch is a more loosely related bugfix. Devfreq > maintainers: would it help to post it separately? If it's just this single patch it probably isn't a problem, I'd be more concerned about multiple unrelated patches or if the changes are complex. > >> @@ -1415,15 +1416,20 @@ 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; > >> > >> + mutex_lock(&devfreq->lock); > >> if (!devfreq->stop_polling && > >> - devfreq_update_status(devfreq, devfreq->previous_freq)) > >> - return 0; > >> - if (max_state == 0) > >> - return sprintf(buf, "Not Supported.\n"); > >> + devfreq_update_status(devfreq, devfreq->previous_freq)) { > >> + len = 0; > > > > you could assign 'len' in the declaration instead, but it's just > > another option, it'ss fine as is > >> + goto out; > >> + } > >> + if (max_state == 0) { > >> + len = sprintf(buf, "Not Supported.\n"); > >> + goto out; > >> + } > > > > This leaves the general structure of the code as is, which is great, > > but since you are already touching this part you can consider to > > improve it: 'max_state' is constant after device creation, hence the > > check could be done at the beginning, which IMO would be clearer, it > > could also save an unnecessary devfreq_update_status() call and it > > wouldn't be necessary to hold the lock (one goto less). > > Now that I look at this more closely &devfreq->lock only really needs to > be held during the stats update, it can be released during sprintf. right, another simplification :)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 2494ee16f502..665575228c4f 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -159,10 +159,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; @@ -1415,15 +1416,20 @@ 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; + mutex_lock(&devfreq->lock); if (!devfreq->stop_polling && - devfreq_update_status(devfreq, devfreq->previous_freq)) - return 0; - if (max_state == 0) - return sprintf(buf, "Not Supported.\n"); + devfreq_update_status(devfreq, devfreq->previous_freq)) { + len = 0; + goto out; + } + if (max_state == 0) { + len = sprintf(buf, "Not Supported.\n"); + goto out; + } len = sprintf(buf, " From : To\n"); len += sprintf(buf + len, " :"); for (i = 0; i < max_state; i++) len += sprintf(buf + len, "%10lu", @@ -1447,10 +1453,13 @@ static ssize_t trans_stat_show(struct device *dev, jiffies_to_msecs(devfreq->time_in_state[i])); } len += sprintf(buf + len, "Total transition : %u\n", devfreq->total_trans); + +out: + mutex_unlock(&devfreq->lock); return len; } static DEVICE_ATTR_RO(trans_stat); static struct attribute *devfreq_attrs[] = {
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 | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)