device property: Allow error pointer to be passed to fwnode APIs

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

drivers/base/property.c +0 -0

index c0e94cce9c29..1922818470b0 100644
@@ -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>

drivers/base/property.c +2 -1

@@ -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;

drivers/base/property.c +5 -0

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

drivers/base/property.c +0 -0

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

drivers/base/property.c +0 -0

@@ -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,

Quoted 16 lines · 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 to be passed to the fwnode APIs.

drivers/base/property.c int fwnode_property_read_string_array(const struct fwnode_handle *fwnode,

Quoted 8 lines

ret = fwnode_call_int_op(fwnode, property_read_string_array,
propname,

                                 val, nval);

drivers/base/property.c +1 -2

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

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

Quoted 2 lines

From: Andy Shevchenko <[email protected]>
Sent: Friday, March 4, 2022 6:33 PM

Quoted 18 lines

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.

Quoted 5 lines

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