pinctrl: imx: Fix imx_dt_node_to_map handling of IMX_NO_PAD_CTL
diff mbox series

Message ID 1547d44dba63cbfd1b813018866f8916013b3381.1541973902.git.leonard.crestez@nxp.com
State New
Headers show
Series
  • pinctrl: imx: Fix imx_dt_node_to_map handling of IMX_NO_PAD_CTL
Related show

Commit Message

Leonard Crestez Nov. 11, 2018, 10:15 p.m. UTC
After changes for SCU support the IMX_NO_PAD_CTL flag is not longer
handled correctly in imx_dt_node_to_map. Pins with this flag are no
longer skipped and the new_map array can overflow and corrupt memory.

This fixes imx6-sabreauto boards failing to boot.

Fixes: b96eea718bf6 ("pinctrl: fsl: add scu based pinctrl support")
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

---
 drivers/pinctrl/freescale/pinctrl-imx.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

A different was posted earlier: https://lore.kernel.org/patchwork/patch/1009504/

I don't think that's correct because it assumes num_configs is
zero-initialized but new_map comes from kmalloc_array.

Code is clearer if SCU and MMIO paths are separate.

Comments

Leonard Crestez Nov. 12, 2018, 12:46 a.m. UTC | #1
On 11/12/2018 12:15 AM, Leonard Crestez wrote:
> After changes for SCU support the IMX_NO_PAD_CTL flag is not longer
> handled correctly in imx_dt_node_to_map. Pins with this flag are no
> longer skipped and the new_map array can overflow and corrupt memory.
> 
> This fixes imx6-sabreauto boards failing to boot.

On a second look this is not sufficient; this just prevents a crash but 
network boot is not successful. After some investigation it seems that 
pin parsing also got broken. Here's a possible fix:

diff --git drivers/pinctrl/freescale/pinctrl-imx.c 
drivers/pinctrl/freescale/pinctrl-imx.c
index 51312e81eff7..2005d31601f9 100644
--- drivers/pinctrl/freescale/pinctrl-imx.c
+++ drivers/pinctrl/freescale/pinctrl-imx.c
@@ -551,10 +551,11 @@ static void imx_pinctrl_parse_pin_mmio(struct 
imx_pinctrl *ipctl,
                 pin_mmio->config = config & ~IMX_PAD_SION;
         }

         dev_dbg(ipctl->dev, "%s: 0x%x 0x%08lx", info->pins[*pin_id].name,
                              pin_mmio->mux_mode, pin_mmio->config);
+       *list_p = list;
  }

  static int imx_pinctrl_parse_groups(struct device_node *np,
                                     struct group_desc *grp,
                                     struct imx_pinctrl *ipctl
It needs a bit more testing.
Aisheng Dong Nov. 12, 2018, 3:33 p.m. UTC | #2
Hi Leonard,

> -----Original Message-----
> From: Leonard Crestez
> Sent: Monday, November 12, 2018 6:15 AM
[...]
> 
> After changes for SCU support the IMX_NO_PAD_CTL flag is not longer handled
> correctly in imx_dt_node_to_map. Pins with this flag are no longer skipped
> and the new_map array can overflow and corrupt memory.
> 
> This fixes imx6-sabreauto boards failing to boot.
> 
> Fixes: b96eea718bf6 ("pinctrl: fsl: add scu based pinctrl support")
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

Please see another more simpler fix just sent out with both you and Martin Kaiser's tags.
Thanks for the effort.

Regards
Dong Aisheng

> 
> ---
>  drivers/pinctrl/freescale/pinctrl-imx.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> A different was posted earlier:
> https://lore.kernel.org/patchwork/patch/1009504/
> 
> I don't think that's correct because it assumes num_configs is zero-initialized
> but new_map comes from kmalloc_array.
> 
> Code is clearer if SCU and MMIO paths are separate.
> 
> diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> b/drivers/pinctrl/freescale/pinctrl-imx.c
> index 78d33dfb4d2d..51312e81eff7 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> @@ -106,29 +106,31 @@ static int imx_dt_node_to_map(struct pinctrl_dev
> *pctldev,
> 
>  	/* create config map */
>  	new_map++;
>  	for (i = j = 0; i < grp->num_pins; i++) {
>  		pin = &((struct imx_pin *)(grp->data))[i];
> -		new_map[j].type = PIN_MAP_TYPE_CONFIGS_PIN;
> -		new_map[j].data.configs.group_or_pin =
> -					pin_get_name(pctldev, pin->pin);
> -
>  		if (info->flags & IMX_USE_SCU) {
>  			/*
>  			 * For SCU case, we set mux and conf together
>  			 * in one IPC call
>  			 */
> +			new_map[j].type = PIN_MAP_TYPE_CONFIGS_PIN;
> +			new_map[j].data.configs.group_or_pin =
> +					pin_get_name(pctldev, pin->pin);
>  			new_map[j].data.configs.configs =
>  					(unsigned long *)&pin->conf.scu;
>  			new_map[j].data.configs.num_configs = 2;
> +			++j;
>  		} else if (!(pin->conf.mmio.config & IMX_NO_PAD_CTL)) {
> +			new_map[j].type = PIN_MAP_TYPE_CONFIGS_PIN;
> +			new_map[j].data.configs.group_or_pin =
> +					pin_get_name(pctldev, pin->pin);
>  			new_map[j].data.configs.configs =
>  					&pin->conf.mmio.config;
>  			new_map[j].data.configs.num_configs = 1;
> +			++j;
>  		}
> -
> -		j++;
>  	}
> 
>  	dev_dbg(pctldev->dev, "maps: function %s group %s num %d\n",
>  		(*map)->data.mux.function, (*map)->data.mux.group, map_num);
> 
> --
> 2.17.1

Patch
diff mbox series

diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
index 78d33dfb4d2d..51312e81eff7 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -106,29 +106,31 @@  static int imx_dt_node_to_map(struct pinctrl_dev *pctldev,
 
 	/* create config map */
 	new_map++;
 	for (i = j = 0; i < grp->num_pins; i++) {
 		pin = &((struct imx_pin *)(grp->data))[i];
-		new_map[j].type = PIN_MAP_TYPE_CONFIGS_PIN;
-		new_map[j].data.configs.group_or_pin =
-					pin_get_name(pctldev, pin->pin);
-
 		if (info->flags & IMX_USE_SCU) {
 			/*
 			 * For SCU case, we set mux and conf together
 			 * in one IPC call
 			 */
+			new_map[j].type = PIN_MAP_TYPE_CONFIGS_PIN;
+			new_map[j].data.configs.group_or_pin =
+					pin_get_name(pctldev, pin->pin);
 			new_map[j].data.configs.configs =
 					(unsigned long *)&pin->conf.scu;
 			new_map[j].data.configs.num_configs = 2;
+			++j;
 		} else if (!(pin->conf.mmio.config & IMX_NO_PAD_CTL)) {
+			new_map[j].type = PIN_MAP_TYPE_CONFIGS_PIN;
+			new_map[j].data.configs.group_or_pin =
+					pin_get_name(pctldev, pin->pin);
 			new_map[j].data.configs.configs =
 					&pin->conf.mmio.config;
 			new_map[j].data.configs.num_configs = 1;
+			++j;
 		}
-
-		j++;
 	}
 
 	dev_dbg(pctldev->dev, "maps: function %s group %s num %d\n",
 		(*map)->data.mux.function, (*map)->data.mux.group, map_num);