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

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

[Andy Shevchenko]

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);
-- 
2.34.1

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

[Sakari Ailus]

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

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

[Andy Shevchenko]

On Thu, Mar 3, 2022 at 11:34 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> 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().

Thanks for the review, my comments below.

...

> 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.

Yes, I was thinking about it (and I even have a local followup), so
this version of the fix is (semi-)RFC. Moreover, we (wrongly!) do
check in many already, but only _after_ trying to dereference error
pointers.

Letme prepare v2 tomorrow.

> 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().

Then call_ptr_op should have the same check as well.

-- 
With Best Regards,
Andy Shevchenko