| Message ID | 1462360671-13668-1-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | New |
| Headers | show |
On Wednesday 04 May 2016 20:17:51 Masahiro Yamada wrote: > Currently, reset_control_put() just returns for error pointer, > but not for NULL pointer. This is not reasonable. > > Passing NULL pointer should be allowed as well to make failure path > handling easier. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > --- > > drivers/reset/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/reset/core.c b/drivers/reset/core.c > index 181b05d..7bb16d1 100644 > --- a/drivers/reset/core.c > +++ b/drivers/reset/core.c > @@ -288,7 +288,7 @@ EXPORT_SYMBOL_GPL(reset_control_get); > > void reset_control_put(struct reset_control *rstc) > { > - if (IS_ERR(rstc)) > + if (IS_ERR_OR_NULL(rstc)) > return; > > module_put(rstc->rcdev->owner); Using IS_ERR_OR_NULL() normally indicates that there is something wrong with the API, or with the caller. What exactly is the idea behind treating an error pointer as a valid input to reset_control_put() here? Maybe it should just test for NULL? Arnd
Hi Arnd, 2016-05-04 20:24 GMT+09:00 Arnd Bergmann <arnd@arndb.de>: > On Wednesday 04 May 2016 20:17:51 Masahiro Yamada wrote: >> Currently, reset_control_put() just returns for error pointer, >> but not for NULL pointer. This is not reasonable. >> >> Passing NULL pointer should be allowed as well to make failure path >> handling easier. >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >> --- >> >> drivers/reset/core.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/reset/core.c b/drivers/reset/core.c >> index 181b05d..7bb16d1 100644 >> --- a/drivers/reset/core.c >> +++ b/drivers/reset/core.c >> @@ -288,7 +288,7 @@ EXPORT_SYMBOL_GPL(reset_control_get); >> >> void reset_control_put(struct reset_control *rstc) >> { >> - if (IS_ERR(rstc)) >> + if (IS_ERR_OR_NULL(rstc)) >> return; >> >> module_put(rstc->rcdev->owner); > > Using IS_ERR_OR_NULL() normally indicates that there is something > wrong with the API, or with the caller. > > What exactly is the idea behind treating an error pointer as a valid > input to reset_control_put() here? Maybe it should just test for > NULL? > I thought about that a bit, but there might be some (not nice) drivers that rely on the current behavior. I did not want to break any boards with my patch. So, should it be if (!rstc) return; or, perhaps if (!rstc || WARN_ON_ONCE(IS_ERR(rstc))) return; ? -- Best Regards Masahiro Yamada
Am Mittwoch, den 04.05.2016, 20:34 +0900 schrieb Masahiro Yamada: > Hi Arnd, > > 2016-05-04 20:24 GMT+09:00 Arnd Bergmann <arnd@arndb.de>: > > On Wednesday 04 May 2016 20:17:51 Masahiro Yamada wrote: > >> Currently, reset_control_put() just returns for error pointer, > >> but not for NULL pointer. This is not reasonable. > >> > >> Passing NULL pointer should be allowed as well to make failure path > >> handling easier. > >> > >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > >> --- > >> > >> drivers/reset/core.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/reset/core.c b/drivers/reset/core.c > >> index 181b05d..7bb16d1 100644 > >> --- a/drivers/reset/core.c > >> +++ b/drivers/reset/core.c > >> @@ -288,7 +288,7 @@ EXPORT_SYMBOL_GPL(reset_control_get); > >> > >> void reset_control_put(struct reset_control *rstc) > >> { > >> - if (IS_ERR(rstc)) > >> + if (IS_ERR_OR_NULL(rstc)) > >> return; > >> > >> module_put(rstc->rcdev->owner); > > > > Using IS_ERR_OR_NULL() normally indicates that there is something > > wrong with the API, or with the caller. > > > > What exactly is the idea behind treating an error pointer as a valid > > input to reset_control_put() here? Maybe it should just test for > > NULL? The idea was that you could do drvdata->rstc = reset_control_get(...) in the probe() function and reset_control_put(drvdata->rstc) in remove() without having to check for IS_ERR(drvdata->rstc) again. I'm not convinced this is necessarily a good idea though. To simplify the teardown path we already have devm_reset_control_get(). > I thought about that a bit, > but there might be some (not nice) drivers that rely on the current behavior. > I did not want to break any boards with my patch. > > So, should it be > > if (!rstc) > return; > or, perhaps > > if (!rstc || WARN_ON_ONCE(IS_ERR(rstc))) > return; > > ? NULL is not a valid input to reset_control, reset_control_get(_optional) should never return NULL. I'd be in favor of turning this into if (WARN_ON(IS_ERR_OR_NULL(rstc))) return; As far as I am aware, ehci-tegra is the only driver that currently makes use of the IS_ERR(rstc) return in reset_control_put(): struct reset_control *usb1_reset; usb1_reset = of_reset_control_get(phy_np, "usb"); if (IS_ERR(usb1_reset)) { /* ... */ } else { reset_control_assert(usb1_reset); udelay(1); reset_control_deassert(usb1_reset); } reset_control_put(usb1_reset); That'd be trivial to fix. regards Philipp
On Wednesday 04 May 2016 20:34:09 Masahiro Yamada wrote: > I thought about that a bit, > but there might be some (not nice) drivers that rely on the current behavior. > I did not want to break any boards with my patch. > > So, should it be > > if (!rstc) > return; > or, perhaps > > if (!rstc || WARN_ON_ONCE(IS_ERR(rstc))) > return; I think the latter is fine, but it would also be good which of the six callers of the function actually rely on that behavior today, if any. Arnd
On Wednesday 04 May 2016 14:34:43 Philipp Zabel wrote: > Am Mittwoch, den 04.05.2016, 20:34 +0900 schrieb Masahiro Yamada: > > Hi Arnd, > > > > 2016-05-04 20:24 GMT+09:00 Arnd Bergmann <arnd@arndb.de>: > > > On Wednesday 04 May 2016 20:17:51 Masahiro Yamada wrote: > > >> Currently, reset_control_put() just returns for error pointer, > > >> but not for NULL pointer. This is not reasonable. > > >> > > >> Passing NULL pointer should be allowed as well to make failure path > > >> handling easier. > > >> > > >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > >> --- > > >> > > >> drivers/reset/core.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/reset/core.c b/drivers/reset/core.c > > >> index 181b05d..7bb16d1 100644 > > >> --- a/drivers/reset/core.c > > >> +++ b/drivers/reset/core.c > > >> @@ -288,7 +288,7 @@ EXPORT_SYMBOL_GPL(reset_control_get); > > >> > > >> void reset_control_put(struct reset_control *rstc) > > >> { > > >> - if (IS_ERR(rstc)) > > >> + if (IS_ERR_OR_NULL(rstc)) > > >> return; > > >> > > >> module_put(rstc->rcdev->owner); > > > > > > Using IS_ERR_OR_NULL() normally indicates that there is something > > > wrong with the API, or with the caller. > > > > > > What exactly is the idea behind treating an error pointer as a valid > > > input to reset_control_put() here? Maybe it should just test for > > > NULL? > > The idea was that you could do > drvdata->rstc = reset_control_get(...) > in the probe() function and > reset_control_put(drvdata->rstc) > in remove() without having to check for IS_ERR(drvdata->rstc) again. > I'm not convinced this is necessarily a good idea though. To simplify > the teardown path we already have devm_reset_control_get(). > > > I thought about that a bit, > > but there might be some (not nice) drivers that rely on the current behavior. > > I did not want to break any boards with my patch. > > > > So, should it be > > > > if (!rstc) > > return; > > or, perhaps > > > > if (!rstc || WARN_ON_ONCE(IS_ERR(rstc))) > > return; > > > > ? > > NULL is not a valid input to reset_control, reset_control_get(_optional) > should never return NULL. > > I'd be in favor of turning this into > > if (WARN_ON(IS_ERR_OR_NULL(rstc))) > return; Sounds good to me too. We'd still have to think about whatever Masahiro was trying to do and how his caller should be written, but hopefully there is a good solution. > As far as I am aware, ehci-tegra is the only driver that currently makes > use of the IS_ERR(rstc) return in reset_control_put(): > > struct reset_control *usb1_reset; > > usb1_reset = of_reset_control_get(phy_np, "usb"); > if (IS_ERR(usb1_reset)) { > /* ... */ > } else { > reset_control_assert(usb1_reset); > udelay(1); > reset_control_deassert(usb1_reset); > } > reset_control_put(usb1_reset); > > That'd be trivial to fix. Ah, good. Could the above code just be converted into a variation of device_reset() that takes the name of a reset line? Arnd
diff --git a/drivers/reset/core.c b/drivers/reset/core.c index 181b05d..7bb16d1 100644 --- a/drivers/reset/core.c +++ b/drivers/reset/core.c @@ -288,7 +288,7 @@ EXPORT_SYMBOL_GPL(reset_control_get); void reset_control_put(struct reset_control *rstc) { - if (IS_ERR(rstc)) + if (IS_ERR_OR_NULL(rstc)) return; module_put(rstc->rcdev->owner);
Currently, reset_control_put() just returns for error pointer, but not for NULL pointer. This is not reasonable. Passing NULL pointer should be allowed as well to make failure path handling easier. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- drivers/reset/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 1.9.1