[PATCH v1] reset: Make optional stuff optional for all users

[PATCH v1] reset: Make optional stuff optional for all users

[Andy Shevchenko]

There is Device Tree oriented check for optional resource. Of course
it will fail on non-DT platforms.

Remove this check to make things optional for all users.

Fixes: bb475230b8e5 ("reset: make optional functions really optional")
Cc: Ramiro Oliveira <Ramiro.Oliveira@synopsys.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

 The reset framework is too Device Tree oriented, and who knows what
 the logic was behind the commit which introduced devm_reset_*()
 functions without thinking out of the DT box.

 This commit fixes almost all Intel newest boards that have no legacy
 UART since UART driver started using this DT-centric framework.

 drivers/reset/core.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index f1e5e65388bb..62314e663f29 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -331,9 +331,6 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
 	int rstc_id;
 	int ret;
 
-	if (!node)
-		return ERR_PTR(-EINVAL);
-
 	if (id) {
 		index = of_property_match_string(node,
 						 "reset-names", id);
-- 
2.11.0

Re: [PATCH v1] reset: Make optional stuff optional for all users

[Philipp Zabel]

Hi Andy,

thank you for the patch.

On Mon, 2017-04-03 at 15:26 +0300, Andy Shevchenko wrote:
> There is Device Tree oriented check for optional resource. Of course
> it will fail on non-DT platforms.
> 
> Remove this check to make things optional for all users.
> 
> Fixes: bb475230b8e5 ("reset: make optional functions really optional")
> Cc: Ramiro Oliveira <Ramiro.Oliveira@synopsys.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> 
>  The reset framework is too Device Tree oriented, and who knows what
>  the logic was behind the commit which introduced devm_reset_*()
>  functions without thinking out of the DT box.

At the time, there was nothing outside of the box to describe reset
lines, and as far as I am aware there still isn't, so the devm_reset_*
should behave as if the reset line is not specified in the non-DT case.
Returning -EINVAL was reasonable in that case, before the API was
changed to describe unavailable, optional reset controls as rstc = NULL.

>  This commit fixes almost all Intel newest boards that have no legacy
>  UART since UART driver started using this DT-centric framework.

Is this is about 8250_dw.c? Unfortunately it sometimes takes a little
while for me to get updated on the big picture, as I only get the actual
reset driver and framework patches in my inbox. Usually I only see the
reset consumer changes when I actively look for them.

But the fault in this case was is with me not considering all possible
code paths influenced by commit bb475230b8e5, in the configurations that
I can't test myself.

>  drivers/reset/core.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index f1e5e65388bb..62314e663f29 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -331,9 +331,6 @@ struct reset_control *__of_reset_control_get(struct device_node *node,

Ideally, __of_reset_control_get would not be called at all in the non-DT
case. I'll change that in the next round, but for now I'd prefer a small
fix in place.

>  	int rstc_id;
>  	int ret;
>  
> -	if (!node)
> -		return ERR_PTR(-EINVAL);
> -

This should be

	if (!node)
		return optional ? NULL : ERR_PTR(-EINVAL);

instead. Can you confirm this works for Intel boards with DW UART? I can
fix it up when applying if you agree.

>  	if (id) {
>  		index = of_property_match_string(node,
>  						 "reset-names", id);

regards
Philipp

Re: [PATCH v1] reset: Make optional stuff optional for all users

[Andy Shevchenko]

On Mon, 2017-04-03 at 16:27 +0200, Philipp Zabel wrote:
> Hi Andy,
> 
> thank you for the patch.
> 
> On Mon, 2017-04-03 at 15:26 +0300, Andy Shevchenko wrote:
> > There is Device Tree oriented check for optional resource. Of course
> > it will fail on non-DT platforms.
> > 
> > Remove this check to make things optional for all users.
> > 
> > Fixes: bb475230b8e5 ("reset: make optional functions really
> > optional")
> > Cc: Ramiro Oliveira <Ramiro.Oliveira@synopsys.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > 
> >  The reset framework is too Device Tree oriented, and who knows what
> >  the logic was behind the commit which introduced devm_reset_*()
> >  functions without thinking out of the DT box.
> 
> At the time, there was nothing outside of the box to describe reset
> lines, and as far as I am aware there still isn't, so the devm_reset_*
> should behave as if the reset line is not specified in the non-DT
> case.

> Returning -EINVAL was reasonable in that case, before the API was
> changed to describe unavailable, optional reset controls as rstc =
> NULL.

Fair enough.

> 
> >  This commit fixes almost all Intel newest boards that have no
> > legacy
> >  UART since UART driver started using this DT-centric framework.
> 
> Is this is about 8250_dw.c? Unfortunately it sometimes takes a little
> while for me to get updated on the big picture, as I only get the
> actual
> reset driver and framework patches in my inbox. Usually I only see the
> reset consumer changes when I actively look for them.

Yes, 8250_dw.c in this case.

> 
> But the fault in this case was is with me not considering all possible
> code paths influenced by commit bb475230b8e5, in the configurations
> that
> I can't test myself.

Understood.

> 
> >  drivers/reset/core.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> > index f1e5e65388bb..62314e663f29 100644
> > --- a/drivers/reset/core.c
> > +++ b/drivers/reset/core.c
> > @@ -331,9 +331,6 @@ struct reset_control
> > *__of_reset_control_get(struct device_node *node,
> 
> Ideally, __of_reset_control_get would not be called at all in the non-
> DT
> case. I'll change that in the next round, but for now I'd prefer a
> small
> fix in place.
> 
> >  	int rstc_id;
> >  	int ret;
> >  
> > -	if (!node)
> > -		return ERR_PTR(-EINVAL);
> > -
> 
> This should be
> 
> 	if (!node)
> 		return optional ? NULL : ERR_PTR(-EINVAL);
> 
> instead. Can you confirm this works for Intel boards with DW UART? I
> can
> fix it up when applying if you agree.

I don't think it worth to change. I specifically checked all of_* calls
in that function and they cope pretty nice with node == NULL.

So, I rather to go with my initial change.

Thanks for review!

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1] reset: Make optional stuff optional for all users

[Shevchenko, Andriy]

On Mon, 2017-04-03 at 17:31 +0300, Andy Shevchenko wrote:
> On Mon, 2017-04-03 at 16:27 +0200, Philipp Zabel wrote:
> > 

> > >  	int rstc_id;
> > >  	int ret;
> > >  
> > > -	if (!node)
> > > -		return ERR_PTR(-EINVAL);
> > > -
> > 
> > This should be
> > 
> > 	if (!node)
> > 		return optional ? NULL : ERR_PTR(-EINVAL);
> > 
> > instead. Can you confirm this works for Intel boards with DW UART? I
> > can
> > fix it up when applying if you agree.
> 
> I don't think it worth to change. I specifically checked all of_*
> calls
> in that function and they cope pretty nice with node == NULL.
> 
> So, I rather to go with my initial change.
> 

Hit Enter before closing another thought.

When you come with solution where this __of_reset_control_get() will be
called only for node != NULL case you will not need that check either.

So, I would go my solution because of two benefits:
- it fixes bug
- if will not bring ping-ponging code

> Thanks for review!
> 

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

Re: [PATCH v1] reset: Make optional stuff optional for all users

[Philipp Zabel]

On Mon, 2017-04-03 at 14:33 +0000, Shevchenko, Andriy wrote:
> On Mon, 2017-04-03 at 17:31 +0300, Andy Shevchenko wrote:
> > On Mon, 2017-04-03 at 16:27 +0200, Philipp Zabel wrote:
> > > 
> 
> > > >  	int rstc_id;
> > > >  	int ret;
> > > >  
> > > > -	if (!node)
> > > > -		return ERR_PTR(-EINVAL);
> > > > -
> > > 
> > > This should be
> > > 
> > > 	if (!node)
> > > 		return optional ? NULL : ERR_PTR(-EINVAL);
> > > 
> > > instead. Can you confirm this works for Intel boards with DW UART? I
> > > can
> > > fix it up when applying if you agree.
> > 
> > I don't think it worth to change. I specifically checked all of_*
> > calls
> > in that function and they cope pretty nice with node == NULL.

__of_reset_control_get called with id != NULL calls
of_property_match_string first, which then returns -EINVAL if
node == NULL, which makes __of_reset_control_get return NULL if optional
or -ENOENT otherwise, even though the correct return value would be
-EINVAL in the DT case.

__of_reset_control_get called with id == NULL calls
of_parse_phandle_with_args first, which calls
__of_parse_phandle_with_args, which returns an undefined value if
np == NULL, as far as I can tell:
of_for_each_phandle first calls of_phandle_iterator_init, which, when
called with np == NULL clears the iterator structure returns -ENOENT.
The return value is ignored in the of_for_each_phandle macro, and
of_phandle_iterator_next is then called and returns -ENOENT because
it->cur == NULL, ending the loop without ever assigning a value to rc.
__of_parse_phandle_with_args then returns the uninitialized value.

The point being, instead of having to regularly forage through a number
of of_ API functions to make sure my expectations are still met, I'd
prefer to keep the check in place.

> > 
> > So, I rather to go with my initial change.
> > 
> 
> Hit Enter before closing another thought.
> 
> When you come with solution where this __of_reset_control_get() will be
> called only for node != NULL case you will not need that check either.

__of_reset_control_get is public API (via of_reset_control_get), so I
can't guarantee node != NULL even in the DT case.

> So, I would go my solution because of two benefits:
> - it fixes bug

True.

> - if will not bring ping-ponging code

Unfortunately not.

regards
Philipp

Re: [PATCH v1] reset: Make optional stuff optional for all users

[Andy Shevchenko]

On Mon, 2017-04-03 at 17:09 +0200, Philipp Zabel wrote:
> On Mon, 2017-04-03 at 14:33 +0000, Shevchenko, Andriy wrote:
> > On Mon, 2017-04-03 at 17:31 +0300, Andy Shevchenko wrote:
> > > On Mon, 2017-04-03 at 16:27 +0200, Philipp Zabel wrote:
> > > > 
> > > > >  	int rstc_id;
> > > > >  	int ret;
> > > > >  
> > > > > -	if (!node)
> > > > > -		return ERR_PTR(-EINVAL);
> > > > > -
> > > > 
> > > > This should be
> > > > 
> > > > 	if (!node)
> > > > 		return optional ? NULL : ERR_PTR(-EINVAL);
> > > > 
> > > > instead. Can you confirm this works for Intel boards with DW
> > > > UART? I
> > > > can
> > > > fix it up when applying if you agree.
> > > 
> > > I don't think it worth to change. I specifically checked all of_*
> > > calls
> > > in that function and they cope pretty nice with node == NULL.
> 
> __of_reset_control_get called with id != NULL calls
> of_property_match_string first, which then returns -EINVAL if
> node == NULL, which makes __of_reset_control_get return NULL if
> optional
> or -ENOENT otherwise, even though the correct return value would be
> -EINVAL in the DT case.

Error handling mess as usual. :-)

> 
> __of_reset_control_get called with id == NULL calls
> of_parse_phandle_with_args first, which calls
> __of_parse_phandle_with_args, which returns an undefined value if
> np == NULL, as far as I can tell:
> of_for_each_phandle first calls of_phandle_iterator_init, which, when
> called with np == NULL clears the iterator structure returns -ENOENT.
> The return value is ignored in the of_for_each_phandle macro, and
> of_phandle_iterator_next is then called and returns -ENOENT because
> it->cur == NULL, ending the loop without ever assigning a value to rc.
> __of_parse_phandle_with_args then returns the uninitialized value.

It returns -ENOENT. Error value is kept in function variable rc.

> 
> The point being, instead of having to regularly forage through a
> number
> of of_ API functions to make sure my expectations are still met, I'd
> prefer to keep the check in place.

I would not insist, I already shared my view on this.

I really don't like ping-ponging of the code. Perhaps you would fix it
once for the best now?

> 
> > > 
> > > So, I rather to go with my initial change.
> > > 
> > 
> > Hit Enter before closing another thought.
> > 
> > When you come with solution where this __of_reset_control_get() will
> > be
> > called only for node != NULL case you will not need that check
> > either.
> 
> __of_reset_control_get is public API (via of_reset_control_get), so I
> can't guarantee node != NULL even in the DT case.

Yes, and that's why other callees will return some error codes there.

> 
> > So, I would go my solution because of two benefits:
> > - it fixes bug
> 
> True.
> 
> > - if will not bring ping-ponging code
> 
> Unfortunately not.

Fortunately yes, if someone will fix DT error code mess in the first
place.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1] reset: Make optional stuff optional for all users

[Philipp Zabel]

On Mon, 2017-04-03 at 18:23 +0300, Andy Shevchenko wrote:
> On Mon, 2017-04-03 at 17:09 +0200, Philipp Zabel wrote:
> > On Mon, 2017-04-03 at 14:33 +0000, Shevchenko, Andriy wrote:
> > > On Mon, 2017-04-03 at 17:31 +0300, Andy Shevchenko wrote:
> > > > On Mon, 2017-04-03 at 16:27 +0200, Philipp Zabel wrote:
> > > > > 
> > > > > >  	int rstc_id;
> > > > > >  	int ret;
> > > > > >  
> > > > > > -	if (!node)
> > > > > > -		return ERR_PTR(-EINVAL);
> > > > > > -
> > > > > 
> > > > > This should be
> > > > > 
> > > > > 	if (!node)
> > > > > 		return optional ? NULL : ERR_PTR(-EINVAL);
> > > > > 
> > > > > instead. Can you confirm this works for Intel boards with DW
> > > > > UART? I
> > > > > can
> > > > > fix it up when applying if you agree.
> > > > 
> > > > I don't think it worth to change. I specifically checked all of_*
> > > > calls
> > > > in that function and they cope pretty nice with node == NULL.
> > 
> > __of_reset_control_get called with id != NULL calls
> > of_property_match_string first, which then returns -EINVAL if
> > node == NULL, which makes __of_reset_control_get return NULL if
> > optional
> > or -ENOENT otherwise, even though the correct return value would be
> > -EINVAL in the DT case.
> 
> Error handling mess as usual. :-)
>
> > 
> > __of_reset_control_get called with id == NULL calls
> > of_parse_phandle_with_args first, which calls
> > __of_parse_phandle_with_args, which returns an undefined value if
> > np == NULL, as far as I can tell:
> > of_for_each_phandle first calls of_phandle_iterator_init, which, when
> > called with np == NULL clears the iterator structure returns -ENOENT.
> > The return value is ignored in the of_for_each_phandle macro, and
> > of_phandle_iterator_next is then called and returns -ENOENT because
> > it->cur == NULL, ending the loop without ever assigning a value to rc.
> > __of_parse_phandle_with_args then returns the uninitialized value.
> 
> It returns -ENOENT. Error value is kept in function variable rc.

Thanks for the correction. That's still the wrong error code IMHO, but
it certainly makes your patch unproblematic.

> > The point being, instead of having to regularly forage through a
> > number
> > of of_ API functions to make sure my expectations are still met, I'd
> > prefer to keep the check in place.
> 
> I would not insist, I already shared my view on this.

Appreciated. With a correct understanding of the current of_* error
return paths it makes a lot more sense.

> I really don't like ping-ponging of the code. Perhaps you would fix it
> once for the best now?

How about:

----------8<----------
>From 75205aeaf52cef5c62710630cceb871468175c12 Mon Sep 17 00:00:00 2001
From: Philipp Zabel <p.zabel@pengutronix.de>
Date: Fri, 5 Feb 2016 13:41:39 +0100
Subject: [PATCH] reset: add exported __reset_control_get, return NULL if
 optional

Rename the internal __reset_control_get/put functions to
__reset_control_get/put_internal and add an exported
__reset_control_get equivalent to __of_reset_control_get
that takes a struct device parameter.
This avoids the confusing call to __of_reset_control_get in
the non-DT case and fixes the devm_reset_control_get_optional
function to return NULL if RESET_CONTROLLER is enabled but
dev->of_node == NULL.

Fixes: bb475230b8e5 ("reset: make optional functions really optional")
Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Ramiro Oliveira <Ramiro.Oliveira@synopsys.com>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/reset/core.c  | 22 ++++++++++++++++------
 include/linux/reset.h | 22 ++++++++++++++--------
 2 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index f1e5e65388bb5..d227168c8971a 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -275,7 +275,7 @@ int reset_control_status(struct reset_control *rstc)
 }
 EXPORT_SYMBOL_GPL(reset_control_status);
 
