device property: Add fwnode_property_get_reference_optional_args

get_reference_args does not permit falling back to nargs when nargs_prop is missing. This makes it difficult to support older devicetrees where nargs_prop may not be present. Add support for this by converting nargs to a signed value. Where before nargs was ignored if nargs_prop was passed, now nargs is only ignored if it is strictly negative. When it is positive, nargs represents the fallback cells to use if nargs_prop is absent.

drivers/base/swnode.c +6 -2

+			if (nargs < 0)
+				return error;
+		} else if (error) {
 			return error;
-
-		nargs = nargs_prop_val;
+		} else {
+			nargs = nargs_prop_val;
+		}

Quoted 7 lines · Sean Anderson

get_reference_args does not permit falling back to nargs when nargs_prop is missing. This makes it difficult to support older devicetrees where nargs_prop may not be present. Add support for this by converting nargs to a signed value. Where before nargs was ignored if nargs_prop was passed, now nargs is only ignored if it is strictly negative. When it is positive, nargs represents the fallback cells to use if nargs_prop is absent.

And what is the case to support old DTs on most likely outdated hardware?

get_reference_args does not permit falling back to nargs when nargs_prop is missing. This makes it difficult to support older devicetrees where nargs_prop may not be present. Add support for this by converting nargs to a signed value. Where before nargs was ignored if nargs_prop was passed, now nargs is only ignored if it is strictly negative. When it is positive, nargs represents the fallback cells to use if nargs_prop is absent.

Signed-off-by Sean Anderson <[email protected]>

---

 drivers/base/property.c |  4 ++--
 drivers/base/swnode.c   | 13 +++++++++----
 drivers/of/property.c   | 10 +++-------
 include/linux/fwnode.h  |  2 +-
 4 files changed, 15 insertions(+), 14 deletions(-)

drivers/base/property.c +2 -2

index c1392743df9c..049f8a6088a1 100644
@@ -606,7 +606,7 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
 		return -ENOENT;
 
 	ret = fwnode_call_int_op(fwnode, get_reference_args, prop, nargs_prop,
-				 nargs, index, args);
+				 nargs_prop ? -1 : nargs, index, args);
 	if (ret == 0)
 		return ret;
 

@@ -614,7 +614,7 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
 		return ret;
 
 	return fwnode_call_int_op(fwnode->secondary, get_reference_args, prop, nargs_prop,
-				  nargs, index, args);
+				  nargs_prop ? -1 : nargs, index, args);
 }
 EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args);
 

drivers/base/swnode.c +9 -4

