linux-kernel - Re: [PATCH v2] iio: dac: ad5593r: Dynamically set AD5593R channel modes

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] [day] [month] [year] [list]

Message-ID: <CAHp75VceTBHJ1p3amCQ0PpDSEP8L5+Tf-Qro69+G1WZBrt2oDw@mail.gmail.com>
Date:   Wed, 26 Aug 2020 19:03:46 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     William Sung <william.sung@...antech.com.tw>
Cc:     Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Hartmut Knaack <knaack.h@....de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        AceLan Kao <acelan.kao@...onical.com>,
        Campion Kang <Campion.Kang@...antech.com.tw>
Subject: Re: [PATCH v2] iio: dac: ad5593r: Dynamically set AD5593R channel modes

On Tue, Aug 25, 2020 at 1:19 PM William Sung
<william.sung@...antech.com.tw> wrote:
>
> To use ad5593r more flexibly, we use the module parameter to setting the
> channel modes dynamically whenever the module probe up. Users can pass
> the channel modes to the module parameter for allocating the
> functionality of channels as desired.
>
> For example:
> * Use in the kernel command line:
> Users can add the module parameter in the kernel command line such as
>
>     "ad5593r.ch_mode=88001122"
>
> "88001122" means the channel mode setting for each channel. The most
> left side indicates the mode of channel 7, and the most right side
> indicates the mode of channel 0.
>
> * Use when manually probe the module:
> Similar to the kernel command line usage, users can enter
>
>     "modprobe ad5593r ch_mode=88001122"
>
> to start the ad5593r module with the desired channel mode setting.

> v2: Fix the patch description and remove redundant for loop

This should go after the cutter '---' line below.

> Signed-off-by: William Sung <william.sung@...antech.com.tw>
> ---

...

> +/* Parameters for dynamic channel mode setting */
> +static u8 update_channel_mode;
> +static u8 new_channel_modes[AD559XR_CHANNEL_NR];

Huh?! Global variables?!

...

> +static void ad5592r_set_def_channel_modes(struct ad5592r_state *st)
> +{
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(st->channel_modes); i++)
> +               st->channel_modes[i] = new_channel_modes[i];
> +}

NIH of memcpy() ?

...

> +void ad5592r_update_default_channel_modes(u8 *new_modes)
> +{
> +       int idx = 0;
> +
> +       update_channel_mode = 1;
> +       for (idx = 0; idx < AD559XR_CHANNEL_NR; idx++)
> +               new_channel_modes[idx] = new_modes[idx];

> +

Redundant blank line.

> +}

Ditto (see memcpy() above).

> +EXPORT_SYMBOL_GPL(ad5592r_update_default_channel_modes);

What?!

...

> +/* Parameters for dynamic channel mode setting */
> +static char *ch_mode = "";
> +module_param(ch_mode, charp, 0400);

We have sysfs ABI, what's wrong with it?

...

> +static void ad5593r_check_new_channel_mode(void)
> +{
> +       char *new_mode = NULL, tmp[2];
> +       u8 new_ch_modes[AD559XR_CHANNEL_NR];

> +       int idx = 0;

Redundant assignment.
Also for new_mode.

> +

> +       if (strlen(ch_mode) != AD559XR_CHANNEL_NR)

This is interesting...

> +               return;
> +
> +       new_mode = ch_mode;
> +
> +       /* Check if all channel modes are valid */
> +       for (idx = 0; idx < AD559XR_CHANNEL_NR; idx++) {
> +               switch (new_mode[idx]) {
> +               case '0':
> +               case '1':
> +               case '2':
> +               case '3':
> +               case '8':
> +                       continue;
> +               default:
> +                       /* There is invalid mode exist, ignore the settings */

> +                       pr_err("%s: invalid(%c) in index(%d)\n",
> +                               __func__, new_mode[idx], idx);

Oh...

> +                       return;
> +               }
> +       }
> +
> +       /* Set the new modes to ad5592r-base driver to setup the new channel modes */
> +       memset(tmp, 0, 2);
> +       for (idx = 0; idx < AD559XR_CHANNEL_NR; idx++) {
> +               tmp[0] = new_mode[idx];

> +               if (kstrtou8(tmp, 10, &new_ch_modes[AD559XR_CHANNEL_NR - idx - 1])) {

Shadowing errors?

> +                       /* Something error when converting the string to integer */
> +                       /* Ignore this settings */

Wrong style of comment.

> +                       pr_err("%s: kstr error idx(%d)\n", __func__, idx);
> +                       return;
> +               }
> +       }
> +
> +       ad5592r_update_default_channel_modes(new_ch_modes);
> +}

-- 
With Best Regards,
Andy Shevchenko