| Message ID | 1437035444-13867-2-git-send-email-joro@8bytes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 16, 2015 at 09:30:43AM +0100, Joerg Roedel wrote: > From: Joerg Roedel <jroedel@suse.de> > > The main use of MAX_PHANDLE_ARGS is to define the number of > args elements in 'struct of_phandle_args'. This struct is > often declared on the stack and thus it is impractical to > increase MAX_PHANDLE_ARGS again and again. > > To handle situations where more than MAX_PHANDLE_ARGS > elements may appear in a device-tree, introduce functions > to allocate/free 'struct of_phandle_args' with more than > MAX_PHANDLE_ARGS elements and provide the new function > of_parse_phandle_with_var_args(), which can handle those > variable-size structs. > > This is necessary for the ARM-SMMU driver, where the number > of mmu-masters can be up to 128. > > Signed-off-by: Joerg Roedel <jroedel@suse.de> > --- > drivers/of/base.c | 43 ++++++++++++++++++++++++++++++++++++------- > include/linux/of.h | 7 +++++++ > 2 files changed, 43 insertions(+), 7 deletions(-) > > diff --git a/drivers/of/base.c b/drivers/of/base.c > index 8b5a187..2b288db 100644 > --- a/drivers/of/base.c > +++ b/drivers/of/base.c > @@ -54,6 +54,24 @@ DEFINE_MUTEX(of_mutex); > */ > DEFINE_RAW_SPINLOCK(devtree_lock); > > +struct of_phandle_args *of_alloc_phandle_args(int size) > +{ > + struct of_phandle_args *args; > + int e = max(0, size - MAX_PHANDLE_ARGS); > + > + args = kzalloc(sizeof(struct of_phandle_args) + e * sizeof(uint32_t), > + GFP_KERNEL); Should you also update args->args_count to reflect the extended array? That said, extending the fixed-size array member like this feels a bit fragile. Does GCC not complain about out-of-bounds accesses if you statically address args->args[MAX_PHANDLE_ARGS]? Admittedly, I can't think *why* this would be break (things like additional padding will be harmless), but I'm not intimate with the C standard. I guess the more worrying possibility is if somebody adds a new member to the end of of_phandle_args. Will
Hi Will, On Thu, Jul 16, 2015 at 11:23:26AM +0100, Will Deacon wrote: > On Thu, Jul 16, 2015 at 09:30:43AM +0100, Joerg Roedel wrote: > > +struct of_phandle_args *of_alloc_phandle_args(int size) > > +{ > > + struct of_phandle_args *args; > > + int e = max(0, size - MAX_PHANDLE_ARGS); > > + > > + args = kzalloc(sizeof(struct of_phandle_args) + e * sizeof(uint32_t), > > + GFP_KERNEL); > > Should you also update args->args_count to reflect the extended array? The args_count member just tells us how many of the array elements are used and not how many there are. So it doesn't need to be updated here. > That said, extending the fixed-size array member like this feels a bit > fragile. Does GCC not complain about out-of-bounds accesses if you > statically address args->args[MAX_PHANDLE_ARGS]? Admittedly, I can't > think *why* this would be break (things like additional padding will be > harmless), but I'm not intimate with the C standard. Yeah, I agree, it is not the best possible solution. But this way I don't need to update all callers, and thus it works better with our development model. But I am open for suggestions on how to solve this problem better. In fact, my main motivation in sending this was to get the discussion about an upstreamable solution started :) Lets see what the device-tree maintainers have to say. > I guess the more worrying possibility is if somebody adds a new member to > the end of of_phandle_args. I should probably add a comment there. Joerg
diff --git a/drivers/of/base.c b/drivers/of/base.c index 8b5a187..2b288db 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -54,6 +54,24 @@ DEFINE_MUTEX(of_mutex); */ DEFINE_RAW_SPINLOCK(devtree_lock); +struct of_phandle_args *of_alloc_phandle_args(int size) +{ + struct of_phandle_args *args; + int e = max(0, size - MAX_PHANDLE_ARGS); + + args = kzalloc(sizeof(struct of_phandle_args) + e * sizeof(uint32_t), + GFP_KERNEL); + + return args; +} +EXPORT_SYMBOL(of_alloc_phandle_args); + +void of_free_phandle_args(struct of_phandle_args *args) +{ + kfree(args); +} +EXPORT_SYMBOL(of_free_phandle_args); + int of_n_addr_cells(struct device_node *np) { const __be32 *ip; @@ -1446,7 +1464,8 @@ static int __of_parse_phandle_with_args(const struct device_node *np, const char *list_name, const char *cells_name, int cell_count, int index, - struct of_phandle_args *out_args) + struct of_phandle_args *out_args, + int elems) { const __be32 *list, *list_end; int rc = 0, size, cur_index = 0; @@ -1525,8 +1544,8 @@ static int __of_parse_phandle_with_args(const struct device_node *np, if (out_args) { int i; - if (WARN_ON(count > MAX_PHANDLE_ARGS)) - count = MAX_PHANDLE_ARGS; + if (WARN_ON(count > elems)) + count = elems; out_args->np = node; out_args->args_count = count; for (i = 0; i < count; i++) @@ -1577,7 +1596,7 @@ struct device_node *of_parse_phandle(const struct device_node *np, return NULL; if (__of_parse_phandle_with_args(np, phandle_name, NULL, 0, - index, &args)) + index, &args, MAX_PHANDLE_ARGS)) return NULL; return args.np; @@ -1623,10 +1642,20 @@ int of_parse_phandle_with_args(const struct device_node *np, const char *list_na if (index < 0) return -EINVAL; return __of_parse_phandle_with_args(np, list_name, cells_name, 0, - index, out_args); + index, out_args, MAX_PHANDLE_ARGS); } EXPORT_SYMBOL(of_parse_phandle_with_args); +int of_parse_phandle_with_var_args(const struct device_node *np, const char *list_name, + const char *cells_name, int index, + struct of_phandle_args *out_args, int elems) +{ + if (index < 0) + return -EINVAL; + return __of_parse_phandle_with_args(np, list_name, cells_name, 0, + index, out_args, elems); +} +EXPORT_SYMBOL(of_parse_phandle_with_var_args); /** * of_parse_phandle_with_fixed_args() - Find a node pointed by phandle in a list * @np: pointer to a device tree node containing a list @@ -1664,7 +1693,7 @@ int of_parse_phandle_with_fixed_args(const struct device_node *np, if (index < 0) return -EINVAL; return __of_parse_phandle_with_args(np, list_name, NULL, cell_count, - index, out_args); + index, out_args, MAX_PHANDLE_ARGS); } EXPORT_SYMBOL(of_parse_phandle_with_fixed_args); @@ -1687,7 +1716,7 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na const char *cells_name) { return __of_parse_phandle_with_args(np, list_name, cells_name, 0, -1, - NULL); + NULL, MAX_PHANDLE_ARGS); } EXPORT_SYMBOL(of_count_phandle_with_args); diff --git a/include/linux/of.h b/include/linux/of.h index edc068d..cd9f225 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -75,6 +75,10 @@ struct of_phandle_args { uint32_t args[MAX_PHANDLE_ARGS]; }; +/* Use these if you need more that MAX_PHANDLE_ARGS */ +extern struct of_phandle_args *of_alloc_phandle_args(int size); +extern void of_free_phandle_args(struct of_phandle_args *args); + struct of_reconfig_data { struct device_node *dn; struct property *prop; @@ -327,6 +331,9 @@ extern struct device_node *of_parse_phandle(const struct device_node *np, extern int of_parse_phandle_with_args(const struct device_node *np, const char *list_name, const char *cells_name, int index, struct of_phandle_args *out_args); +extern int of_parse_phandle_with_var_args(const struct device_node *np, + const char *list_name, const char *cells_name, int index, + struct of_phandle_args *out_args, int elems); extern int of_parse_phandle_with_fixed_args(const struct device_node *np, const char *list_name, int cells_count, int index, struct of_phandle_args *out_args);