From: Drew Fustini <drew@beagleboard.org>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Tony Lindgren <tony@atomide.com>,
Haojian Zhuang <haojian.zhuang@linaro.org>,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
Linux-OMAP <linux-omap@vger.kernel.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Grygorii Strashko <grygorii.strashko@ti.com>,
Jason Kridner <jkridner@beagleboard.org>,
Robert Nelson <robertcnelson@beagleboard.org>,
Robert Jarzmik <robert.jarzmik@free.fr>,
Daniel Mack <daniel@zonque.org>
Subject: Re: [PATCH v2] pinctrl-single: fix pcs_parse_pinconf() return value
Date: Fri, 26 Jun 2020 21:19:46 +0200 [thread overview]
Message-ID: <20200626191946.GA97666@x1> (raw)
In-Reply-To: <20200616115951.GA3976568@x1>
On Tue, Jun 16, 2020 at 01:59:51PM +0200, Drew Fustini wrote:
> On Tue, Jun 16, 2020 at 10:31:54AM +0200, Linus Walleij wrote:
> > On Mon, Jun 8, 2020 at 2:51 PM Drew Fustini <drew@beagleboard.org> wrote:
> >
> > > This patch causes pcs_parse_pinconf() to return -ENOTSUPP when no
> > > pinctrl_map is added. The current behavior is to return 0 when
> > > !PCS_HAS_PINCONF or !nconfs. Thus pcs_parse_one_pinctrl_entry()
> > > incorrectly assumes that a map was added and sets num_maps = 2.
> > >
> > > Analysis:
> > > =========
> > > The function pcs_parse_one_pinctrl_entry() calls pcs_parse_pinconf()
> > > if PCS_HAS_PINCONF is enabled. The function pcs_parse_pinconf()
> > > returns 0 to indicate there was no error and num_maps is then set to 2:
> > >
> > > 980 static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
> > > 981 struct device_node *np,
> > > 982 struct pinctrl_map **map,
> > > 983 unsigned *num_maps,
> > > 984 const char **pgnames)
> > > 985 {
> > > <snip>
> > > 1053 (*map)->type = PIN_MAP_TYPE_MUX_GROUP;
> > > 1054 (*map)->data.mux.group = np->name;
> > > 1055 (*map)->data.mux.function = np->name;
> > > 1056
> > > 1057 if (PCS_HAS_PINCONF && function) {
> > > 1058 res = pcs_parse_pinconf(pcs, np, function, map);
> > > 1059 if (res)
> > > 1060 goto free_pingroups;
> > > 1061 *num_maps = 2;
> > > 1062 } else {
> > > 1063 *num_maps = 1;
> > > 1064 }
> > >
> > > However, pcs_parse_pinconf() will also return 0 if !PCS_HAS_PINCONF or
> > > !nconfs. I believe these conditions should indicate that no map was
> > > added by returning -ENOTSUPP. Otherwise pcs_parse_one_pinctrl_entry()
> > > will set num_maps = 2 even though no maps were successfully added, as
> > > it does not reach "m++" on line 940:
> > >
> > > 895 static int pcs_parse_pinconf(struct pcs_device *pcs, struct device_node *np,
> > > 896 struct pcs_function *func,
> > > 897 struct pinctrl_map **map)
> > > 898
> > > 899 {
> > > 900 struct pinctrl_map *m = *map;
> > > <snip>
> > > 917 /* If pinconf isn't supported, don't parse properties in below. */
> > > 918 if (!PCS_HAS_PINCONF)
> > > 919 return 0;
> > > 920
> > > 921 /* cacluate how much properties are supported in current node */
> > > 922 for (i = 0; i < ARRAY_SIZE(prop2); i++) {
> > > 923 if (of_find_property(np, prop2[i].name, NULL))
> > > 924 nconfs++;
> > > 925 }
> > > 926 for (i = 0; i < ARRAY_SIZE(prop4); i++) {
> > > 927 if (of_find_property(np, prop4[i].name, NULL))
> > > 928 nconfs++;
> > > 929 }
> > > 930 if (!nconfs)
> > > 919 return 0;
> > > 932
> > > 933 func->conf = devm_kcalloc(pcs->dev,
> > > 934 nconfs, sizeof(struct pcs_conf_vals),
> > > 935 GFP_KERNEL);
> > > 936 if (!func->conf)
> > > 937 return -ENOMEM;
> > > 938 func->nconfs = nconfs;
> > > 939 conf = &(func->conf[0]);
> > > 940 m++;
> > >
> > > This situtation will cause a boot failure [0] on the BeagleBone Black
> > > (AM3358) when am33xx_pinmux node in arch/arm/boot/dts/am33xx-l4.dtsi
> > > has compatible = "pinconf-single" instead of "pinctrl-single".
> > >
> > > The patch fixes this issue by returning -ENOSUPP when !PCS_HAS_PINCONF
> > > or !nconfs, so that pcs_parse_one_pinctrl_entry() will know that no
> > > map was added.
> > >
> > > Logic is also added to pcs_parse_one_pinctrl_entry() to distinguish
> > > between -ENOSUPP and other errors. In the case of -ENOSUPP, num_maps
> > > is set to 1 as it is valid for pinconf to be enabled and a given pin
> > > group to not any pinconf properties.
> > >
> > > [0] https://lore.kernel.org/linux-omap/20200529175544.GA3766151@x1/
> > >
> > > Fixes: 9dddb4df90d1 ("pinctrl: single: support generic pinconf")
> > > Signed-off-by: Drew Fustini <drew@beagleboard.org>
> >
> > Patch applied as non-critical (for-next) fix.
> >
> > If there is no hurry let's merge it this way with lots of testing
> > along the way.
> >
> > Yours,
> > Linus Walleij
>
> Thanks, I agree more testing is a good idea.
>
> In particular, do you have a way to followup with Haojian Zhuang within
> Linaro?
>
Linus - do you have a way to contact Haojian Zhuan?
I found them on Freenode but they have been idle since beginning of
June. Last email I can find on the mailing list is from March.
Tony and I would both like someone familiar with Hisilicon to comment on
this patch as it is using pinctrl-single.
Thanks,
Drew
next prev parent reply other threads:[~2020-06-26 19:19 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-08 12:51 [PATCH v2] pinctrl-single: fix pcs_parse_pinconf() return value Drew Fustini
2020-06-09 18:06 ` Tony Lindgren
2020-06-16 8:31 ` Linus Walleij
2020-06-16 11:59 ` Drew Fustini
2020-06-26 19:19 ` Drew Fustini [this message]
2020-07-05 8:58 ` Haojian Zhuang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200626191946.GA97666@x1 \
--to=drew@beagleboard.org \
--cc=daniel@zonque.org \
--cc=grygorii.strashko@ti.com \
--cc=haojian.zhuang@linaro.org \
--cc=jkridner@beagleboard.org \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=robert.jarzmik@free.fr \
--cc=robertcnelson@beagleboard.org \
--cc=tony@atomide.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).