-static struct reset_control *__reset_control_get(
+static struct reset_control *__reset_control_get_internal(
 				struct reset_controller_dev *rcdev,
 				unsigned int index, bool shared)
 {
@@ -308,7 +308,7 @@ static struct reset_control *__reset_control_get(
 	return rstc;
 }
 
-static void __reset_control_put(struct reset_control *rstc)
+static void __reset_control_put_internal(struct reset_control *rstc)
 {
 	lockdep_assert_held(&reset_list_mutex);
 
@@ -377,7 +377,7 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
 	}
 
 	/* reset_list_mutex also protects the rcdev's reset_control list */
-	rstc = __reset_control_get(rcdev, rstc_id, shared);
+	rstc = __reset_control_get_internal(rcdev, rstc_id, shared);
 
 	mutex_unlock(&reset_list_mutex);
 
@@ -385,6 +385,17 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
 }
 EXPORT_SYMBOL_GPL(__of_reset_control_get);
 
+struct reset_control *__reset_control_get(struct device *dev, const char *id,
+					  int index, bool shared, bool optional)
+{
+	if (dev->of_node)
+		return __of_reset_control_get(dev->of_node, id, index, shared,
+					      optional);
+	else
+		return optional ? NULL : ERR_PTR(-EINVAL);
+}
+EXPORT_SYMBOL_GPL(__reset_control_get);
+
 /**
  * reset_control_put - free the reset controller
  * @rstc: reset controller
@@ -396,7 +407,7 @@ void reset_control_put(struct reset_control *rstc)
 		return;
 
 	mutex_lock(&reset_list_mutex);
-	__reset_control_put(rstc);
+	__reset_control_put_internal(rstc);
 	mutex_unlock(&reset_list_mutex);
 }
 EXPORT_SYMBOL_GPL(reset_control_put);
@@ -417,8 +428,7 @@ struct reset_control *__devm_reset_control_get(struct device *dev,
 	if (!ptr)
 		return ERR_PTR(-ENOMEM);
 
-	rstc = __of_reset_control_get(dev ? dev->of_node : NULL,
-				      id, index, shared, optional);
+	rstc = __reset_control_get(dev, id, index, shared, optional);
 	if (!IS_ERR(rstc)) {
 		*ptr = rstc;
 		devres_add(dev, ptr);
diff --git a/include/linux/reset.h b/include/linux/reset.h
index 96fb139bdd08f..13d8681210d54 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -15,6 +15,9 @@ int reset_control_status(struct reset_control *rstc);
 struct reset_control *__of_reset_control_get(struct device_node *node,
 				     const char *id, int index, bool shared,
 				     bool optional);
+struct reset_control *__reset_control_get(struct device *dev, const char *id,
+					  int index, bool shared,
+					  bool optional);
 void reset_control_put(struct reset_control *rstc);
 struct reset_control *__devm_reset_control_get(struct device *dev,
 				     const char *id, int index, bool shared,
@@ -72,6 +75,13 @@ static inline struct reset_control *__of_reset_control_get(
 	return optional ? NULL : ERR_PTR(-ENOTSUPP);
 }
 
+static inline struct reset_control *__reset_control_get(
+					struct device *dev, const char *id,
+					int index, bool shared, bool optional)
+{
+	return optional ? NULL : ERR_PTR(-ENOTSUPP);
+}
+
 static inline struct reset_control *__devm_reset_control_get(
 					struct device *dev, const char *id,
 					int index, bool shared, bool optional)
@@ -102,8 +112,7 @@ __must_check reset_control_get_exclusive(struct device *dev, const char *id)
 #ifndef CONFIG_RESET_CONTROLLER
 	WARN_ON(1);
 #endif
-	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, false,
-									false);
+	return __reset_control_get(dev, id, 0, false, false);
 }
 
 /**
@@ -131,22 +140,19 @@ __must_check reset_control_get_exclusive(struct device *dev, const char *id)
 static inline struct reset_control *reset_control_get_shared(
 					struct device *dev, const char *id)
 {
-	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, true,
-									false);
+	return __reset_control_get(dev, id, 0, true, false);
 }
 
 static inline struct reset_control *reset_control_get_optional_exclusive(
 					struct device *dev, const char *id)
 {
-	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, false,
-									true);
+	return __reset_control_get(dev, id, 0, false, true);
 }
 
 static inline struct reset_control *reset_control_get_optional_shared(
 					struct device *dev, const char *id)
 {
-	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, true,
-									true);
+	return __reset_control_get(dev, id, 0, true, true);
 }
 
 /**
-- 
2.11.0
---------->8----------

That should make both the __reset_control_get stub and implementation
return NULL if optional && (dev->of_node == NULL), and stop calling
__of_reset_control_get in the non-DT case, while maintaining the -EINVAL
error return value if of_reset_control_get* is called with node == NULL.

regards
Philipp

Re: [PATCH v1] reset: Make optional stuff optional for all users

[Andy Shevchenko]

On Mon, Apr 3, 2017 at 7:04 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> On Mon, 2017-04-03 at 18:23 +0300, Andy Shevchenko wrote:
>> On Mon, 2017-04-03 at 17:09 +0200, Philipp Zabel wrote:
>> > On Mon, 2017-04-03 at 14:33 +0000, Shevchenko, Andriy wrote:
>> > > On Mon, 2017-04-03 at 17:31 +0300, Andy Shevchenko wrote:

>> I really don't like ping-ponging of the code. Perhaps you would fix it
>> once for the best now?
>
> How about:

Below looks fine to me, one nit still and I would like to test it tomorrow.
Thanks for taking care.

> +struct reset_control *__reset_control_get(struct device *dev, const char *id,
> +                                         int index, bool shared, bool optional)
> +{
> +       if (dev->of_node)
> +               return __of_reset_control_get(dev->of_node, id, index, shared,
> +
                                optional);
> +       else

Redundant.

> +               return optional ? NULL : ERR_PTR(-EINVAL);
> +}
> +EXPORT_SYMBOL_GPL(__reset_control_get);

-- 
With Best Regards,
Andy Shevchenko