Hi again and sorry for the noise.
On Sun, Dec 30, 2018 at 8:37 AM Sergio Paracuellos
<[email protected]> wrote:
>
> Hi again,
>
> Sorry, Too early in the morning and I misunderstood the problem :-)
>
> On Sun, Dec 30, 2018 at 8:24 AM Sergio Paracuellos
> <[email protected]> wrote:
> >
> > Hi Neil,
> >
> > On Sun, Dec 30, 2018 at 4:58 AM NeilBrown <[email protected]> wrote:
> > >
> > > 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 <[email protected]>
> > >
> > > 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?
>
> So according the code before this PATCH the 'num_max' variable now
> should be set to value of 'ngroups'.
>
> So the following should fix the problem?
>
> diff --git a/drivers/staging/mt7621-pinctrl/pinctrl-rt2880.c
> b/drivers/staging/mt7621-pinctrl/pinctrl-rt2880.c
> index aa98fbb17013..180f2afd7984 100644
> --- a/drivers/staging/mt7621-pinctrl/pinctrl-rt2880.c
> +++ b/drivers/staging/mt7621-pinctrl/pinctrl-rt2880.c
> @@ -88,6 +88,7 @@ static int rt2880_pinctrl_dt_node_to_map(struct
> pinctrl_dev *pctrldev,
> for_each_node_with_property(np_config, "group")
> ngroups++;
>
> + *num_maps = ngroups;
+ *num_maps = 0;
Looking other drivers and 'pinctrl_utils_reserve_map' code it seems
this should be first set to zero.
In that case the number of reserved will be:
new_map = krealloc(*map, sizeof(*new_map) * new_num, GFP_KERNEL);
where new_num is set to:
unsigned new_num = *num_maps + reserve;
which will be 'ngroups' value which is the good one AFAICT.
Best regards,
Sergio Paracuellos
> *map = NULL;
> ret = pinctrl_utils_reserve_map(pctrldev, map, &reserved_maps,
> num_maps, ngroups);
>
> If so let me know and I'll send a real patch with all the fixes tags
> and reported-by stuff.
>
> Thanks,
> Sergio Paracuellos
> >
> > In my staging tree the code is a bit different and the patch you are
> > pointing
> > out obviously is wrong because the value of 'num_maps' should be the number
> > of bindings where the "group" property is set and not the "groups" one
> > which is not
> > a standard binding. So what I have is:
> >
> > 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)
> > {
> > struct rt2880_priv *p = pinctrl_dev_get_drvdata(pctrldev);
> > struct property *prop;
> > const char *function_name, *group_name;
> > int ret;
> > int ngroups = 0;
> > unsigned int reserved_maps = 0;
> >
> > for_each_node_with_property(np_config, "group")
> > ngroups++;
> >
> > *map = NULL;
> > ret = pinctrl_utils_reserve_map(pctrldev, map, &reserved_maps,
> > num_maps, ngroups);
> > if (ret) {
> > dev_err(p->dev, "can't reserve map: %d\n", ret);
> > return ret;
> > }
> >
> > of_property_for_each_string(np_config, "group", prop, group_name) {
> > ret = pinctrl_utils_add_map_mux(pctrldev, map, &reserved_maps,
> > num_maps, group_name,
> > function_name);
> > if (ret) {
> > dev_err(p->dev, "can't add map: %d\n", ret);
> > return ret;
> > }
> > }
> >
> > return 0;
> > }
> >
> > There was a fix, about the ngroups property to be initialized to zero
> > so at least
> > even if there is no bindings defined in the DT the value should be
> > initialized.
> >
> > See:
> > https://lkml.org/lkml/2018/11/11/115
> >
> > Am I missing something?
> >
> > >
> > > Thanks,
> > > NeilBrown
> > >
> >
> > Hope this helps.
> >
> > Best regards,
> > Sergio Paracuellos
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
