linux-kernel - Re: [PATCH V2] mfd: replace strict

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VeFW1PWsPoothDvQrMXYvUXzjBkf5akV0ZPXtmnzbMLYw@mail.gmail.com>
Date:	Mon, 3 Jun 2013 13:30:38 +0300
From:	Andy Shevchenko <andy.shevchenko@...il.com>
To:	Jingoo Han <jg1.han@...sung.com>
Cc:	Samuel Ortiz <sameo@...ux.intel.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Lee Jones <lee.jones@...aro.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Srinidhi Kasagar <srinidhi.kasagar@...ricsson.com>
Subject: Re: [PATCH V2] mfd: replace strict_strtoul() with kstrtoul()

On Mon, Jun 3, 2013 at 12:04 PM, Jingoo Han <jg1.han@...sung.com> wrote:
> The usage of strict_strtoul() is not preferred, because
> strict_strtoul() is obsolete. Thus, kstrtoul() should be
> used.

Few comments below.
After addressing them take my
Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>

> --- a/drivers/mfd/ab3100-core.c
> +++ b/drivers/mfd/ab3100-core.c

> @@ -514,22 +514,20 @@ static ssize_t ab3100_get_set_reg(struct file *file,
>         /*
>          * Advance pointer to end of string then terminate
>          * the register string. This is needed to satisfy
> -        * the strict_strtoul() function.
> +        * the kstrtou8() function.
>          */
>         while ((i < buf_size) && (buf[i] != ' '))
>                 i++;
>         buf[i] = '\0';
>
> -       err = strict_strtoul(&buf[regp], 16, &user_reg);
> +       err = kstrtou8(&buf[regp], 16, &user_reg);
>         if (err)
>                 return err;
> -       if (user_reg > 0xff)
> -               return -EINVAL;
>
>         /* Either we read or we write a register here */
>         if (!priv->mode) {
>                 /* Reading */
> -               u8 reg = (u8) user_reg;
> +               u8 reg = user_reg;

Since the reg and user_reg have same type I think no harm to drop away
this local reg variable.

>                 u8 regvalue;
>
>                 ab3100_get_register_interruptible(ab3100, reg, &regvalue);
> @@ -540,7 +538,7 @@ static ssize_t ab3100_get_set_reg(struct file *file,
>         } else {
>                 int valp;
>                 unsigned long user_value;
> -               u8 reg = (u8) user_reg;
> +               u8 reg = user_reg;

Same idea.

>                 u8 value;
>                 u8 regvalue;
>
> @@ -557,11 +555,9 @@ static ssize_t ab3100_get_set_reg(struct file *file,
>                         i++;
>                 buf[i] = '\0';
>
> -               err = strict_strtoul(&buf[valp], 16, &user_value);
> +               err = kstrtoul(&buf[valp], 16, &user_value);

kstrtou8() as well (see below)

>                 if (err)
>                         return err;
> -               if (user_reg > 0xff)
> -                       return -EINVAL;

It seems a bug, and further the u8 is used for value as well.

>                 value = (u8) user_value;

Thus, you can easily use u8 type for user_value and drop off val variable.

--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/