pinctrl: imx: fix unsigned check if nfuncs with less than or equal zero
diff mbox series

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
Related show

Commit Message

Colin King Feb. 26, 2018, 12:04 p.m. UTC
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(-)

Comments

Fabio Estevam Feb. 26, 2018, 12:43 p.m. UTC | #1
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
Dong Aisheng Feb. 26, 2018, 1:14 p.m. UTC | #2
> -----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
Linus Walleij March 21, 2018, 8:49 a.m. UTC | #3
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

Patch
diff mbox series

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)