Skip to contentSkip to conversation

pinctrl: pinctrl-imx: fix map setting problem if NO_PAD_CTL

PATCH New Hui Wang 3 participants 5 messages Jun 20, 2012

Subsystems (2)
Arm i.MX and Layerscape Pin control
lore
Share & tools
lore

Hui Wang

author
#0 lore raw

PATCH 1/1 pinctrl: pinctrl-imx: fix map setting problem if NO_PAD_CTL

new_map is allocated according to map_num instead of grp->npins, if a pin or some pins of a group has NO_PAD_CTL property, the map_num and the grp->npin are different definitely.

When we set mapping information to the new_map[], we should skip those pins with NO_PAD_CTL from index, otherwise it is possible the driver will aceesss to a unallocated region.

Cc Dong Aisheng <[email protected]>
Cc Linus Walleij <[email protected]>
Cc Shawn Guo <[email protected]>
Signed-off-by Hui Wang <[email protected]>

---
This problem is easily reproduced if we set pinctrl in the ${board}.dts like following:

fsl,pins = <pin_func_id1   config_with_NO_PAD_CTL
            pin_func_id2   config_with_NO_PAD_CTL
            pin_func_id3   config_normal>;
 drivers/pinctrl/pinctrl-imx.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/pinctrl/pinctrl-imx.c +5 -4
