* [PATCH v2] pinctrl-single: fix pcs_parse_pinconf() return value
@ 2020-06-08 12:51 Drew Fustini
2020-06-09 18:06 ` Tony Lindgren
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Drew Fustini @ 2020-06-08 12:51 UTC (permalink / raw)
To: Tony Lindgren, Haojian Zhuang, Linus Walleij, linux-arm-kernel,
linux-omap, linux-gpio, linux-kernel, Grygorii Strashko
Cc: Jason Kridner, Robert Nelson
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>
---
changes from V1 [0]:
- if pcs_parse_pinconf() returns -ENOSUPP, then set num_maps to 1 and
proceed normally as it is valid for group to have no pinconf props
- added Fixes: tag thanks to Gustavo A. R. Silva
[0] https://lore.kernel.org/linux-omap/20200531204147.GA664833@x1/
drivers/pinctrl/pinctrl-single.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 1e0614daee9b..a9d511982780 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -916,7 +916,7 @@ static int pcs_parse_pinconf(struct pcs_device *pcs, struct device_node *np,
/* If pinconf isn't supported, don't parse properties in below. */
if (!PCS_HAS_PINCONF)
- return 0;
+ return -ENOTSUPP;
/* cacluate how much properties are supported in current node */
for (i = 0; i < ARRAY_SIZE(prop2); i++) {
@@ -928,7 +928,7 @@ static int pcs_parse_pinconf(struct pcs_device *pcs, struct device_node *np,
nconfs++;
}
if (!nconfs)
- return 0;
+ return -ENOTSUPP;
func->conf = devm_kcalloc(pcs->dev,
nconfs, sizeof(struct pcs_conf_vals),
@@ -1056,9 +1056,12 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
if (PCS_HAS_PINCONF && function) {
res = pcs_parse_pinconf(pcs, np, function, map);
- if (res)
+ if (res == 0)
+ *num_maps = 2;
+ else if (res == -ENOTSUPP)
+ *num_maps = 1;
+ else
goto free_pingroups;
- *num_maps = 2;
} else {
*num_maps = 1;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] pinctrl-single: fix pcs_parse_pinconf() return value
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-07-05 8:58 ` Haojian Zhuang
2 siblings, 0 replies; 6+ messages in thread
From: Tony Lindgren @ 2020-06-09 18:06 UTC (permalink / raw)
To: Drew Fustini
Cc: Haojian Zhuang, Linus Walleij, linux-arm-kernel, linux-omap,
linux-gpio, linux-kernel, Grygorii Strashko, Jason Kridner,
Robert Nelson
* Drew Fustini <drew@beagleboard.org> [200608 12:52]:
> 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.
...
> Fixes: 9dddb4df90d1 ("pinctrl: single: support generic pinconf")
It would be good to get an ack/tested-by from Haojian for this.
The patch looks right to me:
Acked-by: Tony Lindgren <tony@atomide.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] pinctrl-single: fix pcs_parse_pinconf() return value
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-07-05 8:58 ` Haojian Zhuang
2 siblings, 1 reply; 6+ messages in thread
From: Linus Walleij @ 2020-06-16 8:31 UTC (permalink / raw)
To: Drew Fustini
Cc: Tony Lindgren, Haojian Zhuang, Linux ARM, Linux-OMAP,
open list:GPIO SUBSYSTEM, linux-kernel, Grygorii Strashko,
Jason Kridner, Robert Nelson
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] pinctrl-single: fix pcs_parse_pinconf() return value
2020-06-16 8:31 ` Linus Walleij
@ 2020-06-16 11:59 ` Drew Fustini
2020-06-26 19:19 ` Drew Fustini
0 siblings, 1 reply; 6+ messages in thread
From: Drew Fustini @ 2020-06-16 11:59 UTC (permalink / raw)
To: Linus Walleij
Cc: Tony Lindgren, Haojian Zhuang, Linux ARM, Linux-OMAP,
open list:GPIO SUBSYSTEM, linux-kernel, Grygorii Strashko,
Jason Kridner, Robert Nelson, Robert Jarzmik, Daniel Mack
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?
Haojian added the generic pinconf support back in 2013, so it would be
good to get his review.
Also, neither Tony or I have the Hikey hardware to test.
The most important to test would be those which include a .dtsi with
"pinconf-single" compatible. I do plan to add pinconf-single to the
AM3358 BeagleBone (which is what I am testing on), but the current
mainline users are:
arch/arm/boot/dts/hi3620.dtsi
|- arch/arm/boot/dts/hi3620-hi4511.dts
arch/arm/boot/dts/pxa3xx.dtsi
|- arch/arm/boot/dts/pxa300-raumfeld-common.dtsi
|- arch/arm/boot/dts/pxa300-raumfeld-connector.dts
arch/arm/boot/dts/pxa300-raumfeld-controller.dts
arch/arm/boot/dts/pxa300-raumfeld-speaker-l.dts
arch/arm/boot/dts/pxa300-raumfeld-speaker-m.dts
arch/arm/boot/dts/pxa300-raumfeld-speaker-one.dts
arch/arm/boot/dts/pxa300-raumfeld-speaker-s.dts
I am cc'ing Daniel Mack and Robert Jarzmi as they are listed as
maintainers for the pxa300 related dts files.
Those platforms using compatible = "pinctrl-single" should not be
effected by this patch:
arch/arm/boot/dts/am33xx-l4.dtsi (I have changed to pinconf for test)
arch/arm/boot/dts/da850.dtsi
arch/arm/boot/dts/dm814x.dtsi
arch/arm/boot/dts/dm816x.dtsi
arch/arm/boot/dts/hi3620.dtsi
arch/arm/boot/dts/keystone-k2g.dtsi
arch/arm/boot/dts/keystone-k2l.dtsi
arch/arm/boot/dts/omap3-gta04.dtsi
Thanks,
Drew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] pinctrl-single: fix pcs_parse_pinconf() return value
2020-06-16 11:59 ` Drew Fustini
@ 2020-06-26 19:19 ` Drew Fustini
0 siblings, 0 replies; 6+ messages in thread
From: Drew Fustini @ 2020-06-26 19:19 UTC (permalink / raw)
To: Linus Walleij
Cc: Tony Lindgren, Haojian Zhuang, Linux ARM, Linux-OMAP,
open list:GPIO SUBSYSTEM, linux-kernel, Grygorii Strashko,
Jason Kridner, Robert Nelson, Robert Jarzmik, Daniel Mack
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] pinctrl-single: fix pcs_parse_pinconf() return value
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-07-05 8:58 ` Haojian Zhuang
2 siblings, 0 replies; 6+ messages in thread
From: Haojian Zhuang @ 2020-07-05 8:58 UTC (permalink / raw)
To: Drew Fustini
Cc: Tony Lindgren, Linus Walleij, linux-arm-kernel, linux-omap,
linux-gpio, linux-kernel, Grygorii Strashko, Jason Kridner,
Robert Nelson
On Mon, 8 Jun 2020 at 20:51, 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>
> ---
> changes from V1 [0]:
> - if pcs_parse_pinconf() returns -ENOSUPP, then set num_maps to 1 and
> proceed normally as it is valid for group to have no pinconf props
> - added Fixes: tag thanks to Gustavo A. R. Silva
>
> [0] https://lore.kernel.org/linux-omap/20200531204147.GA664833@x1/
>
> drivers/pinctrl/pinctrl-single.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> index 1e0614daee9b..a9d511982780 100644
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -916,7 +916,7 @@ static int pcs_parse_pinconf(struct pcs_device *pcs, struct device_node *np,
>
> /* If pinconf isn't supported, don't parse properties in below. */
> if (!PCS_HAS_PINCONF)
> - return 0;
> + return -ENOTSUPP;
>
> /* cacluate how much properties are supported in current node */
> for (i = 0; i < ARRAY_SIZE(prop2); i++) {
> @@ -928,7 +928,7 @@ static int pcs_parse_pinconf(struct pcs_device *pcs, struct device_node *np,
> nconfs++;
> }
> if (!nconfs)
> - return 0;
> + return -ENOTSUPP;
>
> func->conf = devm_kcalloc(pcs->dev,
> nconfs, sizeof(struct pcs_conf_vals),
> @@ -1056,9 +1056,12 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
>
> if (PCS_HAS_PINCONF && function) {
> res = pcs_parse_pinconf(pcs, np, function, map);
> - if (res)
> + if (res == 0)
> + *num_maps = 2;
> + else if (res == -ENOTSUPP)
> + *num_maps = 1;
> + else
> goto free_pingroups;
> - *num_maps = 2;
> } else {
> *num_maps = 1;
> }
> --
> 2.25.1
Tested-by: Haojian Zhuang <haojian.zhuang@linaro.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-05 8:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2020-07-05 8:58 ` Haojian Zhuang
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).