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 to be passed to the fwnode APIs.
Fixes: 83b34afb6b79 ("device property: Introduce fwnode_find_reference()")
Reported-by: Nuno Sá <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
v2: adjusted the entire fwnode API (Sakari)
Nuno, can you test this with the ltc2983 series, including the IS_ERR_OR_NULL()
fix to it?
drivers/base/property.c | 45 +++++++++++++++++++++++++----------------
include/linux/fwnode.h | 10 ++++-----
2 files changed, 33 insertions(+), 22 deletions(-)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index c0e94cce9c29..1922818470b0 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>
#include <linux/kernel.h>
#include <linux/of.h>
#include <linux/of_address.h>
@@ -45,14 +46,16 @@ EXPORT_SYMBOL_GPL(device_property_present);
bool fwnode_property_present(const struct fwnode_handle *fwnode,
const char *propname)
{
- 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 (fwnode_call_bool_op(fwnode, property_present, propname))
+ return true;
+
+ if (IS_ERR_OR_NULL(fwnode->secondary))
+ return false;
+
+ return fwnode_call_bool_op(fwnode->secondary, property_present, propname);
}
EXPORT_SYMBOL_GPL(fwnode_property_present);
@@ -232,10 +235,12 @@ 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))
+ if (ret == -EINVAL && !IS_ERR_OR_NULL(fwnode->secondary))
ret = fwnode_call_int_op(
fwnode->secondary, property_read_int_array, propname,
elem_size, val, nval);
@@ -371,10 +376,12 @@ 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))
+ if (ret == -EINVAL && !IS_ERR_OR_NULL(fwnode->secondary))
ret = fwnode_call_int_op(fwnode->secondary,
property_read_string_array, propname,
val, nval);
@@ -480,11 +487,12 @@ 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 && !IS_ERR_OR_NULL(fwnode) &&
- !IS_ERR_OR_NULL(fwnode->secondary))
+ if (ret < 0 && !IS_ERR_OR_NULL(fwnode->secondary))
ret = fwnode_call_int_op(fwnode->secondary, get_reference_args,
prop, nargs_prop, nargs, index, args);
@@ -798,6 +806,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,11 +999,11 @@ 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 (IS_ERR_OR_NULL(ep) &&
- !IS_ERR_OR_NULL(parent) && !IS_ERR_OR_NULL(parent->secondary))
+ if (!ep && !IS_ERR_OR_NULL(parent->secondary))
ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL);
return ep;
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
On Fri, 2022-03-04 at 19:32 +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 to be passed to the fwnode APIs.
>
> Fixes: 83b34afb6b79 ("device property: Introduce
> fwnode_find_reference()")
> Reported-by: Nuno Sá <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
>
> v2: adjusted the entire fwnode API (Sakari)
>
> Nuno, can you test this with the ltc2983 series, including the
> IS_ERR_OR_NULL()
> fix to it?
>
Sure... I will test that series together with this patch.
> drivers/base/property.c | 45 +++++++++++++++++++++++++--------------
> --
> include/linux/fwnode.h | 10 ++++-----
> 2 files changed, 33 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index c0e94cce9c29..1922818470b0 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>
> #include <linux/kernel.h>
> #include <linux/of.h>
> #include <linux/of_address.h>
> @@ -45,14 +46,16 @@ EXPORT_SYMBOL_GPL(device_property_present);
> bool fwnode_property_present(const struct fwnode_handle *fwnode,
> const char *propname)
> {
> - 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 (fwnode_call_bool_op(fwnode, property_present, propname))
> + return true;
> +
> + if (IS_ERR_OR_NULL(fwnode->secondary))
> + return false;
> +
> + return fwnode_call_bool_op(fwnode->secondary,
> property_present, propname);
> }
> EXPORT_SYMBOL_GPL(fwnode_property_present);
>
> @@ -232,10 +235,12 @@ 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))
> + if (ret == -EINVAL && !IS_ERR_OR_NULL(fwnode->secondary))
> ret = fwnode_call_int_op(
> fwnode->secondary, property_read_int_array,
> propname,
> elem_size, val, nval);
> @@ -371,10 +376,12 @@ 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))
> + if (ret == -EINVAL && !IS_ERR_OR_NULL(fwnode->secondary))
> ret = fwnode_call_int_op(fwnode->secondary,
> property_read_string_array,
Isn't !IS_ERR_OR_NULL(fwnode->secondary)) redundant? AFAIU,
fwnode_call_int_op() will already check the fwnode and only call the op
if the pointer is valid...
- Nuno Sá
On Sat, Mar 5, 2022 at 10:28 PM Nuno Sá <[email protected]> wrote:
>
> On Fri, 2022-03-04 at 19:32 +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 to be passed to the fwnode APIs.
...
> > 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))
> > + if (ret == -EINVAL && !IS_ERR_OR_NULL(fwnode->secondary))
> > ret = fwnode_call_int_op(fwnode->secondary,
> > property_read_string_array,
>
> Isn't !IS_ERR_OR_NULL(fwnode->secondary)) redundant? AFAIU,
> fwnode_call_int_op() will already check the fwnode and only call the op
> if the pointer is valid...
It will shadow the error code, but it seems currently it's the same error code.
So, the question here is if we hide something important with that change.
I dunno what is the best approach here (esp. taking into account that
this is a fix), but ideally we should open code those macros to avoid
double test for fwnode being valid. Because it seems that validation
of fwnode and validation of the operation of the fwnode are orthogonal
and here we mix them. I made this way for the sake of easier
backporting and kicking off a discussion (as you already did). TL;DR:
I think the introduction of the macros was a controversial move, for
which I see pros and cons.
--
With Best Regards,
Andy Shevchenko
> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Friday, March 4, 2022 6:33 PM
> To: Andy Shevchenko <[email protected]>; Rafael J.
> Wysocki <[email protected]>; [email protected];
> [email protected]
> Cc: Daniel Scally <[email protected]>; Heikki Krogerus
> <[email protected]>; Sakari Ailus
> <[email protected]>; Greg Kroah-Hartman
> <[email protected]>; Rafael J. Wysocki
> <[email protected]>; Len Brown <[email protected]>; Sa, Nuno
> <[email protected]>
> Subject: [PATCH v2 1/1] device property: Allow error pointer to be
> passed to fwnode APIs
>
> [External]
>
> 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 to be passed to the fwnode APIs.
>
> Fixes: 83b34afb6b79 ("device property: Introduce
> fwnode_find_reference()")
> Reported-by: Nuno Sá <[email protected]>
> Signed-off-by: Andy Shevchenko
> <[email protected]>
> ---
>
> v2: adjusted the entire fwnode API (Sakari)
>
> Nuno, can you test this with the ltc2983 series, including the
> IS_ERR_OR_NULL()
> fix to it?
Hi Andy,
Just tested this patch with the ltc2983 series and now
fwnode_handle_put() does not crash when fwnode is an
error pointer. I think this usecase does not cover all
of the patch so I'm not sure if a tested by tag here is
meaningful... If it is, go ahead:
Tested-by: Nuno Sá <[email protected]>
- Nuno Sá
On Mon, Mar 07, 2022 at 04:15:23PM +0000, Sa, Nuno wrote:
> > From: Andy Shevchenko <[email protected]>
> > Sent: Friday, March 4, 2022 6:33 PM
...
> > 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 to be passed to the fwnode APIs.
...
> > v2: adjusted the entire fwnode API (Sakari)
> >
> > Nuno, can you test this with the ltc2983 series, including the
> > IS_ERR_OR_NULL()
> > fix to it?
>
> Hi Andy,
>
> Just tested this patch with the ltc2983 series and now
> fwnode_handle_put() does not crash when fwnode is an
> error pointer. I think this usecase does not cover all
> of the patch so I'm not sure if a tested by tag here is
> meaningful...
I believe it still makes sense because we understand what you have tested.
And at least it has some kind of BAT:
- compile testing
- testing (some of the) branches
> If it is, go ahead:
>
> Tested-by: Nuno S? <[email protected]>
Thank you!
I'll send v3 because I want to amend the commit message.
--
With Best Regards,
Andy Shevchenko