| Message ID | 20180226120418.3589-1-colin.king@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: imx: fix unsigned check if nfuncs with less than or equal zero | expand |
On Mon, Feb 26, 2018 at 9:04 AM, Colin King <colin.king@canonical.com> wrote: > From: Colin Ian King <colin.king@canonical.com> > > The unsigned integer nfuncs is being error checked with a value less > or equal to zero; this is always false if of_get_child_count returns > a -ve for an error condition since nfuncs is not signed. Fix this by > making variables nfuncs and i signed integers. > > Detected with Coccinelle: > drivers/pinctrl/freescale/pinctrl-imx.c:620:6-12: WARNING: Unsigned > expression compared with zero: nfuncs <= 0 > > Fixes: ae75ff814538 ("pinctrl: pinctrl-imx: add imx pinctrl core driver") > Signed-off-by: Colin Ian King <colin.king@canonical.com> Reviewed-by: Fabio Estevam <festevam@gmail.com> -- 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
> -----Original Message----- > From: Colin King [mailto:colin.king@canonical.com] > Sent: Monday, February 26, 2018 8:04 PM > To: A.s. Dong <aisheng.dong@nxp.com>; Fabio Estevam > <festevam@gmail.com>; Shawn Guo <shawnguo@kernel.org>; Stefan > Agner <stefan@agner.ch>; inus Walleij <linus.walleij@linaro.org>; linux- > gpio@vger.kernel.org > Cc: kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: [PATCH] pinctrl: imx: fix unsigned check if nfuncs with less than or > equal zero > > From: Colin Ian King <colin.king@canonical.com> > > The unsigned integer nfuncs is being error checked with a value less or equal > to zero; this is always false if of_get_child_count returns a -ve for an error > condition since nfuncs is not signed. Fix this by making variables nfuncs and i > signed integers. > > Detected with Coccinelle: > drivers/pinctrl/freescale/pinctrl-imx.c:620:6-12: WARNING: Unsigned > expression compared with zero: nfuncs <= 0 > > Fixes: ae75ff814538 ("pinctrl: pinctrl-imx: add imx pinctrl core driver") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/pinctrl/freescale/pinctrl-imx.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c > b/drivers/pinctrl/freescale/pinctrl-imx.c > index 24aaddd760a0..1e8ca83352d0 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c > @@ -605,8 +605,8 @@ static int imx_pinctrl_probe_dt(struct > platform_device *pdev, > struct device_node *np = pdev->dev.of_node; > struct device_node *child; > struct pinctrl_dev *pctl = ipctl->pctl; > - u32 nfuncs = 0; > - u32 i = 0; > + int nfuncs = 0; > + int i = 0; > bool flat_funcs; > I saw 'i', later used, is converted to u32 unconditionally. (GCC did not complain) e.g. radix_tree_insert imx_pinctrl_parse_functions And of_get_child_count seems can't return a minor value. So does something like below look better? diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c index c976ffe..4259209 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx.c +++ b/drivers/pinctrl/freescale/pinctrl-imx.c @@ -612,7 +612,7 @@ static int imx_pinctrl_probe_dt(struct platform_device *pdev, nfuncs = 1; } else { nfuncs = of_get_child_count(np); - if (nfuncs <= 0) { + if (nfuncs == 0) { dev_err(&pdev->dev, "no functions defined\n"); return -EINVAL; } Regards Dong Aisheng > if (!np) > -- > 2.15.1
On Mon, Feb 26, 2018 at 2:14 PM, A.s. Dong <aisheng.dong@nxp.com> wrote: >> - u32 nfuncs = 0; >> - u32 i = 0; >> + int nfuncs = 0; >> + int i = 0; >> bool flat_funcs; >> > > I saw 'i', later used, is converted to u32 unconditionally. (GCC did not complain) > e.g. > radix_tree_insert > imx_pinctrl_parse_functions > > And of_get_child_count seems can't return a minor value. > > So does something like below look better? > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c > index c976ffe..4259209 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c > @@ -612,7 +612,7 @@ static int imx_pinctrl_probe_dt(struct platform_device *pdev, > nfuncs = 1; > } else { > nfuncs = of_get_child_count(np); > - if (nfuncs <= 0) { > + if (nfuncs == 0) { This makes more sense to me. You can have zero functions but not negative number of functions. Aisheng, can you send a patch like this? (Or is it already somewhere in my inbox?) 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
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c index 24aaddd760a0..1e8ca83352d0 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx.c +++ b/drivers/pinctrl/freescale/pinctrl-imx.c @@ -605,8 +605,8 @@ static int imx_pinctrl_probe_dt(struct platform_device *pdev, struct device_node *np = pdev->dev.of_node; struct device_node *child; struct pinctrl_dev *pctl = ipctl->pctl; - u32 nfuncs = 0; - u32 i = 0; + int nfuncs = 0; + int i = 0; bool flat_funcs; if (!np)