Hi Andy,
This makes secondary handling quite a big nicer, thanks! A few comments
below. Apart from that,
Reviewed-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
On Mon, Mar 07, 2022 at 10:29:49PM +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@xxxxxxxxxx>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Tested-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> ---
>
> v3: dropped test of secondary fwnode (Nuno), added tag (Nuno), amended commit message
> v2: adjusted the entire fwnode API (Sakari)
>
> Nuno, can you re-test this with the ltc2983 series to be sure it is still okay?
>
> drivers/base/property.c | 76 +++++++++++++++++++++++------------------
> include/linux/fwnode.h | 10 +++---
> 2 files changed, 48 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index c0e94cce9c29..635a0e556a4f 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -9,6 +9,7 @@
>
> #include <linux/acpi.h>
> #include <linux/export.h>
> +#include <linux/fwnode.h>
Is this intended? linux/property.h already includes linux/fwnode.h.
> #include <linux/kernel.h>
> #include <linux/of.h>
> #include <linux/of_address.h>
> @@ -47,12 +48,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 == true)
It's already bool. I'd instead use:
if (ret)
> + return ret;
> +
> + return fwnode_call_bool_op(fwnode->secondary, property_present, propname);
> }
> EXPORT_SYMBOL_GPL(fwnode_property_present);
>
> @@ -232,15 +235,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 +375,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 +486,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 +705,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 +729,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 +805,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 +998,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) ? \
--
Kind regards,
Sakari Ailus
