| 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 | expand |
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.
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
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);
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.