Re: [PATCH v1 1/1] device property: Allow error pointer for fwnode_handle_{get,put}()

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Andy,

On Thu, Mar 03, 2022 at 05:06:10PM +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_OR_NULL(fwnode))
> 		...error handling...
> 
> Nevertheless the resulting fwnode may have bumped 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 error pointer for fwnode_handle_get() and
> fwnode_handle_put().
> 
> 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>
> ---
>  drivers/base/property.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 2d70392fc982..df7b8c7ad264 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -776,7 +776,7 @@ EXPORT_SYMBOL_GPL(device_get_named_child_node);
>   */
>  struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode)
>  {
> -	if (!fwnode_has_op(fwnode, get))
> +	if (IS_ERR(fwnode) || !fwnode_has_op(fwnode, get))
>  		return fwnode;
>  
>  	return fwnode_call_ptr_op(fwnode, get);
> @@ -793,6 +793,9 @@ EXPORT_SYMBOL_GPL(fwnode_handle_get);
>   */
>  void fwnode_handle_put(struct fwnode_handle *fwnode)
>  {
> +	if (IS_ERR(fwnode) || !fwnode_has_op(fwnode, put))
> +		return;
> +
>  	fwnode_call_void_op(fwnode, put);
>  }
>  EXPORT_SYMBOL_GPL(fwnode_handle_put);

I guess fwnode_find_reference() is the only fwnode API function returning
errors as pointers? If you changed it returning NULL on error, you'd lose
the error codes.

But I think this is a problem beyond fwnode_handle_{get,put}: fwnode
obtained this way could be passed to any fwnode function and they should
just work correctly with that.

How about moving the check to fwnode_has_op()? That function is responsible
for checking the fwnode is valid and has the op in question.

It also seems the explicit fwnode_has_op() call is redundant in
fwnode_handle_put() as fwnode_call_void_op() already calls fwnode_has_op().

-- 
Kind regards,

Sakari Ailus