index b1726a3515f6..11af2001478f 100644
@@ -503,7 +503,7 @@ software_node_get_named_child_node(const struct fwnode_handle *fwnode,
 static int
 software_node_get_reference_args(const struct fwnode_handle *fwnode,
 				 const char *propname, const char *nargs_prop,
-				 unsigned int nargs, unsigned int index,
+				 int nargs, unsigned int index,
 				 struct fwnode_reference_args *args)
 {
 	struct swnode *swnode = to_swnode(fwnode);

@@ -543,10 +543,15 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 		error = property_entry_read_int_array(ref->node->properties,
 						      nargs_prop, sizeof(u32),
 						      &nargs_prop_val, 1);
-		if (error)
+
+		if (error == -EINVAL) {
+			if (nargs < 0)
+				return error;
+		} else if (error) {
 			return error;
-
-		nargs = nargs_prop_val;
+		} else {
+			nargs = nargs_prop_val;
+		}
 	}
 
 	if (nargs > NR_FWNODE_REFERENCE_ARGS)

drivers/of/property.c +3 -7

index c1feb631e383..c41190e47111 100644
@@ -1116,19 +1116,15 @@ of_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
 static int
 of_fwnode_get_reference_args(const struct fwnode_handle *fwnode,
 			     const char *prop, const char *nargs_prop,
-			     unsigned int nargs, unsigned int index,
+			     int nargs, unsigned int index,
 			     struct fwnode_reference_args *args)
 {
 	struct of_phandle_args of_args;
 	unsigned int i;
 	int ret;
 
-	if (nargs_prop)
-		ret = of_parse_phandle_with_args(to_of_node(fwnode), prop,
-						 nargs_prop, index, &of_args);
-	else
-		ret = of_parse_phandle_with_fixed_args(to_of_node(fwnode), prop,
-						       nargs, index, &of_args);
+	ret = __of_parse_phandle_with_args(to_of_node(fwnode), prop, nargs_prop,
+					   nargs, index, &of_args);
 	if (ret < 0)
 		return ret;
 	if (!args) {

include/linux/fwnode.h +1 -1

index 6fa0a268d538..69fe44c68f8c 100644
@@ -163,7 +163,7 @@ struct fwnode_operations {
 				const char *name);
 	int (*get_reference_args)(const struct fwnode_handle *fwnode,
 				  const char *prop, const char *nargs_prop,
-				  unsigned int nargs, unsigned int index,
+				  int nargs, unsigned int index,
 				  struct fwnode_reference_args *args);
 	struct fwnode_handle *
 	(*graph_get_next_endpoint)(const struct fwnode_handle *fwnode,

Hi Sean,

Quoted 7 lines · Sean Anderson

get_reference_args does not permit falling back to nargs when nargs_prop is missing. This makes it difficult to support older devicetrees where nargs_prop may not be present. Add support for this by converting nargs to a signed value. Where before nargs was ignored if nargs_prop was passed, now nargs is only ignored if it is strictly negative. When it is positive, nargs represents the fallback cells to use if nargs_prop is absent.

If you don't know either the argument count or have a property that tells it, there's no way to differentiate phandles from arguments. I'd say such DTS are broken. Where do they exist?

At the very least this needs to be documented as a workaround and moved to the OF framework. I wouldn't add such a workaround to swnodes either, the bugs should be fixed instead.

Add a fwnode variant of of_parse_phandle_with_optional_args to allow nargs_prop to be absent from the referenced node. This improves compatibility for references where the devicetree might not always have nargs_prop.

+/**
+ * fwnode_property_get_reference_optional_args() - Find a reference with optional arguments
+ * @fwnode: Firmware node where to look for the reference
+ * @prop: The name of the property
+ * @nargs_prop: The name of the property telling the number of

+ * arguments in the referred node.
+ * @index: Index of the reference, from zero onwards.
+ * @args: Result structure with reference and integer arguments.
+ * May be NULL.
+ *
+ * Obtain a reference based on a named property in an fwnode, with
+ * integer arguments. If @nargs_prop is absent from the referenced node, then
+ * number of arguments is be assumed to be 0.
+ *
+ * The caller is responsible for calling fwnode_handle_put() on the returned
+ * @args->fwnode pointer.
+ *
+ * Return: %0 on success
+ * %-ENOENT when the index is out of bounds, the index has an empty
+ * reference or the property was not found
+ * %-EINVAL on parse error
+ */

drivers/base/property.c +3 -0

+	return fwnode_call_int_op(fwnode->secondary, get_reference_args, prop, nargs_prop,
+				  0, index, args);
+}

Quoted 4 lines · Sean Anderson

Add a fwnode variant of of_parse_phandle_with_optional_args to allow nargs_prop to be absent from the referenced node. This improves compatibility for references where the devicetree might not always have nargs_prop.

...

Quoted 5 lines

+/**
+ * fwnode_property_get_reference_optional_args() - Find a reference with optional arguments
+ * @fwnode: Firmware node where to look for the reference
+ * @prop: The name of the property
+ * @nargs_prop: The name of the property telling the number of

Use space instead of TAB as it's already too long to make it aligned with the rest.

Quoted 17 lines

+ * arguments in the referred node.
+ * @index: Index of the reference, from zero onwards.
+ * @args: Result structure with reference and integer arguments.
+ * May be NULL.
+ *
+ * Obtain a reference based on a named property in an fwnode, with
+ * integer arguments. If @nargs_prop is absent from the referenced node, then
+ * number of arguments is be assumed to be 0.
+ *
+ * The caller is responsible for calling fwnode_handle_put() on the returned
+ * @args->fwnode pointer.
+ *
+ * Return: %0 on success
+ * %-ENOENT when the index is out of bounds, the index has an empty
+ * reference or the property was not found
+ * %-EINVAL on parse error
+ */

drivers/base/property.c int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode, · line 652

Quoted 10 lines

drivers/base/property.c +9 -0

+int fwnode_property_get_reference_optional_args(const struct fwnode_handle *fwnode,
+						const char *prop,
+						const char *nargs_prop,
+						unsigned int index,
+						struct fwnode_reference_args *args)
+{
+	int ret;
 
+	if (IS_ERR_OR_NULL(fwnode))
+		return -ENOENT;

This is incorrect most likely, see below.

drivers/base/property.c int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode, · line 660

Quoted 7 lines

drivers/base/property.c +7 -0

+	ret = fwnode_call_int_op(fwnode, get_reference_args, prop, nargs_prop,
+				 0, index, args);
+	if (ret == 0)
+		return ret;
+
+	if (IS_ERR_OR_NULL(fwnode->secondary))
+		return ret;

Here no such error code shadowing, and TBH I do not like the shadowing without real need.

Add a fwnode variant of of_parse_phandle_with_optional_args to allow nargs_prop to be absent from the referenced node. This improves compatibility for references where the devicetree might not always have nargs_prop.

Quoted 5 lines · Sean Anderson

Add a fwnode variant of of_parse_phandle_with_optional_args to allow nargs_prop to be absent from the referenced node. This improves compatibility for references where the devicetree might not always have nargs_prop.

Can't we just make fwnode_property_get_reference_args() handle this case? Or why is it not just a 1 line wrapper function?

Quoted 7 lines · Rob Herring

Quoted 5 lines · Sean Anderson

Add a fwnode variant of of_parse_phandle_with_optional_args to allow nargs_prop to be absent from the referenced node. This improves compatibility for references where the devicetree might not always have nargs_prop.

Quoted 2 lines

Can't we just make fwnode_property_get_reference_args() handle this case? Or why is it not just a 1 line wrapper function?

fwnode_property_get_reference_args ignores nargs when nargs_prop is non-NULL. So all the existing callers just pass 0 to nargs. Rather than convert them, I chose to add another function with different defaults. There are only four callers that pass nargs_prop, so I could just as easily change the callers instead.

Quoted 9 lines · Sean Anderson

Quoted 7 lines · Rob Herring

Quoted 5 lines · Sean Anderson

Add a fwnode variant of of_parse_phandle_with_optional_args to allow nargs_prop to be absent from the referenced node. This improves compatibility for references where the devicetree might not always have nargs_prop.

Quoted 8 lines

Quoted 2 lines

Can't we just make fwnode_property_get_reference_args() handle this case? Or why is it not just a 1 line wrapper function?

fwnode_property_get_reference_args ignores nargs when nargs_prop is non-NULL. So all the existing callers just pass 0 to nargs. Rather than convert them, I chose to add another function with different defaults. There are only four callers that pass nargs_prop, so I could just as easily change the callers instead.

Why do you have to change the callers? nargs value won't matter because they obviously have nargs_prop present or they would not have worked in the first place. If behavior changes because there's an error in their DT, who cares. That's their problem for not validating the DT.

Quoted 19 lines · Rob Herring

Quoted 9 lines · Sean Anderson

Quoted 7 lines · Rob Herring

Quoted 5 lines · Sean Anderson

Add a fwnode variant of of_parse_phandle_with_optional_args to allow nargs_prop to be absent from the referenced node. This improves compatibility for references where the devicetree might not always have nargs_prop.

Quoted 8 lines

Quoted 2 lines

Can't we just make fwnode_property_get_reference_args() handle this case? Or why is it not just a 1 line wrapper function?

fwnode_property_get_reference_args ignores nargs when nargs_prop is non-NULL. So all the existing callers just pass 0 to nargs. Rather than convert them, I chose to add another function with different defaults. There are only four callers that pass nargs_prop, so I could just as easily change the callers instead.

Quoted 5 lines

Why do you have to change the callers? nargs value won't matter because they obviously have nargs_prop present or they would not have worked in the first place. If behavior changes because there's an error in their DT, who cares. That's their problem for not validating the DT.

Because the change would be to make nargs matter even when nargs_prop is present. For the sake of example, consider something like

  foo: foo {
    #my-cells = <1>;
  };

  bar: bar {
  };

  baz {
    my-prop = <&bar>, <&foo 5>, ;
    my-prop-names = "bar", "foo";
  };

Before we would have

fwnode_property_get_reference_args(baz, "my-prop", NULL, 0, "bar", args) <bar> fwnode_property_get_reference_args(baz, "my-prop", NULL, 0, "foo", args) <foo> fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", -1, "bar", args) ERROR fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", -1, "foo", args) ERROR fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", 0, "bar", args) ERROR fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", 0, "foo", args) ERROR

and after we would have

fwnode_property_get_reference_args(baz, "my-prop", NULL, 0, "bar", args) <bar> fwnode_property_get_reference_args(baz, "my-prop", NULL, 0, "foo", args) <foo> fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", -1, "bar", args) ERROR fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", -1, "foo", args) ERROR fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", 0, "bar", args) <bar> fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", 0, "foo", args) <foo 5>

The problem is that all existing callers pass nargs=0 when nargs_prop="#my-cells" so they will get the new behavior even when they shouldn't. So if we change the behavior we have to change the callers too. If we make a new function with new behavior the callers stay the same.

This series adds a fwnode-equivalent to of_parse_phandle_with_optional_args.

Quoted 1 line · Sean Anderson

This series adds a fwnode-equivalent to of_parse_phandle_with_optional_args.

Thanks, but no user. Not acceptable in this form. Also see individual comments.