On Thu, Jun 07 2018, Sergio Paracuellos wrote: > Using 'function' and 'groups' bindings in the device tree give the > posibility of refactor 'rt2880_pinctrl_dt_node_to_map' and simplify > it a lot. Make use of the 'of_property_count_strings' function to get > number of groups for the node and iterate over the groups using > 'of_property_for_each_string' calling 'pinctrl_utils_add_map_mux' > function which is the same of the custom function in this driver code > 'rt2880_pinctrl_dt_subnode_to_map' which is not needed anymore. > > Signed-off-by: Sergio Paracuellos <sergio.paracuel...@gmail.com>
Hi Sergio,
I've just noticed a problem with this code. The problem existed before
your patch - you didn't introduce it. But maybe you are more familiar
with the code and might like to fix it...
> ---
> drivers/staging/mt7621-pinctrl/pinctrl-rt2880.c | 60
> ++++++++-----------------
> 1 file changed, 18 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/staging/mt7621-pinctrl/pinctrl-rt2880.c
> b/drivers/staging/mt7621-pinctrl/pinctrl-rt2880.c
> index 84494a1..8291ec0 100644
> --- a/drivers/staging/mt7621-pinctrl/pinctrl-rt2880.c
> +++ b/drivers/staging/mt7621-pinctrl/pinctrl-rt2880.c
...
> static int rt2880_pinctrl_dt_node_to_map(struct pinctrl_dev *pctrldev,
> struct device_node *np_config,
> struct pinctrl_map **map,
> unsigned int *num_maps)
num_maps passed in ( from dt_to_map_one_config in
drivers/pinctrl/devicetree.c)
is the address of an uninitialised variable - has the value -1681671408 in one
test
run.
> {
> struct rt2880_priv *p = pinctrl_dev_get_drvdata(pctrldev);
> + struct property *prop;
> + const char *function_name, *group_name;
> int ret;
> - int max_maps = 0;
> + int ngroups;
> unsigned int reserved_maps = 0;
> - struct pinctrl_map *tmp;
> - struct device_node *np;
>
> - for_each_child_of_node(np_config, np) {
> - int ret = of_property_count_strings(np, "ralink,group");
> -
> - if (ret >= 0)
> - max_maps += ret;
> + ngroups = of_property_count_strings(np_config, "groups");
> + if (!ngroups) {
> + dev_err(p->dev, "missing groups property in node %s\n",
> + np_config->name);
> + return -EINVAL;
> }
>
> - if (!max_maps)
> - return max_maps;
> -
> ret = pinctrl_utils_reserve_map(pctrldev, map, &reserved_maps,
> - num_maps, max_maps);
> + num_maps, ngroups);
pinctrl_utils_reserve_map() expects *num_maps to be initialized here.
It does a memory allocation based on the value, and triggers a warning
for me:
if (unlikely(order >= MAX_ORDER)) {
WARN_ON_ONCE(!(gfp_mask & __GFP_NOWARN));
return NULL;
}
> if (ret) {
> dev_err(p->dev, "can't reserve map: %d\n", ret);
> return ret;
> }
>
> - tmp = *map;
> -
> - for_each_child_of_node(np_config, np)
> - rt2880_pinctrl_dt_subnode_to_map(pctrldev, np, &tmp);
> - *num_maps = max_maps;
Previously *num_maps was initialised here - clearly too late. Now it
isn't initialised at all.
Do you know what value it should be set to?
Thanks,
NeilBrown
signature.asc
Description: PGP signature
_______________________________________________ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
