| Message ID | 1418771379-24369-3-git-send-email-dtor@chromium.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
On 17 December 2014 at 04:39, Dmitry Torokhov <dtor@chromium.org> wrote: > Not having OPP defined for a device is not a crime, we should not splat > warning in this case. Also, it seems that we are ready to accept invalid > dev (find_device_opp will return ERR_PTR(-EINVAL) then) so let's not > crash in dev_name() in such case. > > Signed-off-by: Dmitry Torokhov <dtor@chromium.org> > --- > drivers/base/power/opp.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c > index b78c14d..413c7fe 100644 > --- a/drivers/base/power/opp.c > +++ b/drivers/base/power/opp.c > @@ -799,9 +799,15 @@ void of_free_opp_table(struct device *dev) > > /* Check for existing list for 'dev' */ > dev_opp = find_device_opp(dev); > - if (WARN(IS_ERR(dev_opp), "%s: dev_opp: %ld\n", dev_name(dev), > - PTR_ERR(dev_opp))) > + if (IS_ERR(dev_opp)) { > + int error = PTR_ERR(dev_opp); > + if (error != -ENODEV) > + WARN(1, "%s: dev_opp: %ld\n", > + IS_ERR_OR_NULL(dev) ? > + "Invalid device" : dev_name(dev), > + error); > return; What about this: if (IS_ERR(dev_opp)) { int error = PTR_ERR(dev_opp); WARN(error != -ENODEV, "%s: dev_opp: %ld\n", IS_ERR_OR_NULL(dev) ? "Invalid device" : dev_name(dev), error); return; } We can get rid of the extra indentation level and an extra comparison check. Otherwise: Acked-by: Viresh Kumar <viresh.kumar@linaro.org> -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
$subject: PM / OPP: Do not warn when no OPP was ever registered in of_free_opp_table might be more appropriate? On 12/16/2014 10:28 PM, Viresh Kumar wrote: > On 17 December 2014 at 04:39, Dmitry Torokhov <dtor@chromium.org> wrote: >> Not having OPP defined for a device is not a crime, we should not splat >> warning in this case. Also, it seems that we are ready to accept invalid >> dev (find_device_opp will return ERR_PTR(-EINVAL) then) so let's not >> crash in dev_name() in such case. >> >> Signed-off-by: Dmitry Torokhov <dtor@chromium.org> >> --- >> drivers/base/power/opp.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c >> index b78c14d..413c7fe 100644 >> --- a/drivers/base/power/opp.c >> +++ b/drivers/base/power/opp.c >> @@ -799,9 +799,15 @@ void of_free_opp_table(struct device *dev) >> >> /* Check for existing list for 'dev' */ >> dev_opp = find_device_opp(dev); >> - if (WARN(IS_ERR(dev_opp), "%s: dev_opp: %ld\n", dev_name(dev), >> - PTR_ERR(dev_opp))) >> + if (IS_ERR(dev_opp)) { >> + int error = PTR_ERR(dev_opp); >> + if (error != -ENODEV) >> + WARN(1, "%s: dev_opp: %ld\n", >> + IS_ERR_OR_NULL(dev) ? >> + "Invalid device" : dev_name(dev), >> + error); >> return; > > What about this: > > if (IS_ERR(dev_opp)) { > int error = PTR_ERR(dev_opp); > WARN(error != -ENODEV, "%s: dev_opp: %ld\n", > IS_ERR_OR_NULL(dev) ? "Invalid device" : dev_name(dev), > error); > return; > } Adds the following build warning: I suggest changing the %ld to %d "warning: format ‘%ld’ expects argument of type ‘long int’, but argument 5 has type ‘int’ [-Wformat]" This also fixes the warning I got on AM437x based platforms. test results: 1: am437x-sk: BOOT: PASS: crit=2 err=13 warn=25, CPUFreq: N/A, CPUIdle: N/A: http://slexy.org/raw/s20MAtNEbt 2: am437x-sk-before: BOOT: PASS: crit=2 err=13 warn=57, CPUFreq: N/A, CPUIdle: N/A: http://slexy.org/raw/s21rZCo4cD With the suggested changes, Acked-by: Nishanth Menon <nm@ti.com> > > We can get rid of the extra indentation level and an extra comparison check. > > Otherwise: > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> PS: I am sure you have already done some level of checks, but I suggest using something like aiaiai or https://github.com/nmenon/kernel_patch_verify or other solutions like those discussed in https://plus.google.com/112464029509057661457/posts/5LTuHHMyXLT to help do some initial clean up of the patches..
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index b78c14d..413c7fe 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -799,9 +799,15 @@ void of_free_opp_table(struct device *dev) /* Check for existing list for 'dev' */ dev_opp = find_device_opp(dev); - if (WARN(IS_ERR(dev_opp), "%s: dev_opp: %ld\n", dev_name(dev), - PTR_ERR(dev_opp))) + if (IS_ERR(dev_opp)) { + int error = PTR_ERR(dev_opp); + if (error != -ENODEV) + WARN(1, "%s: dev_opp: %ld\n", + IS_ERR_OR_NULL(dev) ? + "Invalid device" : dev_name(dev), + error); return; + } /* Hold our list modification lock here */ mutex_lock(&dev_opp_list_lock);
Not having OPP defined for a device is not a crime, we should not splat warning in this case. Also, it seems that we are ready to accept invalid dev (find_device_opp will return ERR_PTR(-EINVAL) then) so let's not crash in dev_name() in such case. Signed-off-by: Dmitry Torokhov <dtor@chromium.org> --- drivers/base/power/opp.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)