Message ID | 20250210-fix_arg_count-v3-1-a084a5013008@quicinc.com (mailing list archive) |
---|---|
State | Handled Elsewhere, archived |
Headers | show |
Series | [v3] of: property: Increase NR_FWNODE_REFERENCE_ARGS | expand |
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.
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 ?
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.
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.
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
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 ?
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