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á <[email protected]>
Tested-by: Nuno Sá <[email protected]>
Acked-by: Nuno Sá <[email protected]>
Reviewed-by: Sakari Ailus <[email protected]>
Reviewed-by: Heikki Krogerus <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
v5: fixed return value in _get_reference_args() for secondary check (Michael)
drivers/base/property.c | 89 +++++++++++++++++++++++------------------
include/linux/fwnode.h | 10 ++---
2 files changed, 56 insertions(+), 43 deletions(-)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index c0e94cce9c29..3376d3971a93 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -47,12 +47,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)
+ return ret;
+
+ return fwnode_call_bool_op(fwnode->secondary, property_present, propname);
}
EXPORT_SYMBOL_GPL(fwnode_property_present);
@@ -232,15 +234,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 +374,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 +485,19 @@ 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);
+ if (IS_ERR_OR_NULL(fwnode->secondary))
+ return -ENOENT;
- 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);
@@ -635,12 +644,13 @@ EXPORT_SYMBOL_GPL(fwnode_count_parents);
struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwnode,
unsigned int depth)
{
- unsigned int i;
-
fwnode_handle_get(fwnode);
- for (i = 0; i < depth && fwnode; i++)
+ do {
+ if (depth-- == 0)
+ break;
fwnode = fwnode_get_next_parent(fwnode);
+ } while (fwnode);
return fwnode;
}
@@ -659,17 +669,17 @@ EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
bool fwnode_is_ancestor_of(struct fwnode_handle *test_ancestor,
struct fwnode_handle *test_child)
{
- if (!test_ancestor)
+ if (IS_ERR_OR_NULL(test_ancestor))
return false;
fwnode_handle_get(test_child);
- while (test_child) {
+ do {
if (test_child == test_ancestor) {
fwnode_handle_put(test_child);
return true;
}
test_child = fwnode_get_next_parent(test_child);
- }
+ } while (test_child);
return false;
}
@@ -698,7 +708,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 +732,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 +808,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 +1001,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) ? \
--
2.35.1
The part 'is' in the function name implies the test against something.
Drop unnecessary 'test' prefix in the fwnode_is_ancestor_of() parameters.
No functional change intended.
Signed-off-by: Andy Shevchenko <[email protected]>
---
v5: new patch
drivers/base/property.c | 20 +++++++++-----------
include/linux/property.h | 3 +--
2 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index bca9a28ce271..839ee7f3ee12 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -656,28 +656,26 @@ struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwnode,
EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
/**
- * fwnode_is_ancestor_of - Test if @test_ancestor is ancestor of @test_child
- * @test_ancestor: Firmware which is tested for being an ancestor
- * @test_child: Firmware which is tested for being the child
+ * fwnode_is_ancestor_of - Test if @ancestor is ancestor of @child
+ * @ancestor: Firmware which is tested for being an ancestor
+ * @child: Firmware which is tested for being the child
*
* A node is considered an ancestor of itself too.
*
- * Returns true if @test_ancestor is an ancestor of @test_child.
- * Otherwise, returns false.
+ * Returns true if @ancestor is an ancestor of @child. Otherwise, returns false.
*/
-bool fwnode_is_ancestor_of(struct fwnode_handle *test_ancestor,
- struct fwnode_handle *test_child)
+bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle *child)
{
struct fwnode_handle *parent;
- if (IS_ERR_OR_NULL(test_ancestor))
+ if (IS_ERR_OR_NULL(ancestor))
return false;
- if (test_child == test_ancestor)
+ if (child == ancestor)
return true;
- fwnode_for_each_parent_node(test_child, parent) {
- if (parent == test_ancestor) {
+ fwnode_for_each_parent_node(child, parent) {
+ if (parent == ancestor) {
fwnode_handle_put(parent);
return true;
}
diff --git a/include/linux/property.h b/include/linux/property.h
index 15d6863ae962..fc24d45632eb 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -95,8 +95,7 @@ struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode);
unsigned int fwnode_count_parents(const struct fwnode_handle *fwn);
struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwn,
unsigned int depth);
-bool fwnode_is_ancestor_of(struct fwnode_handle *test_ancestor,
- struct fwnode_handle *test_child);
+bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle *child);
struct fwnode_handle *fwnode_get_next_child_node(
const struct fwnode_handle *fwnode, struct fwnode_handle *child);
struct fwnode_handle *fwnode_get_next_available_child_node(
--
2.35.1
Due to fwnode_get_next_parent() used to be called against the parameters
of some of fwnode APIs those parameters have no const qualifier. However,
after switching to fwnode_for_each_parent_node() API now it's possible to
constify the parameters. Do it for good.
The affected functions are:
fwnode_get_next_parent_dev()
fwnode_get_nth_parent()
fwnode_is_ancestor_of()
Signed-off-by: Andy Shevchenko <[email protected]>
---
v5: new patch
drivers/base/property.c | 7 +++----
include/linux/property.h | 9 ++++-----
2 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 839ee7f3ee12..79e0adfa1e24 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -594,7 +594,7 @@ EXPORT_SYMBOL_GPL(fwnode_get_next_parent);
* The caller of this function is expected to call put_device() on the returned
* device when they are done.
*/
-struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode)
+struct device *fwnode_get_next_parent_dev(const struct fwnode_handle *fwnode)
{
struct fwnode_handle *parent;
struct device *dev;
@@ -639,8 +639,7 @@ EXPORT_SYMBOL_GPL(fwnode_count_parents);
* The caller is responsible for calling fwnode_handle_put() for the returned
* node.
*/
-struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwnode,
- unsigned int depth)
+struct fwnode_handle *fwnode_get_nth_parent(const struct fwnode_handle *fwnode, unsigned int depth)
{
struct fwnode_handle *parent;
@@ -664,7 +663,7 @@ EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
*
* Returns true if @ancestor is an ancestor of @child. Otherwise, returns false.
*/
-bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle *child)
+bool fwnode_is_ancestor_of(const struct fwnode_handle *ancestor, const struct fwnode_handle *child)
{
struct fwnode_handle *parent;
diff --git a/include/linux/property.h b/include/linux/property.h
index fc24d45632eb..119bf7d6a02f 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -91,11 +91,10 @@ struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode);
for (parent = fwnode_get_parent(fwnode); parent; \
parent = fwnode_get_next_parent(parent))
-struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode);
-unsigned int fwnode_count_parents(const struct fwnode_handle *fwn);
-struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwn,
- unsigned int depth);
-bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle *child);
+struct device *fwnode_get_next_parent_dev(const struct fwnode_handle *fwnode);
+unsigned int fwnode_count_parents(const struct fwnode_handle *fwnode);
+struct fwnode_handle *fwnode_get_nth_parent(const struct fwnode_handle *fwnode, unsigned int depth);
+bool fwnode_is_ancestor_of(const struct fwnode_handle *ancestor, const struct fwnode_handle *child);
struct fwnode_handle *fwnode_get_next_child_node(
const struct fwnode_handle *fwnode, struct fwnode_handle *child);
struct fwnode_handle *fwnode_get_next_available_child_node(
--
2.35.1
In a few cases the functionality of fwnode_for_each_parent_node()
is already in use. Introduce a common helper macro for it.
It may be used by others as well in the future.
Signed-off-by: Andy Shevchenko <[email protected]>
---
v5: new patch
drivers/base/property.c | 56 +++++++++++++++++++++-------------------
include/linux/property.h | 9 +++++--
2 files changed, 36 insertions(+), 29 deletions(-)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 3376d3971a93..bca9a28ce271 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -596,17 +596,17 @@ EXPORT_SYMBOL_GPL(fwnode_get_next_parent);
*/
struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode)
{
+ struct fwnode_handle *parent;
struct device *dev;
- fwnode_handle_get(fwnode);
- do {
- fwnode = fwnode_get_next_parent(fwnode);
- if (!fwnode)
- return NULL;
+ fwnode_for_each_parent_node(fwnode, parent) {
dev = get_dev_from_fwnode(fwnode);
- } while (!dev);
- fwnode_handle_put(fwnode);
- return dev;
+ if (dev) {
+ fwnode_handle_put(parent);
+ return dev;
+ }
+ }
+ return NULL;
}
/**
@@ -617,13 +617,11 @@ struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode)
*/
unsigned int fwnode_count_parents(const struct fwnode_handle *fwnode)
{
- struct fwnode_handle *__fwnode;
- unsigned int count;
-
- __fwnode = fwnode_get_parent(fwnode);
+ struct fwnode_handle *parent;
+ unsigned int count = 0;
- for (count = 0; __fwnode; count++)
- __fwnode = fwnode_get_next_parent(__fwnode);
+ fwnode_for_each_parent_node(fwnode, parent)
+ count++;
return count;
}
@@ -644,15 +642,16 @@ EXPORT_SYMBOL_GPL(fwnode_count_parents);
struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwnode,
unsigned int depth)
{
- fwnode_handle_get(fwnode);
+ struct fwnode_handle *parent;
- do {
- if (depth-- == 0)
- break;
- fwnode = fwnode_get_next_parent(fwnode);
- } while (fwnode);
+ if (depth == 0)
+ return fwnode_handle_get(fwnode);
- return fwnode;
+ fwnode_for_each_parent_node(fwnode, parent) {
+ if (--depth == 0)
+ return parent;
+ }
+ return NULL;
}
EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
@@ -669,17 +668,20 @@ EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
bool fwnode_is_ancestor_of(struct fwnode_handle *test_ancestor,
struct fwnode_handle *test_child)
{
+ struct fwnode_handle *parent;
+
if (IS_ERR_OR_NULL(test_ancestor))
return false;
- fwnode_handle_get(test_child);
- do {
- if (test_child == test_ancestor) {
- fwnode_handle_put(test_child);
+ if (test_child == test_ancestor)
+ return true;
+
+ fwnode_for_each_parent_node(test_child, parent) {
+ if (parent == test_ancestor) {
+ fwnode_handle_put(parent);
return true;
}
- test_child = fwnode_get_next_parent(test_child);
- } while (test_child);
+ }
return false;
}
diff --git a/include/linux/property.h b/include/linux/property.h
index 4cd4b326941f..15d6863ae962 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -83,9 +83,14 @@ struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode,
const char *fwnode_get_name(const struct fwnode_handle *fwnode);
const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode);
+
struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
-struct fwnode_handle *fwnode_get_next_parent(
- struct fwnode_handle *fwnode);
+struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode);
+
+#define fwnode_for_each_parent_node(fwnode, parent) \
+ for (parent = fwnode_get_parent(fwnode); parent; \
+ parent = fwnode_get_next_parent(parent))
+
struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode);
unsigned int fwnode_count_parents(const struct fwnode_handle *fwn);
struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwn,
--
2.35.1
Hi Andy,
I love your patch! Perhaps something to improve:
[auto build test WARNING on driver-core/driver-core-testing]
[also build test WARNING on rafael-pm/linux-next linus/master linux/master v5.18-rc1 next-20220406]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/device-property-Allow-error-pointer-to-be-passed-to-fwnode-APIs/20220407-002511
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 3123109284176b1532874591f7c81f3837bbdc17
config: powerpc-allnoconfig (https://download.01.org/0day-ci/archive/20220407/[email protected]/config)
compiler: powerpc-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/d9d353ada8d8c3b1b7f3965ad7fe191bd7dea930
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Andy-Shevchenko/device-property-Allow-error-pointer-to-be-passed-to-fwnode-APIs/20220407-002511
git checkout d9d353ada8d8c3b1b7f3965ad7fe191bd7dea930
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/base/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/base/property.c: In function 'fwnode_get_nth_parent':
>> drivers/base/property.c:647:42: warning: passing argument 1 of 'fwnode_handle_get' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
647 | return fwnode_handle_get(fwnode);
| ^~~~~~
In file included from include/linux/of.h:22,
from include/linux/irqdomain.h:35,
from include/linux/acpi.h:13,
from drivers/base/property.c:10:
include/linux/property.h:123:63: note: expected 'struct fwnode_handle *' but argument is of type 'const struct fwnode_handle *'
123 | struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
| ~~~~~~~~~~~~~~~~~~~~~~^~~~~~
vim +647 drivers/base/property.c
87e5e95db31a27 Sakari Ailus 2019-10-03 629
87e5e95db31a27 Sakari Ailus 2019-10-03 630 /**
87e5e95db31a27 Sakari Ailus 2019-10-03 631 * fwnode_get_nth_parent - Return an nth parent of a node
87e5e95db31a27 Sakari Ailus 2019-10-03 632 * @fwnode: The node the parent of which is requested
87e5e95db31a27 Sakari Ailus 2019-10-03 633 * @depth: Distance of the parent from the node
87e5e95db31a27 Sakari Ailus 2019-10-03 634 *
87e5e95db31a27 Sakari Ailus 2019-10-03 635 * Returns the nth parent of a node. If there is no parent at the requested
87e5e95db31a27 Sakari Ailus 2019-10-03 636 * @depth, %NULL is returned. If @depth is 0, the functionality is equivalent to
87e5e95db31a27 Sakari Ailus 2019-10-03 637 * fwnode_handle_get(). For @depth == 1, it is fwnode_get_parent() and so on.
87e5e95db31a27 Sakari Ailus 2019-10-03 638 *
87e5e95db31a27 Sakari Ailus 2019-10-03 639 * The caller is responsible for calling fwnode_handle_put() for the returned
87e5e95db31a27 Sakari Ailus 2019-10-03 640 * node.
87e5e95db31a27 Sakari Ailus 2019-10-03 641 */
d9d353ada8d8c3 Andy Shevchenko 2022-04-06 642 struct fwnode_handle *fwnode_get_nth_parent(const struct fwnode_handle *fwnode, unsigned int depth)
87e5e95db31a27 Sakari Ailus 2019-10-03 643 {
040f806ecab6cd Andy Shevchenko 2022-04-06 644 struct fwnode_handle *parent;
87e5e95db31a27 Sakari Ailus 2019-10-03 645
040f806ecab6cd Andy Shevchenko 2022-04-06 646 if (depth == 0)
040f806ecab6cd Andy Shevchenko 2022-04-06 @647 return fwnode_handle_get(fwnode);
87e5e95db31a27 Sakari Ailus 2019-10-03 648
040f806ecab6cd Andy Shevchenko 2022-04-06 649 fwnode_for_each_parent_node(fwnode, parent) {
040f806ecab6cd Andy Shevchenko 2022-04-06 650 if (--depth == 0)
040f806ecab6cd Andy Shevchenko 2022-04-06 651 return parent;
040f806ecab6cd Andy Shevchenko 2022-04-06 652 }
040f806ecab6cd Andy Shevchenko 2022-04-06 653 return NULL;
87e5e95db31a27 Sakari Ailus 2019-10-03 654 }
87e5e95db31a27 Sakari Ailus 2019-10-03 655 EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
87e5e95db31a27 Sakari Ailus 2019-10-03 656
--
0-DAY CI Kernel Test Service
https://01.org/lkp
> @@ -480,15 +485,19 @@ 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);
> + if (IS_ERR_OR_NULL(fwnode->secondary))
> + return -ENOENT;
Doesn't this mean you overwrite any return code != 0 with -ENOENT?
Is this intended?
In any case:
Tested-by: Michael Walle <[email protected]>
-michael
Hi Andy,
I love your patch! Yet something to improve:
[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on rafael-pm/linux-next linus/master linux/master v5.18-rc1 next-20220406]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/device-property-Allow-error-pointer-to-be-passed-to-fwnode-APIs/20220407-002511
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 3123109284176b1532874591f7c81f3837bbdc17
config: hexagon-randconfig-r032-20220406 (https://download.01.org/0day-ci/archive/20220407/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project c4a1b07d0979e7ff20d7d541af666d822d66b566)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/d9d353ada8d8c3b1b7f3965ad7fe191bd7dea930
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Andy-Shevchenko/device-property-Allow-error-pointer-to-be-passed-to-fwnode-APIs/20220407-002511
git checkout d9d353ada8d8c3b1b7f3965ad7fe191bd7dea930
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/base/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/base/property.c:647:28: error: passing 'const struct fwnode_handle *' to parameter of type 'struct fwnode_handle *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
return fwnode_handle_get(fwnode);
^~~~~~
include/linux/property.h:123:63: note: passing argument to parameter 'fwnode' here
struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
^
1 error generated.
vim +647 drivers/base/property.c
87e5e95db31a27d Sakari Ailus 2019-10-03 629
87e5e95db31a27d Sakari Ailus 2019-10-03 630 /**
87e5e95db31a27d Sakari Ailus 2019-10-03 631 * fwnode_get_nth_parent - Return an nth parent of a node
87e5e95db31a27d Sakari Ailus 2019-10-03 632 * @fwnode: The node the parent of which is requested
87e5e95db31a27d Sakari Ailus 2019-10-03 633 * @depth: Distance of the parent from the node
87e5e95db31a27d Sakari Ailus 2019-10-03 634 *
87e5e95db31a27d Sakari Ailus 2019-10-03 635 * Returns the nth parent of a node. If there is no parent at the requested
87e5e95db31a27d Sakari Ailus 2019-10-03 636 * @depth, %NULL is returned. If @depth is 0, the functionality is equivalent to
87e5e95db31a27d Sakari Ailus 2019-10-03 637 * fwnode_handle_get(). For @depth == 1, it is fwnode_get_parent() and so on.
87e5e95db31a27d Sakari Ailus 2019-10-03 638 *
87e5e95db31a27d Sakari Ailus 2019-10-03 639 * The caller is responsible for calling fwnode_handle_put() for the returned
87e5e95db31a27d Sakari Ailus 2019-10-03 640 * node.
87e5e95db31a27d Sakari Ailus 2019-10-03 641 */
d9d353ada8d8c3b Andy Shevchenko 2022-04-06 642 struct fwnode_handle *fwnode_get_nth_parent(const struct fwnode_handle *fwnode, unsigned int depth)
87e5e95db31a27d Sakari Ailus 2019-10-03 643 {
040f806ecab6cd6 Andy Shevchenko 2022-04-06 644 struct fwnode_handle *parent;
87e5e95db31a27d Sakari Ailus 2019-10-03 645
040f806ecab6cd6 Andy Shevchenko 2022-04-06 646 if (depth == 0)
040f806ecab6cd6 Andy Shevchenko 2022-04-06 @647 return fwnode_handle_get(fwnode);
87e5e95db31a27d Sakari Ailus 2019-10-03 648
040f806ecab6cd6 Andy Shevchenko 2022-04-06 649 fwnode_for_each_parent_node(fwnode, parent) {
040f806ecab6cd6 Andy Shevchenko 2022-04-06 650 if (--depth == 0)
040f806ecab6cd6 Andy Shevchenko 2022-04-06 651 return parent;
040f806ecab6cd6 Andy Shevchenko 2022-04-06 652 }
040f806ecab6cd6 Andy Shevchenko 2022-04-06 653 return NULL;
87e5e95db31a27d Sakari Ailus 2019-10-03 654 }
87e5e95db31a27d Sakari Ailus 2019-10-03 655 EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
87e5e95db31a27d Sakari Ailus 2019-10-03 656
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Wed, Apr 06, 2022 at 08:05:23PM +0200, Michael Walle wrote:
...
> > + 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);
> > + if (IS_ERR_OR_NULL(fwnode->secondary))
> > + return -ENOENT;
>
> Doesn't this mean you overwrite any return code != 0 with -ENOENT?
> Is this intended?
Indeed, it would shadow the error code.
So, it should go with
if (IS_ERR_OR_NULL(fwnode->secondary))
return ret;
then.
> In any case:
> Tested-by: Michael Walle <[email protected]>
Thanks!
--
With Best Regards,
Andy Shevchenko
Hi Andy,
I love your patch! Perhaps something to improve:
[auto build test WARNING on driver-core/driver-core-testing]
[also build test WARNING on rafael-pm/linux-next linus/master linux/master v5.18-rc1 next-20220406]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/device-property-Allow-error-pointer-to-be-passed-to-fwnode-APIs/20220407-002511
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 3123109284176b1532874591f7c81f3837bbdc17
config: i386-randconfig-s001 (https://download.01.org/0day-ci/archive/20220407/[email protected]/config)
compiler: gcc-11 (Debian 11.2.0-19) 11.2.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.4-dirty
# https://github.com/intel-lab-lkp/linux/commit/d9d353ada8d8c3b1b7f3965ad7fe191bd7dea930
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Andy-Shevchenko/device-property-Allow-error-pointer-to-be-passed-to-fwnode-APIs/20220407-002511
git checkout d9d353ada8d8c3b1b7f3965ad7fe191bd7dea930
# save the config file to linux build tree
mkdir build_dir
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/base/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> drivers/base/property.c:647:42: sparse: sparse: incorrect type in argument 1 (different modifiers) @@ expected struct fwnode_handle *fwnode @@ got struct fwnode_handle const *fwnode @@
drivers/base/property.c:647:42: sparse: expected struct fwnode_handle *fwnode
drivers/base/property.c:647:42: sparse: got struct fwnode_handle const *fwnode
vim +647 drivers/base/property.c
87e5e95db31a27 Sakari Ailus 2019-10-03 629
87e5e95db31a27 Sakari Ailus 2019-10-03 630 /**
87e5e95db31a27 Sakari Ailus 2019-10-03 631 * fwnode_get_nth_parent - Return an nth parent of a node
87e5e95db31a27 Sakari Ailus 2019-10-03 632 * @fwnode: The node the parent of which is requested
87e5e95db31a27 Sakari Ailus 2019-10-03 633 * @depth: Distance of the parent from the node
87e5e95db31a27 Sakari Ailus 2019-10-03 634 *
87e5e95db31a27 Sakari Ailus 2019-10-03 635 * Returns the nth parent of a node. If there is no parent at the requested
87e5e95db31a27 Sakari Ailus 2019-10-03 636 * @depth, %NULL is returned. If @depth is 0, the functionality is equivalent to
87e5e95db31a27 Sakari Ailus 2019-10-03 637 * fwnode_handle_get(). For @depth == 1, it is fwnode_get_parent() and so on.
87e5e95db31a27 Sakari Ailus 2019-10-03 638 *
87e5e95db31a27 Sakari Ailus 2019-10-03 639 * The caller is responsible for calling fwnode_handle_put() for the returned
87e5e95db31a27 Sakari Ailus 2019-10-03 640 * node.
87e5e95db31a27 Sakari Ailus 2019-10-03 641 */
d9d353ada8d8c3 Andy Shevchenko 2022-04-06 642 struct fwnode_handle *fwnode_get_nth_parent(const struct fwnode_handle *fwnode, unsigned int depth)
87e5e95db31a27 Sakari Ailus 2019-10-03 643 {
040f806ecab6cd Andy Shevchenko 2022-04-06 644 struct fwnode_handle *parent;
87e5e95db31a27 Sakari Ailus 2019-10-03 645
040f806ecab6cd Andy Shevchenko 2022-04-06 646 if (depth == 0)
040f806ecab6cd Andy Shevchenko 2022-04-06 @647 return fwnode_handle_get(fwnode);
87e5e95db31a27 Sakari Ailus 2019-10-03 648
040f806ecab6cd Andy Shevchenko 2022-04-06 649 fwnode_for_each_parent_node(fwnode, parent) {
040f806ecab6cd Andy Shevchenko 2022-04-06 650 if (--depth == 0)
040f806ecab6cd Andy Shevchenko 2022-04-06 651 return parent;
040f806ecab6cd Andy Shevchenko 2022-04-06 652 }
040f806ecab6cd Andy Shevchenko 2022-04-06 653 return NULL;
87e5e95db31a27 Sakari Ailus 2019-10-03 654 }
87e5e95db31a27 Sakari Ailus 2019-10-03 655 EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
87e5e95db31a27 Sakari Ailus 2019-10-03 656
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Andy,
On Fri, Apr 08, 2022 at 03:44:03PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 07, 2022 at 01:19:44PM +0300, Andy Shevchenko wrote:
> > On Wed, Apr 06, 2022 at 08:05:23PM +0200, Michael Walle wrote:
>
> ...
>
> > > > + 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);
> > > > + if (IS_ERR_OR_NULL(fwnode->secondary))
> > > > + return -ENOENT;
> > >
> > > Doesn't this mean you overwrite any return code != 0 with -ENOENT?
> > > Is this intended?
> >
> > Indeed, it would shadow the error code.
>
> I was thinking more on this and am not sure about the best approach here.
> On one hand in the original code this returns the actual error code from
> the call against primary fwnode. But it can be at least -ENOENT or -EINVAL.
>
> But when we check the secondary fwnode we want to have understanding that it's
> secondary fwnode which has not been found, but this requires to have a good
> distinguishing between error codes from the callback.
>
> That said, the error codes convention of ->get_reference_args() simply
> sucks. Sakari, do you have it on your TODO to fix this mess out, if it's
> even feasible?
What would you expect to see compared to what it is now?
I guess the error code could be different for a missing property and a
property with invalid data, but I'm not sure any caller would be interested
in that.
>
> To be on safest side, I will change as suggested in previous mail (see below)
> so it won't have impact on -EINVAL case.
>
> > So, it should go with
> >
> > if (IS_ERR_OR_NULL(fwnode->secondary))
> > return ret;
> >
> > then.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
--
Sakari Ailus
On Thu, Apr 07, 2022 at 01:19:44PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 06, 2022 at 08:05:23PM +0200, Michael Walle wrote:
...
> > > + 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);
> > > + if (IS_ERR_OR_NULL(fwnode->secondary))
> > > + return -ENOENT;
> >
> > Doesn't this mean you overwrite any return code != 0 with -ENOENT?
> > Is this intended?
>
> Indeed, it would shadow the error code.
I was thinking more on this and am not sure about the best approach here.
On one hand in the original code this returns the actual error code from
the call against primary fwnode. But it can be at least -ENOENT or -EINVAL.
But when we check the secondary fwnode we want to have understanding that it's
secondary fwnode which has not been found, but this requires to have a good
distinguishing between error codes from the callback.
That said, the error codes convention of ->get_reference_args() simply
sucks. Sakari, do you have it on your TODO to fix this mess out, if it's
even feasible?
To be on safest side, I will change as suggested in previous mail (see below)
so it won't have impact on -EINVAL case.
> So, it should go with
>
> if (IS_ERR_OR_NULL(fwnode->secondary))
> return ret;
>
> then.
--
With Best Regards,
Andy Shevchenko
On Fri, Apr 08, 2022 at 04:27:26PM +0300, Sakari Ailus wrote:
> On Fri, Apr 08, 2022 at 03:44:03PM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 07, 2022 at 01:19:44PM +0300, Andy Shevchenko wrote:
> > > On Wed, Apr 06, 2022 at 08:05:23PM +0200, Michael Walle wrote:
...
> > > > > + 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);
> > > > > + if (IS_ERR_OR_NULL(fwnode->secondary))
> > > > > + return -ENOENT;
> > > >
> > > > Doesn't this mean you overwrite any return code != 0 with -ENOENT?
> > > > Is this intended?
> > >
> > > Indeed, it would shadow the error code.
> >
> > I was thinking more on this and am not sure about the best approach here.
> > On one hand in the original code this returns the actual error code from
> > the call against primary fwnode. But it can be at least -ENOENT or -EINVAL.
> >
> > But when we check the secondary fwnode we want to have understanding that it's
> > secondary fwnode which has not been found, but this requires to have a good
> > distinguishing between error codes from the callback.
> >
> > That said, the error codes convention of ->get_reference_args() simply
> > sucks. Sakari, do you have it on your TODO to fix this mess out, if it's
> > even feasible?
>
> What would you expect to see compared to what it is now?
>
> I guess the error code could be different for a missing property and a
> property with invalid data,
Yes, something like this.
> but I'm not sure any caller would be interested
> in that.
Yes, but it would be good for the consistency and working with fwnodes in
general. Esp. if we move at some point from primary-secondary to a full
linked list of fwnodes.
> > To be on safest side, I will change as suggested in previous mail (see below)
> > so it won't have impact on -EINVAL case.
> >
> > > So, it should go with
> > >
> > > if (IS_ERR_OR_NULL(fwnode->secondary))
> > > return ret;
> > >
> > > then.
--
With Best Regards,
Andy Shevchenko