All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 1/1] device property: Allow error pointer to be passed to fwnode APIs
@ 2022-03-08 12:37 Andy Shevchenko
  2022-03-08 14:36 ` Heikki Krogerus
  2022-03-14 19:51 ` Michael Walle
  0 siblings, 2 replies; 7+ messages in thread
From: Andy Shevchenko @ 2022-03-08 12:37 UTC (permalink / raw)
  To: Andy Shevchenko, linux-acpi, linux-kernel
  Cc: Daniel Scally, Heikki Krogerus, Sakari Ailus, Greg Kroah-Hartman,
	Rafael J. Wysocki, Len Brown, Nuno Sá

Some of the fwnode APIs might return an error pointer instead of NULL
or valid fwnode handle. The result of such API call may be considered
optional and hence the test for it is usually done in a form of

	fwnode = fwnode_find_reference(...);
	if (IS_ERR(fwnode))
		...error handling...

Nevertheless the resulting fwnode may have bumped the reference count
and hence caller of the above API is obliged to call fwnode_handle_put().
Since fwnode may be not valid either as NULL or error pointer the check
has to be performed there. This approach uglifies the code and adds
a point of making a mistake, i.e. forgetting about error point case.

To prevent this, allow an error pointer to be passed to the fwnode APIs.

Fixes: 83b34afb6b79 ("device property: Introduce fwnode_find_reference()")
Reported-by: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Nuno Sá <nuno.sa@analog.com>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Nuno Sá <nuno.sa@analog.com>
---
v4: dropped unneeded fwnode.h and ' == true' part (Sakari), added tag (Sakari)
 drivers/base/property.c | 75 +++++++++++++++++++++++------------------
 include/linux/fwnode.h  | 10 +++---
 2 files changed, 47 insertions(+), 38 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index c0e94cce9c29..4fbdb14cdec4 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -47,12 +47,14 @@ bool fwnode_property_present(const struct fwnode_handle *fwnode,
 {
 	bool ret;
 
+	if (IS_ERR_OR_NULL(fwnode))
+		return false;
+
 	ret = fwnode_call_bool_op(fwnode, property_present, propname);
-	if (ret == false && !IS_ERR_OR_NULL(fwnode) &&
-	    !IS_ERR_OR_NULL(fwnode->secondary))
-		ret = fwnode_call_bool_op(fwnode->secondary, property_present,
-					 propname);
-	return ret;
+	if (ret)
+		return ret;
+
+	return fwnode_call_bool_op(fwnode->secondary, property_present, propname);
 }
 EXPORT_SYMBOL_GPL(fwnode_property_present);
 
@@ -232,15 +234,16 @@ static int fwnode_property_read_int_array(const struct fwnode_handle *fwnode,
 {
 	int ret;
 
+	if (IS_ERR_OR_NULL(fwnode))
+		return -EINVAL;
+
 	ret = fwnode_call_int_op(fwnode, property_read_int_array, propname,
 				 elem_size, val, nval);
-	if (ret == -EINVAL && !IS_ERR_OR_NULL(fwnode) &&
-	    !IS_ERR_OR_NULL(fwnode->secondary))
-		ret = fwnode_call_int_op(
-			fwnode->secondary, property_read_int_array, propname,
-			elem_size, val, nval);
+	if (ret != -EINVAL)
+		return ret;
 
-	return ret;
+	return fwnode_call_int_op(fwnode->secondary, property_read_int_array, propname,
+				  elem_size, val, nval);
 }
 
 /**
@@ -371,14 +374,16 @@ int fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
 {
 	int ret;
 
+	if (IS_ERR_OR_NULL(fwnode))
+		return -EINVAL;
+
 	ret = fwnode_call_int_op(fwnode, property_read_string_array, propname,
 				 val, nval);
-	if (ret == -EINVAL && !IS_ERR_OR_NULL(fwnode) &&
-	    !IS_ERR_OR_NULL(fwnode->secondary))
-		ret = fwnode_call_int_op(fwnode->secondary,
-					 property_read_string_array, propname,
-					 val, nval);
-	return ret;
+	if (ret != -EINVAL)
+		return ret;
+
+	return fwnode_call_int_op(fwnode->secondary, property_read_string_array, propname,
+				  val, nval);
 }
 EXPORT_SYMBOL_GPL(fwnode_property_read_string_array);
 
@@ -480,15 +485,16 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
 {
 	int ret;
 
+	if (IS_ERR_OR_NULL(fwnode))
+		return -ENOENT;
+
 	ret = fwnode_call_int_op(fwnode, get_reference_args, prop, nargs_prop,
 				 nargs, index, args);
+	if (ret == 0)
+		return ret;
 
-	if (ret < 0 && !IS_ERR_OR_NULL(fwnode) &&
-	    !IS_ERR_OR_NULL(fwnode->secondary))
-		ret = fwnode_call_int_op(fwnode->secondary, get_reference_args,
-					 prop, nargs_prop, nargs, index, args);
-
-	return ret;
+	return fwnode_call_int_op(fwnode->secondary, get_reference_args, prop, nargs_prop,
+				  nargs, index, args);
 }
 EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args);
 
@@ -698,7 +704,7 @@ fwnode_get_next_available_child_node(const struct fwnode_handle *fwnode,
 {
 	struct fwnode_handle *next_child = child;
 
-	if (!fwnode)
+	if (IS_ERR_OR_NULL(fwnode))
 		return NULL;
 
 	do {
@@ -722,16 +728,16 @@ struct fwnode_handle *device_get_next_child_node(struct device *dev,
 	const struct fwnode_handle *fwnode = dev_fwnode(dev);
 	struct fwnode_handle *next;
 
+	if (IS_ERR_OR_NULL(fwnode))
+		return NULL;
+
 	/* Try to find a child in primary fwnode */
 	next = fwnode_get_next_child_node(fwnode, child);
 	if (next)
 		return next;
 
 	/* When no more children in primary, continue with secondary */
-	if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))
-		next = fwnode_get_next_child_node(fwnode->secondary, child);
-
-	return next;
+	return fwnode_get_next_child_node(fwnode->secondary, child);
 }
 EXPORT_SYMBOL_GPL(device_get_next_child_node);
 
