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 <sean.anderson@linux.dev>
---
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(-)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index c1392743df9c..049f8a6088a1 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -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);
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index b1726a3515f6..11af2001478f 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -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)
diff --git a/drivers/of/property.c b/drivers/of/property.c
index c1feb631e383..c41190e47111 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -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) {
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 6fa0a268d538..69fe44c68f8c 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -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,
--
2.35.1.1320.gc452695387.dirtyHi Sean,
On Mon, Apr 07, 2025 at 06:37:13PM -0400, Sean Anderson wrote:
> 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.
>
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
>
> 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(-)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index c1392743df9c..049f8a6088a1 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -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);
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index b1726a3515f6..11af2001478f 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -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)
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index c1feb631e383..c41190e47111 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -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) {
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 6fa0a268d538..69fe44c68f8c 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -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,
--
Regards,
Sakari Ailus
On Tue, Apr 8, 2025 at 4:14 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Sean, > > On Mon, Apr 07, 2025 at 06:37:13PM -0400, Sean Anderson wrote: > > 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? #msi-cells for one is optional because we added it after the initial bindings and missing means 0. > 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. The DT API supports this already. Rob
On Mon, Apr 07, 2025 at 06:37:13PM -0400, Sean Anderson wrote:
> 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?
...
> ret = fwnode_call_int_op(fwnode, get_reference_args, prop, nargs_prop,
> - nargs, index, args);
> + nargs_prop ? -1 : nargs, index, args);
> return fwnode_call_int_op(fwnode->secondary, get_reference_args, prop, nargs_prop,
> - nargs, index, args);
> + nargs_prop ? -1 : nargs, index, args);
I don't understand why it's needed here. The nargs_prop is passed to the callee.
...
> - unsigned int nargs, unsigned int index,
> + int nargs, unsigned int index,
As per above.
...
> error = property_entry_read_int_array(ref->node->properties,
> nargs_prop, sizeof(u32),
> &nargs_prop_val, 1);
> - if (error)
> +
Stray blank line.
> + if (error == -EINVAL) {
Why do we need an explicit error code check? This is fragile. Just check the
parameter before calling the above.
> + if (nargs < 0)
> + return error;
> + } else if (error) {
> return error;
> -
> - nargs = nargs_prop_val;
> + } else {
> + nargs = nargs_prop_val;
> + }
...
> 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)
Same comments as per above.
--
With Best Regards,
Andy Shevchenko
On Tue, Apr 8, 2025 at 3:37 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Apr 07, 2025 at 06:37:13PM -0400, Sean Anderson wrote: > > 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? People still care when I break 1990s PowerMacs... It's more that some bindings (like MSI) start out without #foo-cells, and then we end up adding arg cells later. So we have to support no #foo-cells means 0. Rob
© 2016 - 2025 Red Hat, Inc.