index dd6d93a..0e21abb 100644
@@ -146,6 +146,7 @@ static int imx_dt_node_to_map(struct pinctrl_dev *pctldev,
 	struct pinctrl_map *new_map;
 	struct device_node *parent;
 	int map_num = 1;
+	int map_pos = 0;
 	int i;
 
 	/*

@@ -186,11 +187,11 @@ static int imx_dt_node_to_map(struct pinctrl_dev *pctldev,
 	new_map++;
 	for (i = 0; i < grp->npins; i++) {
 		if (!(grp->configs[i] & IMX_NO_PAD_CTL)) {
-			new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
-			new_map[i].data.configs.group_or_pin =
+			new_map[map_pos].type = PIN_MAP_TYPE_CONFIGS_PIN;
+			new_map[map_pos].data.configs.group_or_pin =
 					pin_get_name(pctldev, grp->pins[i]);
-			new_map[i].data.configs.configs = &grp->configs[i];
-			new_map[i].data.configs.num_configs = 1;
+			new_map[map_pos].data.configs.configs = &grp->configs[i];
+			new_map[map_pos++].data.configs.num_configs = 1;
 		}
 	}
Signature · 1 line
1.7.11
#1 lore raw

PATCH 1/1 pinctrl: pinctrl-imx: fix map setting problem if NO_PAD_CTL

Quoted 4 lines · Hui Wang

new_map is allocated according to map_num instead of grp->npins, if a pin or some pins of a group has NO_PAD_CTL property, the map_num and the grp->npin are different definitely.

Quoted 8 lines

When we set mapping information to the new_map[], we should skip those pins with NO_PAD_CTL from index, otherwise it is possible the driver will aceesss to a unallocated region.

Cc Dong Aisheng <[email protected]>
Cc Linus Walleij <[email protected]>
Cc Shawn Guo <[email protected]>
Signed-off-by Hui Wang <[email protected]>

Song/Shawn: can you check this patch and ACK/NACK it?

Should it go into the -rc:s?

Yours,
Linus Walleij

#2 lore raw

PATCH 1/1 pinctrl: pinctrl-imx: fix map setting problem if NO_PAD_CTL

In reply to Hui Wang
Quoted 8 lines · Hui Wang

new_map is allocated according to map_num instead of grp->npins, if a pin or some pins of a group has NO_PAD_CTL property, the map_num and the grp->npin are different definitely.

When we set mapping information to the new_map[], we should skip those pins with NO_PAD_CTL from index, otherwise it is possible the driver will aceesss to a unallocated region.

Good catching.
Thanks.

Quoted 29 lines
Cc Dong Aisheng <[email protected]>
Cc Linus Walleij <[email protected]>
Cc Shawn Guo <[email protected]>
Signed-off-by Hui Wang <[email protected]>

---
This problem is easily reproduced if we set pinctrl in the ${board}.dts like following:

fsl,pins = <pin_func_id1   config_with_NO_PAD_CTL
            pin_func_id2   config_with_NO_PAD_CTL
            pin_func_id3   config_normal>;
 drivers/pinctrl/pinctrl-imx.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/pinctrl/pinctrl-imx.c +1 -0
index dd6d93a..0e21abb 100644
@@ -146,6 +146,7 @@ static int imx_dt_node_to_map(struct pinctrl_dev *pctldev,
 	struct pinctrl_map *new_map;
 	struct device_node *parent;
 	int map_num = 1;
+	int map_pos = 0;
 	int i;
 
 	/*

@@ -186,11 +187,11 @@ static int imx_dt_node_to_map(struct pinctrl_dev *pctldev,
 	new_map++;
 	for (i = 0; i < grp->npins; i++) {

drivers/pinctrl/pinctrl-imx.c static int imx_dt_node_to_map(struct pinctrl_dev *pctldev, · line 194

Quoted 10 lines
drivers/pinctrl/pinctrl-imx.c +4 -4
 		if (!(grp->configs[i] & IMX_NO_PAD_CTL)) {
-			new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
-			new_map[i].data.configs.group_or_pin =
+			new_map[map_pos].type = PIN_MAP_TYPE_CONFIGS_PIN;
+			new_map[map_pos].data.configs.group_or_pin =
 					pin_get_name(pctldev, grp->pins[i]);
-			new_map[i].data.configs.configs = &grp->configs[i];
-			new_map[i].data.configs.num_configs = 1;
+			new_map[map_pos].data.configs.configs = &grp->configs[i];
+			new_map[map_pos++].data.configs.num_configs = 1;

I'm ok with the change, only a minor comment:
Does the change as follows look better?

- for (i = 0; i < grp->npins; i++) {

+       for (i = j = 0; i < grp->npins; i++) {
                if (!(grp->configs[i] & IMX_NO_PAD_CTL)) {
-                       new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
-                       new_map[i].data.configs.group_or_pin =
+                       new_map[j].type = PIN_MAP_TYPE_CONFIGS_PIN;
+                       new_map[j].data.configs.group_or_pin =
                                        pin_get_name(pctldev, grp->pins[i]);
-                       new_map[i].data.configs.configs = &grp->configs[i];
-                       new_map[i].data.configs.num_configs = 1;
+                       new_map[j].data.configs.configs = &grp->configs[i];
+                       new_map[j].data.configs.num_configs = 1;
+                       j++;

Regards
Dong Aisheng

author
#3 lore raw

PATCH 1/1 pinctrl: pinctrl-imx: fix map setting problem if NO_PAD_CTL

Quoted 49 lines · Dong Aisheng
Quoted 9 lines · Hui Wang

new_map is allocated according to map_num instead of grp->npins, if a pin or some pins of a group has NO_PAD_CTL property, the map_num and the grp->npin are different definitely.

When we set mapping information to the new_map[], we should skip those pins with NO_PAD_CTL from index, otherwise it is possible the driver will aceesss to a unallocated region.

Good catching.
Thanks.

Quoted 34 lines
Cc Dong Aisheng <[email protected]>
Cc Linus Walleij <[email protected]>
Cc Shawn Guo <[email protected]>
Signed-off-by Hui Wang <[email protected]>

---
This problem is easily reproduced if we set pinctrl in the ${board}.dts like following:

fsl,pins = <pin_func_id1   config_with_NO_PAD_CTL
            pin_func_id2   config_with_NO_PAD_CTL
            pin_func_id3   config_normal>;
 drivers/pinctrl/pinctrl-imx.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/pinctrl/pinctrl-imx.c +3 -2
index dd6d93a..0e21abb 100644
@@ -146,6 +146,7 @@ static int imx_dt_node_to_map(struct pinctrl_dev *pctldev,
 	struct pinctrl_map *new_map;
 	struct device_node *parent;
 	int map_num = 1;
+	int map_pos = 0;
 	int i;
 
 	/*

@@ -186,11 +187,11 @@ static int imx_dt_node_to_map(struct pinctrl_dev *pctldev,
 	new_map++;
 	for (i = 0; i < grp->npins; i++) {
 		if (!(grp->configs[i] & IMX_NO_PAD_CTL)) {
-			new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
-			new_map[i].data.configs.group_or_pin =
+			new_map[map_pos].type = PIN_MAP_TYPE_CONFIGS_PIN;
+			new_map[map_pos].data.configs.group_or_pin =
Quoted 9 lines

drivers/pinctrl/pinctrl-imx.c static int imx_dt_node_to_map(struct pinctrl_dev *pctldev, · line 197

Quoted 6 lines
drivers/pinctrl/pinctrl-imx.c +2 -2
 					pin_get_name(pctldev, grp->pins[i]);
-			new_map[i].data.configs.configs = &grp->configs[i];
-			new_map[i].data.configs.num_configs = 1;
+			new_map[map_pos].data.configs.configs = &grp->configs[i];
+			new_map[map_pos++].data.configs.num_configs = 1;

I'm ok with the change, only a minor comment:
Does the change as follows look better?

It is fine to me. Do you need me to prepare a new patch as your suggestion?

Regards,
Hui.

Quoted 19 lines
(quoted diff) +6 -5
-       for (i = 0; i < grp->npins; i++) {
+       for (i = j = 0; i < grp->npins; i++) {
                if (!(grp->configs[i] & IMX_NO_PAD_CTL)) {
-                       new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
-                       new_map[i].data.configs.group_or_pin =
+                       new_map[j].type = PIN_MAP_TYPE_CONFIGS_PIN;
+                       new_map[j].data.configs.group_or_pin =
                                        pin_get_name(pctldev, grp->pins[i]);
-                       new_map[i].data.configs.configs = &grp->configs[i];
-                       new_map[i].data.configs.num_configs = 1;
+                       new_map[j].data.configs.configs = &grp->configs[i];
+                       new_map[j].data.configs.num_configs = 1;
+                       j++;

Regards
Dong Aisheng

author
#4 lore raw

PATCH 1/1 pinctrl: pinctrl-imx: fix map setting problem if NO_PAD_CTL

Quoted 57 lines · Hui Wang
Quoted 48 lines · Dong Aisheng
Quoted 8 lines · Hui Wang

new_map is allocated according to map_num instead of grp->npins, if a pin or some pins of a group has NO_PAD_CTL property, the map_num and the grp->npin are different definitely.

When we set mapping information to the new_map[], we should skip those pins with NO_PAD_CTL from index, otherwise it is possible the driver will aceesss to a unallocated region.

Good catching.
Thanks.

Quoted 36 lines
Cc Dong Aisheng <[email protected]>
Cc Linus Walleij <[email protected]>
Cc Shawn Guo <[email protected]>
Signed-off-by Hui Wang <[email protected]>

---
This problem is easily reproduced if we set pinctrl in the ${board}.dts like following:
fsl,pins = <pin_func_id1 config_with_NO_PAD_CTL pin_func_id2 config_with_NO_PAD_CTL
pin_func_id3 config_normal>;

drivers/pinctrl/pinctrl-imx.c | 9 +++++----

1 file changed, 5 insertions(+), 4 deletions(-)
drivers/pinctrl/pinctrl-imx.c +1 -0
index dd6d93a..0e21abb 100644
@@ -146,6 +146,7 @@ static int imx_dt_node_to_map(struct pinctrl_dev 
 *pctldev,
 struct pinctrl_map *new_map;
 struct device_node *parent;
 int map_num = 1;
+ int map_pos = 0;
 int i;

/*

drivers/pinctrl/pinctrl-imx.c +1 -2
@@ -186,11 +187,11 @@ static int imx_dt_node_to_map(struct 
 pinctrl_dev *pctldev,
 new_map++;
 for (i = 0; i < grp->npins; i++) {
 if (!(grp->configs[i] & IMX_NO_PAD_CTL)) {
- new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
- new_map[i].data.configs.group_or_pin =
+ new_map[map_pos].type = PIN_MAP_TYPE_CONFIGS_PIN;
Quoted 8 lines

drivers/pinctrl/pinctrl-imx.c static int imx_dt_node_to_map(struct pinctrl_dev *pctldev, · line 194

Quoted 6 lines
drivers/pinctrl/pinctrl-imx.c +3 -2
+ new_map[map_pos].data.configs.group_or_pin =
 pin_get_name(pctldev, grp->pins[i]);
- new_map[i].data.configs.configs = &grp->configs[i];
- new_map[i].data.configs.num_configs = 1;
+ new_map[map_pos].data.configs.configs = &grp->configs[i];
+ new_map[map_pos++].data.configs.num_configs = 1;

I'm ok with the change, only a minor comment:
Does the change as follows look better?

Quoted 18 lines

It is fine to me. Do you need me to prepare a new patch as your suggestion?

Regards,
Hui.

Quoted 13 lines

- for (i = 0; i < grp->npins; i++) {
+ for (i = j = 0; i < grp->npins; i++) {
if (!(grp->configs[i] & IMX_NO_PAD_CTL)) {
- new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
- new_map[i].data.configs.group_or_pin =
+ new_map[j].type = PIN_MAP_TYPE_CONFIGS_PIN;
+ new_map[j].data.configs.group_or_pin = pin_get_name(pctldev, grp->pins[i]);
- new_map[i].data.configs.configs = &grp->configs[i];
- new_map[i].data.configs.num_configs = 1;
+ new_map[j].data.configs.configs = &grp->configs[i];
+ new_map[j].data.configs.num_configs = 1;
+ j++;

Changed it in the v2.

Regards,
Hui.

Quoted 12 lines
Quoted 5 lines

Regards
Dong Aisheng

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel