[PATCH 15/18] staging: mt7621-pinctrl: refactor rt2880_pinctrl_dt_node_to_map function

Sergio Paracuellos sergio.paracuellos at gmail.com
Sun Dec 30 07:37:56 UTC 2018


Hi again,

Sorry, Too early in the morning and I misunderstood the problem :-)

On Sun, Dec 30, 2018 at 8:24 AM Sergio Paracuellos
<sergio.paracuellos at gmail.com> wrote:
>
> Hi Neil,
>
> On Sun, Dec 30, 2018 at 4:58 AM NeilBrown <neil at brown.name> 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 <sergio.paracuellos at 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?

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;
        *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


More information about the devel mailing list