| Message ID | 1422399025-25636-1-git-send-email-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
Hello, please disregard the patch here, the email address of Dong Aisheng I used isn't valid any more. I resend with the correct address. Best regards Uwe
On Tue, Jan 27, 2015 at 11:50 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > The right check for conf_reg to be invalid it testing against -1 not 0 > as is done in the rest of the driver. > > This fixes an oops that can be triggered by: > > cat /sys/kernel/debug/pinctrl/43fac000.iomuxc/* > > Fixes: ae75ff814538 ("pinctrl: pinctrl-imx: add imx pinctrl core driver") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Patch applied. But... > - if (!pin_reg || !pin_reg->conf_reg) { > + if (!pin_reg || pin_reg->conf_reg == -1) { I can see that the other code checks for -1 in this variable. But where is conf_reg ever set to -1? freescale]$ git grep "\-1" pinctrl-imx.c: if (pin_reg->mux_reg == -1) { pinctrl-imx.c: if (pin_reg->mux_reg == -1) pinctrl-imx.c: if (pin_reg->mux_reg == -1) pinctrl-imx.c: if (pin_reg->conf_reg == -1) { pinctrl-imx.c: if (pin_reg->conf_reg == -1) { pinctrl-imx.c: if (!pin_reg || pin_reg->conf_reg == -1) { Only checked for, never assigned. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 03, 2015 at 02:01:09PM +0100, Linus Walleij wrote: > On Tue, Jan 27, 2015 at 11:50 PM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > The right check for conf_reg to be invalid it testing against -1 not 0 > > as is done in the rest of the driver. > > > > This fixes an oops that can be triggered by: > > > > cat /sys/kernel/debug/pinctrl/43fac000.iomuxc/* > > > > Fixes: ae75ff814538 ("pinctrl: pinctrl-imx: add imx pinctrl core driver") > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Patch applied. > > But... > > > - if (!pin_reg || !pin_reg->conf_reg) { > > + if (!pin_reg || pin_reg->conf_reg == -1) { > > I can see that the other code checks for -1 in this variable. > But where is conf_reg ever set to -1? > > freescale]$ git grep "\-1" > pinctrl-imx.c: if (pin_reg->mux_reg == -1) { > pinctrl-imx.c: if (pin_reg->mux_reg == -1) > pinctrl-imx.c: if (pin_reg->mux_reg == -1) > pinctrl-imx.c: if (pin_reg->conf_reg == -1) { > pinctrl-imx.c: if (pin_reg->conf_reg == -1) { > pinctrl-imx.c: if (!pin_reg || pin_reg->conf_reg == -1) { Uah, ugly. This member gets -1 by means of: memset(info->pin_regs, 0xff, sizeof(*info->pin_regs) * info->npins); Best regards Uwe
On Tue, Feb 3, 2015 at 8:01 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Tue, Feb 03, 2015 at 02:01:09PM +0100, Linus Walleij wrote: >> On Tue, Jan 27, 2015 at 11:50 PM, Uwe Kleine-König >> <u.kleine-koenig@pengutronix.de> wrote: >> >> > The right check for conf_reg to be invalid it testing against -1 not 0 >> > as is done in the rest of the driver. >> > >> > This fixes an oops that can be triggered by: >> > >> > cat /sys/kernel/debug/pinctrl/43fac000.iomuxc/* >> > >> > Fixes: ae75ff814538 ("pinctrl: pinctrl-imx: add imx pinctrl core driver") >> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >> >> Patch applied. >> >> But... >> >> > - if (!pin_reg || !pin_reg->conf_reg) { >> > + if (!pin_reg || pin_reg->conf_reg == -1) { >> >> I can see that the other code checks for -1 in this variable. >> But where is conf_reg ever set to -1? >> >> freescale]$ git grep "\-1" >> pinctrl-imx.c: if (pin_reg->mux_reg == -1) { >> pinctrl-imx.c: if (pin_reg->mux_reg == -1) >> pinctrl-imx.c: if (pin_reg->mux_reg == -1) >> pinctrl-imx.c: if (pin_reg->conf_reg == -1) { >> pinctrl-imx.c: if (pin_reg->conf_reg == -1) { >> pinctrl-imx.c: if (!pin_reg || pin_reg->conf_reg == -1) { > Uah, ugly. This member gets -1 by means of: > > memset(info->pin_regs, 0xff, sizeof(*info->pin_regs) * info->npins); OMG. Uwe can you make a patch changing that initialization code to something sane? We can't have it like this. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015-02-05 14:53, Linus Walleij wrote: > On Tue, Feb 3, 2015 at 8:01 PM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: >> On Tue, Feb 03, 2015 at 02:01:09PM +0100, Linus Walleij wrote: >>> On Tue, Jan 27, 2015 at 11:50 PM, Uwe Kleine-König >>> <u.kleine-koenig@pengutronix.de> wrote: >>> >>> > The right check for conf_reg to be invalid it testing against -1 not 0 >>> > as is done in the rest of the driver. >>> > >>> > This fixes an oops that can be triggered by: >>> > >>> > cat /sys/kernel/debug/pinctrl/43fac000.iomuxc/* >>> > >>> > Fixes: ae75ff814538 ("pinctrl: pinctrl-imx: add imx pinctrl core driver") This was actually introduce by me in 3dac1918a491 ("pinctrl: imx: detect uninitialized pins"). >>> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >>> >>> Patch applied. >>> >>> But... >>> >>> > - if (!pin_reg || !pin_reg->conf_reg) { >>> > + if (!pin_reg || pin_reg->conf_reg == -1) { >>> >>> I can see that the other code checks for -1 in this variable. >>> But where is conf_reg ever set to -1? >>> >>> freescale]$ git grep "\-1" >>> pinctrl-imx.c: if (pin_reg->mux_reg == -1) { >>> pinctrl-imx.c: if (pin_reg->mux_reg == -1) >>> pinctrl-imx.c: if (pin_reg->mux_reg == -1) >>> pinctrl-imx.c: if (pin_reg->conf_reg == -1) { >>> pinctrl-imx.c: if (pin_reg->conf_reg == -1) { >>> pinctrl-imx.c: if (!pin_reg || pin_reg->conf_reg == -1) { >> Uah, ugly. This member gets -1 by means of: >> >> memset(info->pin_regs, 0xff, sizeof(*info->pin_regs) * info->npins); > > OMG. Uwe can you make a patch changing that initialization > code to something sane? We can't have it like this. Oh sorry, this was actually me, in the commit mentioned above. That's ugly, I agree. I'm wondering how I came to that idea.... Will send a patch for that. -- Stefan -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c index 52f2b9404fe0..448f10986c28 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx.c +++ b/drivers/pinctrl/freescale/pinctrl-imx.c @@ -437,7 +437,7 @@ static void imx_pinconf_dbg_show(struct pinctrl_dev *pctldev, const struct imx_pin_reg *pin_reg = &info->pin_regs[pin_id]; unsigned long config; - if (!pin_reg || !pin_reg->conf_reg) { + if (!pin_reg || pin_reg->conf_reg == -1) { seq_printf(s, "N/A"); return; }
The right check for conf_reg to be invalid it testing against -1 not 0 as is done in the rest of the driver. This fixes an oops that can be triggered by: cat /sys/kernel/debug/pinctrl/43fac000.iomuxc/* Fixes: ae75ff814538 ("pinctrl: pinctrl-imx: add imx pinctrl core driver") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/pinctrl/freescale/pinctrl-imx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)