| Message ID | 10b1e5f93e7594852aa5cbfe1309bf486f70ecbb.1570044052.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PM / devfreq: Add dev_pm_qos support | expand |
On Wed, Oct 02, 2019 at 10:25:05PM +0300, Leonard Crestez wrote: > Notifier callbacks shouldn't return negative errno but one of the > NOTIFY_OK/DONE/BAD values. > > The OPP core will ignore return values from notifiers but returning a > value that matches NOTIFY_STOP_MASK will stop the notification chain. > > Fix by always returning NOTIFY_OK. > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > --- > drivers/devfreq/devfreq.c | 24 +++++++++++++----------- > 1 file changed, 13 insertions(+), 11 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 7dc899da1172..32bbf6e80380 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -548,30 +548,32 @@ EXPORT_SYMBOL(devfreq_interval_update); > */ > static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, > void *devp) > { > struct devfreq *devfreq = container_of(nb, struct devfreq, nb); > - int ret; > + int err = -EINVAL; > > mutex_lock(&devfreq->lock); > > devfreq->scaling_min_freq = find_available_min_freq(devfreq); > - if (!devfreq->scaling_min_freq) { > - mutex_unlock(&devfreq->lock); > - return -EINVAL; > - } > + if (!devfreq->scaling_min_freq) > + goto out; > > devfreq->scaling_max_freq = find_available_max_freq(devfreq); > - if (!devfreq->scaling_max_freq) { > - mutex_unlock(&devfreq->lock); > - return -EINVAL; > - } > + if (!devfreq->scaling_max_freq) > + goto out; > + > + err = update_devfreq(devfreq); > > - ret = update_devfreq(devfreq); > +out: > mutex_unlock(&devfreq->lock); > + if (err) > + dev_err(devfreq->dev.parent, > + "failed to update frequency from OPP notifier (%d)\n", > + err); In case an OPP freq can't be found the log doesn't provide clues about what the problem could be. I couldn't find a clearly superior errno value though, so I guess this is as good as it gets, unless you want to have a dedicated message for those errors. Should be a rare exception anyway, and previously there was no log at all. Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
On 2019-10-03 12:24 AM, Matthias Kaehlcke wrote: > On Wed, Oct 02, 2019 at 10:25:05PM +0300, Leonard Crestez wrote: >> Notifier callbacks shouldn't return negative errno but one of the >> NOTIFY_OK/DONE/BAD values. >> >> The OPP core will ignore return values from notifiers but returning a >> value that matches NOTIFY_STOP_MASK will stop the notification chain. >> >> Fix by always returning NOTIFY_OK. >> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >> --- >> drivers/devfreq/devfreq.c | 24 +++++++++++++----------- >> 1 file changed, 13 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 7dc899da1172..32bbf6e80380 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -548,30 +548,32 @@ EXPORT_SYMBOL(devfreq_interval_update); >> */ >> static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, >> void *devp) >> { >> struct devfreq *devfreq = container_of(nb, struct devfreq, nb); >> - int ret; >> + int err = -EINVAL; >> >> mutex_lock(&devfreq->lock); >> >> devfreq->scaling_min_freq = find_available_min_freq(devfreq); >> - if (!devfreq->scaling_min_freq) { >> - mutex_unlock(&devfreq->lock); >> - return -EINVAL; >> - } >> + if (!devfreq->scaling_min_freq) >> + goto out; >> >> devfreq->scaling_max_freq = find_available_max_freq(devfreq); >> - if (!devfreq->scaling_max_freq) { >> - mutex_unlock(&devfreq->lock); >> - return -EINVAL; >> - } >> + if (!devfreq->scaling_max_freq) >> + goto out; >> + >> + err = update_devfreq(devfreq); >> >> - ret = update_devfreq(devfreq); >> +out: >> mutex_unlock(&devfreq->lock); >> + if (err) >> + dev_err(devfreq->dev.parent, >> + "failed to update frequency from OPP notifier (%d)\n", >> + err); > > In case an OPP freq can't be found the log doesn't provide clues > about what the problem could be. I couldn't find a clearly superior > errno value though, so I guess this is as good as it gets, unless > you want to have a dedicated message for those errors. Should be a > rare exception anyway, and previously there was no log at all. I guess it could happen if all OPPs are disabled after probe? The devfreq core will attempt to switch away if the current OPP get disabled but if nothing else is available then printing an error and sticking to the current frequency seems reasonable. It would indicate a bug somewhere else. > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
On 19. 10. 3. 오전 4:25, Leonard Crestez wrote: > Notifier callbacks shouldn't return negative errno but one of the > NOTIFY_OK/DONE/BAD values. > > The OPP core will ignore return values from notifiers but returning a > value that matches NOTIFY_STOP_MASK will stop the notification chain. > > Fix by always returning NOTIFY_OK. > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > --- > drivers/devfreq/devfreq.c | 24 +++++++++++++----------- > 1 file changed, 13 insertions(+), 11 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 7dc899da1172..32bbf6e80380 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -548,30 +548,32 @@ EXPORT_SYMBOL(devfreq_interval_update); > */ > static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, > void *devp) > { > struct devfreq *devfreq = container_of(nb, struct devfreq, nb); > - int ret; > + int err = -EINVAL; > > mutex_lock(&devfreq->lock); > > devfreq->scaling_min_freq = find_available_min_freq(devfreq); > - if (!devfreq->scaling_min_freq) { > - mutex_unlock(&devfreq->lock); > - return -EINVAL; > - } > + if (!devfreq->scaling_min_freq) > + goto out; > > devfreq->scaling_max_freq = find_available_max_freq(devfreq); > - if (!devfreq->scaling_max_freq) { > - mutex_unlock(&devfreq->lock); > - return -EINVAL; > - } > + if (!devfreq->scaling_max_freq) > + goto out; > + > + err = update_devfreq(devfreq); > > - ret = update_devfreq(devfreq); > +out: > mutex_unlock(&devfreq->lock); > + if (err) > + dev_err(devfreq->dev.parent, > + "failed to update frequency from OPP notifier (%d)\n", > + err); > > - return ret; > + return NOTIFY_OK; > } > > /** > * devfreq_dev_release() - Callback for struct device to release the device. > * @dev: the devfreq device > Sorry for the late reply. Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 7dc899da1172..32bbf6e80380 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -548,30 +548,32 @@ EXPORT_SYMBOL(devfreq_interval_update); */ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, void *devp) { struct devfreq *devfreq = container_of(nb, struct devfreq, nb); - int ret; + int err = -EINVAL; mutex_lock(&devfreq->lock); devfreq->scaling_min_freq = find_available_min_freq(devfreq); - if (!devfreq->scaling_min_freq) { - mutex_unlock(&devfreq->lock); - return -EINVAL; - } + if (!devfreq->scaling_min_freq) + goto out; devfreq->scaling_max_freq = find_available_max_freq(devfreq); - if (!devfreq->scaling_max_freq) { - mutex_unlock(&devfreq->lock); - return -EINVAL; - } + if (!devfreq->scaling_max_freq) + goto out; + + err = update_devfreq(devfreq); - ret = update_devfreq(devfreq); +out: mutex_unlock(&devfreq->lock); + if (err) + dev_err(devfreq->dev.parent, + "failed to update frequency from OPP notifier (%d)\n", + err); - return ret; + return NOTIFY_OK; } /** * devfreq_dev_release() - Callback for struct device to release the device. * @dev: the devfreq device
Notifier callbacks shouldn't return negative errno but one of the NOTIFY_OK/DONE/BAD values. The OPP core will ignore return values from notifiers but returning a value that matches NOTIFY_STOP_MASK will stop the notification chain. Fix by always returning NOTIFY_OK. Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> --- drivers/devfreq/devfreq.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)