This patch causes pcs_parse_pinconf() to return an error 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 non-zero. 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.
[0] https://lore.kernel.org/linux-omap/20200529175544.GA3766151@x1/
Signed-off-by: Drew Fustini <redacted>
---
drivers/pinctrl/pinctrl-single.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -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)-return0;+return-ENOTSUPP;/* do not return 0 as no map added *//* 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)-return0;+return-ENOTSUPP;/* do not return 0 as no map added */func->conf=devm_kcalloc(pcs->dev,nconfs,sizeof(structpcs_conf_vals),
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Tony Lindgren <tony@atomide.com> Date: 2020-06-01 16:18:56
* Drew Fustini [off-list ref] [200531 20:42]:
This patch causes pcs_parse_pinconf() to return an error 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.
Looks OK to me, would be good to wait for Haojian to test this one.
Regards,
Tony
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Jun 01, 2020 at 09:18:51AM -0700, Tony Lindgren wrote:
* Drew Fustini [off-list ref] [200531 20:42]:
quoted
This patch causes pcs_parse_pinconf() to return an error 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.
Looks OK to me, would be good to wait for Haojian to test this one.
Regards,
Tony
Yes, I would like to get input as I don't have the other platforms using
"pinconf,single":
$ git grep 'compatible = "pinconf-single"' arch/
arch/arm/boot/dts/am33xx-l4.dtsi: compatible = "pinconf-single";
arch/arm/boot/dts/hi3620.dtsi: compatible = "pinconf-single";
arch/arm/boot/dts/pxa3xx.dtsi: compatible = "pinconf-single";
arch/arm64/boot/dts/broadcom/stingray/stingray-pinctrl.dtsi: compatible = "pinconf-single";
arch/arm64/boot/dts/hisilicon/hi3798cv200.dtsi: compatible = "pinconf-single";
arch/arm64/boot/dts/hisilicon/hi6220.dtsi: compatible = "pinconf-single";
arch/arm64/boot/dts/hisilicon/hi6220.dtsi: compatible = "pinconf-single";
arch/arm64/boot/dts/hisilicon/hikey960-pinctrl.dtsi: compatible = "pinconf-single";
arch/arm64/boot/dts/hisilicon/hikey960-pinctrl.dtsi: compatible = "pinconf-single";
arch/arm64/boot/dts/hisilicon/hikey960-pinctrl.dtsi: compatible = "pinconf-single";
arch/arm64/boot/dts/hisilicon/hikey960-pinctrl.dtsi: compatible = "pinconf-single";
arch/arm64/boot/dts/hisilicon/hikey960-pinctrl.dtsi: compatible = "pinconf-single";
arch/arm64/boot/dts/hisilicon/hikey970-pinctrl.dtsi: compatible = "pinconf-single";
arch/arm64/boot/dts/hisilicon/hikey970-pinctrl.dtsi: compatible = "pinconf-single";
arch/arm64/boot/dts/hisilicon/hikey970-pinctrl.dtsi: compatible = "pinconf-single";
arch/arm64/boot/dts/hisilicon/hikey970-pinctrl.dtsi: compatible = "pinconf-single";
NOTE: the arch/arm/boot/dts/am33xx-l4.dtsi was patched by me from
"pinctrl-single" to "pinconf-single. But, I think for upstream
submission I would need to move that to one of the beaglebone specific
dts files like am335x-bone-common.dtsi.
I believe this pinctrl-single.c patch fixes a flaw in return logic and
is useful regardless of whether beaglebone adopts "pinconf,single".
However, I would very much like to get input from others in case my
analysis is too narrow.
Thanks,
Drew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Gustavo A. R. Silva <hidden> Date: 2020-06-01 19:10:39
On 5/31/20 15:41, Drew Fustini wrote:
This patch causes pcs_parse_pinconf() to return an error 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 non-zero. 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.
[0] https://lore.kernel.org/linux-omap/20200529175544.GA3766151@x1/
Maybe add this Fixes tag here:
Fixes: 9dddb4df90d1 ("pinctrl: single: support generic pinconf")
--
Gustavo
@@ -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)-return0;+return-ENOTSUPP;/* do not return 0 as no map added *//* 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)-return0;+return-ENOTSUPP;/* do not return 0 as no map added */func->conf=devm_kcalloc(pcs->dev,nconfs,sizeof(structpcs_conf_vals),