@@ -798,6 +804,9 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put);
  */
 bool fwnode_device_is_available(const struct fwnode_handle *fwnode)
 {
+	if (IS_ERR_OR_NULL(fwnode))
+		return false;
+
 	if (!fwnode_has_op(fwnode, device_is_available))
 		return true;
 
@@ -988,14 +997,14 @@ fwnode_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
 		parent = fwnode_graph_get_port_parent(prev);
 	else
 		parent = fwnode;
+	if (IS_ERR_OR_NULL(parent))
+		return NULL;
 
 	ep = fwnode_call_ptr_op(parent, graph_get_next_endpoint, prev);
+	if (ep)
+		return ep;
 
-	if (IS_ERR_OR_NULL(ep) &&
-	    !IS_ERR_OR_NULL(parent) && !IS_ERR_OR_NULL(parent->secondary))
-		ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL);
-
-	return ep;
+	return fwnode_graph_get_next_endpoint(parent->secondary, NULL);
 }
 EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint);
 
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 3a532ba66f6c..7defac04f9a3 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -148,12 +148,12 @@ struct fwnode_operations {
 	int (*add_links)(struct fwnode_handle *fwnode);
 };
 
-#define fwnode_has_op(fwnode, op)				\
-	((fwnode) && (fwnode)->ops && (fwnode)->ops->op)
+#define fwnode_has_op(fwnode, op)					\
+	(!IS_ERR_OR_NULL(fwnode) && (fwnode)->ops && (fwnode)->ops->op)
+
 #define fwnode_call_int_op(fwnode, op, ...)				\
