[PATCH v3] of: property: Increase NR_FWNODE_REFERENCE_ARGS

[PATCH v3] of: property: Increase NR_FWNODE_REFERENCE_ARGS

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

Currently, the following two macros have different values:

// The maximal argument count for firmware node reference
 #define NR_FWNODE_REFERENCE_ARGS	8
// The maximal argument count for DT node reference
 #define MAX_PHANDLE_ARGS 16

It may cause firmware node reference's argument count out of range if
directly assign DT node reference's argument count to firmware's.

drivers/of/property.c:of_fwnode_get_reference_args() is doing the direct
assignment, so may cause firmware's argument count @args->nargs got out
of range, namely, in [9, 16].

Fix by increasing NR_FWNODE_REFERENCE_ARGS to 16 to meet DT requirement.

Fixes: 3e3119d3088f ("device property: Introduce fwnode_property_get_reference_args")
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
May remove MAX_PHANDLE_ARGS and use NR_FWNODE_REFERENCE_ARGS instead later.
---
Changes in v3:
- Remove RFC, correct title and commit message.
- Link to v2: https://lore.kernel.org/r/20250114-fix_arg_count-v2-1-efa35ee6572b@quicinc.com

Changes in v2:
- Increase macro @NR_FWNODE_REFERENCE_ARGS to align with @MAX_PHANDLE_ARGS.
- Correct fix tag and send as RFC patch.
- Link to v1: https://lore.kernel.org/r/20250109-of_core_fix-v4-7-db8a72415b8c@quicinc.com
---
 include/linux/fwnode.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 0731994b9d7c832cae8a30063f3a64194e4f19aa..6fa0a268d53827a376d7f258c6194a2a088e4325 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -91,7 +91,7 @@ struct fwnode_endpoint {
 #define SWNODE_GRAPH_PORT_NAME_FMT		"port@%u"
 #define SWNODE_GRAPH_ENDPOINT_NAME_FMT		"endpoint@%u"
 
-#define NR_FWNODE_REFERENCE_ARGS	8
+#define NR_FWNODE_REFERENCE_ARGS	16
 
 /**
  * struct fwnode_reference_args - Fwnode reference with additional arguments

---
base-commit: 40fc0083a9dbcf2e81b1506274cb541f84d022ed
change-id: 20250114-fix_arg_count-73feae90fccb

Best regards,
-- 
Zijun Hu <quic_zijuhu@quicinc.com>

Re: [PATCH v3] of: property: Increase NR_FWNODE_REFERENCE_ARGS

[Andy Shevchenko]

On Mon, Feb 10, 2025 at 11:00:32PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> Currently, the following two macros have different values:
> 
> // The maximal argument count for firmware node reference
>  #define NR_FWNODE_REFERENCE_ARGS	8
> // The maximal argument count for DT node reference
>  #define MAX_PHANDLE_ARGS 16
> 
> It may cause firmware node reference's argument count out of range if
> directly assign DT node reference's argument count to firmware's.
> 
> drivers/of/property.c:of_fwnode_get_reference_args() is doing the direct
> assignment, so may cause firmware's argument count @args->nargs got out
> of range, namely, in [9, 16].
> 
> Fix by increasing NR_FWNODE_REFERENCE_ARGS to 16 to meet DT requirement.

...

> -#define NR_FWNODE_REFERENCE_ARGS	8
> +#define NR_FWNODE_REFERENCE_ARGS	16

Thinking of the case, perhaps you also want

static_assert(NR_FWNODE_REFERENCE_ARGS == MAX_PHANDLE_ARGS);

to be put somewhere, but I don't think we can do it in this header file.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] of: property: Increase NR_FWNODE_REFERENCE_ARGS

[Zijun Hu]

On 2025/2/10 23:34, Andy Shevchenko wrote:
>> It may cause firmware node reference's argument count out of range if
>> directly assign DT node reference's argument count to firmware's.
>>
>> drivers/of/property.c:of_fwnode_get_reference_args() is doing the direct
>> assignment, so may cause firmware's argument count @args->nargs got out
>> of range, namely, in [9, 16].
>>
>> Fix by increasing NR_FWNODE_REFERENCE_ARGS to 16 to meet DT requirement.
> ...
> 
>> -#define NR_FWNODE_REFERENCE_ARGS	8
>> +#define NR_FWNODE_REFERENCE_ARGS	16
> Thinking of the case, perhaps you also want
> 
> static_assert(NR_FWNODE_REFERENCE_ARGS == MAX_PHANDLE_ARGS);
> 
> to be put somewhere, but I don't think we can do it in this header file.

thank you Andy for code review.

yes. it seems there are good location to place the static_assert().

is it okay to associate two macros by
#define MAX_PHANDLE_ARGS NR_FWNODE_REFERENCE_ARGS
OR
replace all MAX_PHANDLE_ARGS instances with NR_FWNODE_REFERENCE_ARGS
?

Re: [PATCH v3] of: property: Increase NR_FWNODE_REFERENCE_ARGS

[Andy Shevchenko]

On Tue, Feb 11, 2025 at 08:20:03PM +0800, Zijun Hu wrote:
> On 2025/2/10 23:34, Andy Shevchenko wrote:
> >> It may cause firmware node reference's argument count out of range if
> >> directly assign DT node reference's argument count to firmware's.
> >>
> >> drivers/of/property.c:of_fwnode_get_reference_args() is doing the direct
> >> assignment, so may cause firmware's argument count @args->nargs got out
> >> of range, namely, in [9, 16].
> >>
> >> Fix by increasing NR_FWNODE_REFERENCE_ARGS to 16 to meet DT requirement.

...

> >> -#define NR_FWNODE_REFERENCE_ARGS	8
> >> +#define NR_FWNODE_REFERENCE_ARGS	16
> > Thinking of the case, perhaps you also want
> > 
> > static_assert(NR_FWNODE_REFERENCE_ARGS == MAX_PHANDLE_ARGS);
> > 
> > to be put somewhere, but I don't think we can do it in this header file.
> 
> thank you Andy for code review.
> 
> yes. it seems there are good location to place the static_assert().
> 
> is it okay to associate two macros by
> #define MAX_PHANDLE_ARGS NR_FWNODE_REFERENCE_ARGS

I was thinking about this and I don't see how it can be done without
introducing more chaos (dependency hell) into the headers. So, I won't
take this path or even consider it deeper.

> OR
> replace all MAX_PHANDLE_ARGS instances with NR_FWNODE_REFERENCE_ARGS
> ?

This sounds plausible to me, but you need a blessing from OF people as
the naming may be a bit confusing (for them) as "phandle" is well established
term in OF realm.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] of: property: Increase NR_FWNODE_REFERENCE_ARGS

[Zijun Hu]

On 2025/2/11 20:24, Andy Shevchenko wrote:
>>>> -#define NR_FWNODE_REFERENCE_ARGS	8
>>>> +#define NR_FWNODE_REFERENCE_ARGS	16
>>> Thinking of the case, perhaps you also want
>>>
>>> static_assert(NR_FWNODE_REFERENCE_ARGS == MAX_PHANDLE_ARGS);
>>>
>>> to be put somewhere, but I don't think we can do it in this header file.
>> thank you Andy for code review.
>>
>> yes. it seems there are good location to place the static_assert().
>>
>> is it okay to associate two macros by
>> #define MAX_PHANDLE_ARGS NR_FWNODE_REFERENCE_ARGS
> I was thinking about this and I don't see how it can be done without
> introducing more chaos (dependency hell) into the headers. So, I won't
> take this path or even consider it deeper.
> 

i have confirmed that:

of.h includes fwnode.h indirectly
fwnode.h does not include of.h directly or indirectly

in theory, dependency between both headers should also be like this.

So, it is simple to use below define in of.h
#define MAX_PHANDLE_ARGS NR_FWNODE_REFERENCE_ARGS

>> OR
>> replace all MAX_PHANDLE_ARGS instances with NR_FWNODE_REFERENCE_ARGS
>> ?
> This sounds plausible to me, but you need a blessing from OF people as
> the naming may be a bit confusing (for them) as "phandle" is well established
> term in OF realm.

phandle is a type of DT firmware node reference. so this solution
seems suitable as well.

struct software_node_ref_args also uses NR_FWNODE_REFERENCE_ARGS directly.

let us wait for more comments.

Re: [PATCH v3] of: property: Increase NR_FWNODE_REFERENCE_ARGS

[Rob Herring]

On Tue, Feb 11, 2025 at 7:40 AM Zijun Hu <zijun_hu@icloud.com> wrote:
>
> On 2025/2/11 20:24, Andy Shevchenko wrote:
> >>>> -#define NR_FWNODE_REFERENCE_ARGS  8
> >>>> +#define NR_FWNODE_REFERENCE_ARGS  16
> >>> Thinking of the case, perhaps you also want
> >>>
> >>> static_assert(NR_FWNODE_REFERENCE_ARGS == MAX_PHANDLE_ARGS);
> >>>
> >>> to be put somewhere, but I don't think we can do it in this header file.
> >> thank you Andy for code review.
> >>
> >> yes. it seems there are good location to place the static_assert().
> >>
> >> is it okay to associate two macros by
> >> #define MAX_PHANDLE_ARGS NR_FWNODE_REFERENCE_ARGS
> > I was thinking about this and I don't see how it can be done without
> > introducing more chaos (dependency hell) into the headers. So, I won't
> > take this path or even consider it deeper.
> >
>
> i have confirmed that:
>
> of.h includes fwnode.h indirectly
> fwnode.h does not include of.h directly or indirectly

Only for struct fwnode_handle. I don't think we want to add to that.
For the most part, fwnode is a layer above DT and the DT code should
know nothing about fwnode.

> in theory, dependency between both headers should also be like this.
>
> So, it is simple to use below define in of.h
> #define MAX_PHANDLE_ARGS NR_FWNODE_REFERENCE_ARGS
>
> >> OR
> >> replace all MAX_PHANDLE_ARGS instances with NR_FWNODE_REFERENCE_ARGS
> >> ?
> > This sounds plausible to me, but you need a blessing from OF people as
> > the naming may be a bit confusing (for them) as "phandle" is well established
> > term in OF realm.
>
> phandle is a type of DT firmware node reference. so this solution
> seems suitable as well.
>
> struct software_node_ref_args also uses NR_FWNODE_REFERENCE_ARGS directly.
>
> let us wait for more comments.

I prefer what you have here with the static_assert() added.

Rob

Re: [PATCH v3] of: property: Increase NR_FWNODE_REFERENCE_ARGS

[Zijun Hu]

On 2025/2/11 22:22, Rob Herring wrote:
>> i have confirmed that:
>>
>> of.h includes fwnode.h indirectly
>> fwnode.h does not include of.h directly or indirectly
> Only for struct fwnode_handle. I don't think we want to add to that.
> For the most part, fwnode is a layer above DT and the DT code should
> know nothing about fwnode.

DT, as a type of firmware node, needs to implement firmware node
interfaces 'struct fwnode_operations' defined in fwnode.h.

so it includes fwnode.h and knows everything of fwnode.h

A)
diff --git a/include/linux/of.h b/include/linux/of.h
#define MAX_PHANDLE_ARGS 16
 struct of_phandle_args {
        struct device_node *np;
        int args_count;
        uint32_t args[MAX_PHANDLE_ARGS];
 };
+static_assert(NR_FWNODE_REFERENCE_ARGS == MAX_PHANDLE_ARGS);

B)
diff --git a/include/linux/of.h b/include/linux/of.h
-#define MAX_PHANDLE_ARGS 16
+#define MAX_PHANDLE_ARGS NR_FWNODE_REFERENCE_ARGS

you like solution A), right ?