* [PATCH] reset: allow to pass NULL pointer to reset_control_put()
@ 2016-05-04 11:17 Masahiro Yamada
2016-05-04 11:24 ` Arnd Bergmann
0 siblings, 1 reply; 6+ messages in thread
From: Masahiro Yamada @ 2016-05-04 11:17 UTC (permalink / raw)
To: linux-kernel, Philipp Zabel; +Cc: Arnd Bergmann, Masahiro Yamada
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);
--
1.9.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] reset: allow to pass NULL pointer to reset_control_put()
2016-05-04 11:17 [PATCH] reset: allow to pass NULL pointer to reset_control_put() Masahiro Yamada
@ 2016-05-04 11:24 ` Arnd Bergmann
2016-05-04 11:34 ` Masahiro Yamada
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2016-05-04 11:24 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-kernel, Philipp Zabel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] reset: allow to pass NULL pointer to reset_control_put()
2016-05-04 11:24 ` Arnd Bergmann
@ 2016-05-04 11:34 ` Masahiro Yamada
2016-05-04 12:34 ` Philipp Zabel
2016-05-04 12:35 ` Arnd Bergmann
0 siblings, 2 replies; 6+ messages in thread
From: Masahiro Yamada @ 2016-05-04 11:34 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Linux Kernel Mailing List, Philipp Zabel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] reset: allow to pass NULL pointer to reset_control_put()
2016-05-04 11:34 ` Masahiro Yamada
@ 2016-05-04 12:34 ` Philipp Zabel
2016-05-04 12:47 ` Arnd Bergmann
2016-05-04 12:35 ` Arnd Bergmann
1 sibling, 1 reply; 6+ messages in thread
From: Philipp Zabel @ 2016-05-04 12:34 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: Arnd Bergmann, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] reset: allow to pass NULL pointer to reset_control_put()
2016-05-04 12:34 ` Philipp Zabel
@ 2016-05-04 12:47 ` Arnd Bergmann
0 siblings, 0 replies; 6+ messages in thread
From: Arnd Bergmann @ 2016-05-04 12:47 UTC (permalink / raw)
To: Philipp Zabel; +Cc: Masahiro Yamada, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] reset: allow to pass NULL pointer to reset_control_put()
2016-05-04 11:34 ` Masahiro Yamada
2016-05-04 12:34 ` Philipp Zabel
@ 2016-05-04 12:35 ` Arnd Bergmann
1 sibling, 0 replies; 6+ messages in thread
From: Arnd Bergmann @ 2016-05-04 12:35 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: Linux Kernel Mailing List, Philipp Zabel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-05-04 12:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-04 11:17 [PATCH] reset: allow to pass NULL pointer to reset_control_put() Masahiro Yamada
2016-05-04 11:24 ` Arnd Bergmann
2016-05-04 11:34 ` Masahiro Yamada
2016-05-04 12:34 ` Philipp Zabel
2016-05-04 12:47 ` Arnd Bergmann
2016-05-04 12:35 ` Arnd Bergmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®