-	(fwnode ? (fwnode_has_op(fwnode, op) ?				\
-		   (fwnode)->ops->op(fwnode, ## __VA_ARGS__) : -ENXIO) : \
-	 -EINVAL)
+	(fwnode_has_op(fwnode, op) ?					\
+	 (fwnode)->ops->op(fwnode, ## __VA_ARGS__) : (IS_ERR_OR_NULL(fwnode) ? -EINVAL : -ENXIO))
 
 #define fwnode_call_bool_op(fwnode, op, ...)		\
 	(fwnode_has_op(fwnode, op) ?			\
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 1/1] device property: Allow error pointer to be passed to fwnode APIs
  2022-03-08 12:37 [PATCH v4 1/1] device property: Allow error pointer to be passed to fwnode APIs Andy Shevchenko
@ 2022-03-08 14:36 ` Heikki Krogerus
  2022-03-08 19:03   ` Rafael J. Wysocki
  2022-03-14 19:51 ` Michael Walle
  1 sibling, 1 reply; 7+ messages in thread
From: Heikki Krogerus @ 2022-03-08 14:36 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-acpi, linux-kernel, Daniel Scally, Sakari Ailus,
	Greg Kroah-Hartman, Rafael J. Wysocki, Len Brown, Nuno Sá

On Tue, Mar 08, 2022 at 02:37:12PM +0200, Andy Shevchenko wrote:
> Some of the fwnode APIs might return an error pointer instead of NULL
> or valid fwnode handle. The result of such API call may be considered
> optional and hence the test for it is usually done in a form of
> 
> 	fwnode = fwnode_find_reference(...);
> 	if (IS_ERR(fwnode))
> 		...error handling...
> 
> Nevertheless the resulting fwnode may have bumped the reference count
> and hence caller of the above API is obliged to call fwnode_handle_put().
> Since fwnode may be not valid either as NULL or error pointer the check
> has to be performed there. This approach uglifies the code and adds
> a point of making a mistake, i.e. forgetting about error point case.
> 
> To prevent this, allow an error pointer to be passed to the fwnode APIs.
> 
> Fixes: 83b34afb6b79 ("device property: Introduce fwnode_find_reference()")
> Reported-by: Nuno Sá <nuno.sa@analog.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Tested-by: Nuno Sá <nuno.sa@analog.com>
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Nuno Sá <nuno.sa@analog.com>

This is nice! FWIW:

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> v4: dropped unneeded fwnode.h and ' == true' part (Sakari), added tag (Sakari)
>  drivers/base/property.c | 75 +++++++++++++++++++++++------------------
>  include/linux/fwnode.h  | 10 +++---
>  2 files changed, 47 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index c0e94cce9c29..4fbdb14cdec4 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -47,12 +47,14 @@ bool fwnode_property_present(const struct fwnode_handle *fwnode,
>  {
>  	bool ret;
>  
> +	if (IS_ERR_OR_NULL(fwnode))
> +		return false;
> +
>  	ret = fwnode_call_bool_op(fwnode, property_present, propname);
> -	if (ret == false && !IS_ERR_OR_NULL(fwnode) &&
> -	    !IS_ERR_OR_NULL(fwnode->secondary))
> -		ret = fwnode_call_bool_op(fwnode->secondary, property_present,
> -					 propname);
> -	return ret;
> +	if (ret)
> +		return ret;
> +
> +	return fwnode_call_bool_op(fwnode->secondary, property_present, propname);
>  }
>  EXPORT_SYMBOL_GPL(fwnode_property_present);
>  
> @@ -232,15 +234,16 @@ static int fwnode_property_read_int_array(const struct fwnode_handle *fwnode,
>  {
>  	int ret;
>  
> +	if (IS_ERR_OR_NULL(fwnode))
> +		return -EINVAL;
> +
>  	ret = fwnode_call_int_op(fwnode, property_read_int_array, propname,
>  				 elem_size, val, nval);
> -	if (ret == -EINVAL && !IS_ERR_OR_NULL(fwnode) &&
> -	    !IS_ERR_OR_NULL(fwnode->secondary))
> -		ret = fwnode_call_int_op(
> -			fwnode->secondary, property_read_int_array, propname,
> -			elem_size, val, nval);
> +	if (ret != -EINVAL)
> +		return ret;
>  
> -	return ret;
> +	return fwnode_call_int_op(fwnode->secondary, property_read_int_array, propname,
> +				  elem_size, val, nval);
>  }
>  
>  /**
> @@ -371,14 +374,16 @@ int fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
>  {
>  	int ret;
>  
> +	if (IS_ERR_OR_NULL(fwnode))
> +		return -EINVAL;
> +
>  	ret = fwnode_call_int_op(fwnode, property_read_string_array, propname,
>  				 val, nval);
> -	if (ret == -EINVAL && !IS_ERR_OR_NULL(fwnode) &&
> -	    !IS_ERR_OR_NULL(fwnode->secondary))
> -		ret = fwnode_call_int_op(fwnode->secondary,
> -					 property_read_string_array, propname,
> -					 val, nval);
> -	return ret;
> +	if (ret != -EINVAL)
> +		return ret;
> +
> +	return fwnode_call_int_op(fwnode->secondary, property_read_string_array, propname,
> +				  val, nval);
>  }
>  EXPORT_SYMBOL_GPL(fwnode_property_read_string_array);
>  
> @@ -480,15 +485,16 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
>  {
>  	int ret;
>  
> +	if (IS_ERR_OR_NULL(fwnode))
> +		return -ENOENT;
> +
>  	ret = fwnode_call_int_op(fwnode, get_reference_args, prop, nargs_prop,
>  				 nargs, index, args);
> +	if (ret == 0)
> +		return ret;
>  
> -	if (ret < 0 && !IS_ERR_OR_NULL(fwnode) &&
> -	    !IS_ERR_OR_NULL(fwnode->secondary))
> -		ret = fwnode_call_int_op(fwnode->secondary, get_reference_args,
> -					 prop, nargs_prop, nargs, index, args);
> -
> -	return ret;
> +	return fwnode_call_int_op(fwnode->secondary, get_reference_args, prop, nargs_prop,
> +				  nargs, index, args);
>  }
>  EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args);
>  
> @@ -698,7 +704,7 @@ fwnode_get_next_available_child_node(const struct fwnode_handle *fwnode,
>  {
>  	struct fwnode_handle *next_child = child;
>  
> -	if (!fwnode)
> +	if (IS_ERR_OR_NULL(fwnode))
>  		return NULL;
>  
>  	do {
> @@ -722,16 +728,16 @@ struct fwnode_handle *device_get_next_child_node(struct device *dev,
>  	const struct fwnode_handle *fwnode = dev_fwnode(dev);
>  	struct fwnode_handle *next;
>  
> +	if (IS_ERR_OR_NULL(fwnode))
> +		return NULL;
> +
>  	/* Try to find a child in primary fwnode */
>  	next = fwnode_get_next_child_node(fwnode, child);
>  	if (next)
>  		return next;
>  
>  	/* When no more children in primary, continue with secondary */
> -	if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))
> -		next = fwnode_get_next_child_node(fwnode->secondary, child);
> -
> -	return next;
> +	return fwnode_get_next_child_node(fwnode->secondary, child);
>  }
>  EXPORT_SYMBOL_GPL(device_get_next_child_node);
>  
> @@ -798,6 +804,9 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put);
>   */
>  bool fwnode_device_is_available(const struct fwnode_handle *fwnode)
>  {
> +	if (IS_ERR_OR_NULL(fwnode))
> +		return false;
> +
>  	if (!fwnode_has_op(fwnode, device_is_available))
>  		return true;
>  
> @@ -988,14 +997,14 @@ fwnode_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
>  		parent = fwnode_graph_get_port_parent(prev);
>  	else
>  		parent = fwnode;
> +	if (IS_ERR_OR_NULL(parent))
> +		return NULL;
>  
>  	ep = fwnode_call_ptr_op(parent, graph_get_next_endpoint, prev);
> +	if (ep)
> +		return ep;
>  
> -	if (IS_ERR_OR_NULL(ep) &&
> -	    !IS_ERR_OR_NULL(parent) && !IS_ERR_OR_NULL(parent->secondary))
> -		ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL);
> -
> -	return ep;
> +	return fwnode_graph_get_next_endpoint(parent->secondary, NULL);
>  }
>  EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint);
>  
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 3a532ba66f6c..7defac04f9a3 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -148,12 +148,12 @@ struct fwnode_operations {
>  	int (*add_links)(struct fwnode_handle *fwnode);
>  };
>  
> -#define fwnode_has_op(fwnode, op)				\
> -	((fwnode) && (fwnode)->ops && (fwnode)->ops->op)
> +#define fwnode_has_op(fwnode, op)					\
> +	(!IS_ERR_OR_NULL(fwnode) && (fwnode)->ops && (fwnode)->ops->op)
> +
>  #define fwnode_call_int_op(fwnode, op, ...)				\
> -	(fwnode ? (fwnode_has_op(fwnode, op) ?				\
> -		   (fwnode)->ops->op(fwnode, ## __VA_ARGS__) : -ENXIO) : \
> -	 -EINVAL)
> +	(fwnode_has_op(fwnode, op) ?					\
> +	 (fwnode)->ops->op(fwnode, ## __VA_ARGS__) : (IS_ERR_OR_NULL(fwnode) ? -EINVAL : -ENXIO))
>  
>  #define fwnode_call_bool_op(fwnode, op, ...)		\
>  	(fwnode_has_op(fwnode, op) ?			\
> -- 
> 2.34.1

-- 
heikki

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 1/1] device property: Allow error pointer to be passed to fwnode APIs
  2022-03-08 14:36 ` Heikki Krogerus
@ 2022-03-08 19:03   ` Rafael J. Wysocki
  0 siblings, 0 replies; 7+ messages in thread
From: Rafael J. Wysocki @ 2022-03-08 19:03 UTC (permalink / raw)
  To: Heikki Krogerus, Andy Shevchenko
  Cc: ACPI Devel Maling List, Linux Kernel Mailing List, Daniel Scally,
	Sakari Ailus, Greg Kroah-Hartman, Rafael J. Wysocki, Len Brown,
	Nuno Sá

On Tue, Mar 8, 2022 at 3:36 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Tue, Mar 08, 2022 at 02:37:12PM +0200, Andy Shevchenko wrote:
> > Some of the fwnode APIs might return an error pointer instead of NULL
> > or valid fwnode handle. The result of such API call may be considered
> > optional and hence the test for it is usually done in a form of
> >
> >       fwnode = fwnode_find_reference(...);
> >       if (IS_ERR(fwnode))
> >               ...error handling...
> >
> > Nevertheless the resulting fwnode may have bumped the reference count
> > and hence caller of the above API is obliged to call fwnode_handle_put().
> > Since fwnode may be not valid either as NULL or error pointer the check
> > has to be performed there. This approach uglifies the code and adds
> > a point of making a mistake, i.e. forgetting about error point case.
> >
> > To prevent this, allow an error pointer to be passed to the fwnode APIs.
> >
> > Fixes: 83b34afb6b79 ("device property: Introduce fwnode_find_reference()")
> > Reported-by: Nuno Sá <nuno.sa@analog.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Tested-by: Nuno Sá <nuno.sa@analog.com>
> > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Acked-by: Nuno Sá <nuno.sa@analog.com>
>
> This is nice! FWIW:
>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Applied as 5.18 material, thanks!

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 1/1] device property: Allow error pointer to be passed to fwnode APIs
  2022-03-08 12:37 [PATCH v4 1/1] device property: Allow error pointer to be passed to fwnode APIs Andy Shevchenko
  2022-03-08 14:36 ` Heikki Krogerus
@ 2022-03-14 19:51 ` Michael Walle
  2022-03-15 11:16   ` Rafael J. Wysocki
  2022-04-06 13:35   ` Andy Shevchenko
  1 sibling, 2 replies; 7+ messages in thread
From: Michael Walle @ 2022-03-14 19:51 UTC (permalink / raw)
  To: andriy.shevchenko
  Cc: djrscally, gregkh, heikki.krogerus, lenb, linux-acpi,
	linux-kernel, nuno.sa, rafael, sakari.ailus, Russell King,
	Horatiu Vultur, Michael Walle

Hi Andy,

> Some of the fwnode APIs might return an error pointer instead of NULL
> or valid fwnode handle. The result of such API call may be considered
> optional and hence the test for it is usually done in a form of
> 
> 	fwnode = fwnode_find_reference(...);
> 	if (IS_ERR(fwnode))
> 		...error handling...
> 
> Nevertheless the resulting fwnode may have bumped the reference count
> and hence caller of the above API is obliged to call fwnode_handle_put().
> Since fwnode may be not valid either as NULL or error pointer the check
> has to be performed there. This approach uglifies the code and adds
> a point of making a mistake, i.e. forgetting about error point case.
> 
> To prevent this, allow an error pointer to be passed to the fwnode APIs.
> 
> Fixes: 83b34afb6b79 ("device property: Introduce fwnode_find_reference()")
> Reported-by: Nuno Sá <nuno.sa@analog.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Tested-by: Nuno Sá <nuno.sa@analog.com>
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Nuno Sá <nuno.sa@analog.com>

This breaks SFP/phylink (using the lan966x switch) on my board. See below
for more details.

[..]

> @@ -480,15 +485,16 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
>  {
>  	int ret;
>  
> +	if (IS_ERR_OR_NULL(fwnode))
> +		return -ENOENT;
> +
>  	ret = fwnode_call_int_op(fwnode, get_reference_args, prop, nargs_prop,
>  				 nargs, index, args);
> +	if (ret == 0)

Should this be "if (ret == 0 || IS_ERR_OR_NULL(fwnode->secondary))" ?

> +		return ret;
>  
> -	if (ret < 0 && !IS_ERR_OR_NULL(fwnode) &&
> -	    !IS_ERR_OR_NULL(fwnode->secondary))
> -		ret = fwnode_call_int_op(fwnode->secondary, get_reference_args,
> -					 prop, nargs_prop, nargs, index, args);
> -
> -	return ret;
> +	return fwnode_call_int_op(fwnode->secondary, get_reference_args, prop, nargs_prop,
> +				  nargs, index, args);
>  }
>  EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args);

What happens before this patch is that sfp_bus_find_fwnode() will call
fwnode_property_get_reference_args() and the first calls return -ENOENT
which sfp_bus_find_fwnode() will handle in a special way. After your
patch, -EINVAL is returned, because fwnode_call_int_op() on
fwnode->secondary is always called regardless of the return value of
the original fwnode.

-michael

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 1/1] device property: Allow error pointer to be passed to fwnode APIs
  2022-03-14 19:51 ` Michael Walle
@ 2022-03-15 11:16   ` Rafael J. Wysocki
  2022-03-15 13:27     ` Andy Shevchenko
  2022-04-06 13:35   ` Andy Shevchenko
  1 sibling, 1 reply; 7+ messages in thread
From: Rafael J. Wysocki @ 2022-03-15 11:16 UTC (permalink / raw)
  To: Michael Walle
  Cc: Andy Shevchenko, Daniel Scally, Greg Kroah-Hartman,
	Heikki Krogerus, Len Brown, ACPI Devel Maling List,
	Linux Kernel Mailing List, Nuno Sá,
	Rafael J. Wysocki, Sakari Ailus, Russell King, Horatiu Vultur

On Mon, Mar 14, 2022 at 8:51 PM Michael Walle <michael@walle.cc> wrote:
>
> Hi Andy,
>
> > Some of the fwnode APIs might return an error pointer instead of NULL
> > or valid fwnode handle. The result of such API call may be considered
> > optional and hence the test for it is usually done in a form of
> >
> >       fwnode = fwnode_find_reference(...);
> >       if (IS_ERR(fwnode))
> >               ...error handling...
> >
> > Nevertheless the resulting fwnode may have bumped the reference count
> > and hence caller of the above API is obliged to call fwnode_handle_put().
> > Since fwnode may be not valid either as NULL or error pointer the check
> > has to be performed there. This approach uglifies the code and adds
> > a point of making a mistake, i.e. forgetting about error point case.
> >
> > To prevent this, allow an error pointer to be passed to the fwnode APIs.
> >
> > Fixes: 83b34afb6b79 ("device property: Introduce fwnode_find_reference()")
> > Reported-by: Nuno Sá <nuno.sa@analog.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Tested-by: Nuno Sá <nuno.sa@analog.com>
> > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Acked-by: Nuno Sá <nuno.sa@analog.com>
>
> This breaks SFP/phylink (using the lan966x switch) on my board. See below
> for more details.

I'm dropping this commit for the time being.

> [..]
>
> > @@ -480,15 +485,16 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
> >  {
> >       int ret;
> >
> > +     if (IS_ERR_OR_NULL(fwnode))
> > +             return -ENOENT;
> > +
> >       ret = fwnode_call_int_op(fwnode, get_reference_args, prop, nargs_prop,
> >                                nargs, index, args);
> > +     if (ret == 0)
>
> Should this be "if (ret == 0 || IS_ERR_OR_NULL(fwnode->secondary))" ?
>
> > +             return ret;
> >
> > -     if (ret < 0 && !IS_ERR_OR_NULL(fwnode) &&
> > -         !IS_ERR_OR_NULL(fwnode->secondary))
> > -             ret = fwnode_call_int_op(fwnode->secondary, get_reference_args,
> > -                                      prop, nargs_prop, nargs, index, args);
> > -
> > -     return ret;
> > +     return fwnode_call_int_op(fwnode->secondary, get_reference_args, prop, nargs_prop,
> > +                               nargs, index, args);
> >  }
> >  EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args);
>
> What happens before this patch is that sfp_bus_find_fwnode() will call
> fwnode_property_get_reference_args() and the first calls return -ENOENT
> which sfp_bus_find_fwnode() will handle in a special way. After your
> patch, -EINVAL is returned, because fwnode_call_int_op() on
> fwnode->secondary is always called regardless of the return value of
> the original fwnode.
>
> -michael

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 1/1] device property: Allow error pointer to be passed to fwnode APIs
  2022-03-15 11:16   ` Rafael J. Wysocki
@ 2022-03-15 13:27     ` Andy Shevchenko
  0 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2022-03-15 13:27 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Michael Walle, Daniel Scally, Greg Kroah-Hartman,
	Heikki Krogerus, Len Brown, ACPI Devel Maling List,
	Linux Kernel Mailing List, Nuno Sá,
	Sakari Ailus, Russell King, Horatiu Vultur

On Tue, Mar 15, 2022 at 12:16:01PM +0100, Rafael J. Wysocki wrote:
> On Mon, Mar 14, 2022 at 8:51 PM Michael Walle <michael@walle.cc> wrote:

...

> > This breaks SFP/phylink (using the lan966x switch) on my board. See below
> > for more details.

Michael, thank you for the report, I'll investigate it further.

> I'm dropping this commit for the time being.

Thanks! Fine with me as I think it's not critical (the current or pending users
of the fwnode_find_reference() are / will be aware of the error pointer).

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 1/1] device property: Allow error pointer to be passed to fwnode APIs
  2022-03-14 19:51 ` Michael Walle
  2022-03-15 11:16   ` Rafael J. Wysocki
@ 2022-04-06 13:35   ` Andy Shevchenko
  1 sibling, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2022-04-06 13:35 UTC (permalink / raw)
  To: Michael Walle
  Cc: djrscally, gregkh, heikki.krogerus, lenb, linux-acpi,
	linux-kernel, nuno.sa, rafael, sakari.ailus, Russell King,
	Horatiu Vultur

On Mon, Mar 14, 2022 at 08:51:37PM +0100, Michael Walle wrote:

> This breaks SFP/phylink (using the lan966x switch) on my board. See below
> for more details.

I have just sent v5 (it's now a series of patches where the first one is
the fixed version of this patch), please (re-)test.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-04-06 16:03 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-08 12:37 [PATCH v4 1/1] device property: Allow error pointer to be passed to fwnode APIs Andy Shevchenko
2022-03-08 14:36 ` Heikki Krogerus
2022-03-08 19:03   ` Rafael J. Wysocki
2022-03-14 19:51 ` Michael Walle
2022-03-15 11:16   ` Rafael J. Wysocki
2022-03-15 13:27     ` Andy Shevchenko
2022-04-06 13:35   ` Andy Shevchenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.