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@xxxxxxxxxx>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
> 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
