linux-kernel - Re: [PATCH 1/3] gpio: regmap: Support registers with more than one bit per GPIO

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: <CAHp75VeosViEcLZ3LF92=_bpSmyGthh2j9V4xYtHTt0Z-fn3-Q@mail.gmail.com>
Date:   Sun, 3 Jul 2022 16:09:26 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Aidan MacDonald <aidanmacdonald.0x0@...il.com>
Cc:     Michael Walle <michael@...le.cc>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] gpio: regmap: Support registers with more than one
 bit per GPIO

On Sun, Jul 3, 2022 at 1:11 PM Aidan MacDonald
<aidanmacdonald.0x0@...il.com> wrote:
>
> Some devices use a multi-bit register field to change the GPIO
> input/output direction. Add the ->reg_field_xlate() callback to
> support such devices in gpio-regmap.
>
> ->reg_field_xlate() builds on ->reg_mask_xlate() by allowing the
> driver to return a mask and values to describe a register field.
> gpio-regmap will use the mask to isolate the field and compare or
> update it using the values to implement GPIO level and direction
> get and set ops.

Thanks for the proposal. My comments below.

...

> +static void
> +gpio_regmap_simple_field_xlate(struct gpio_regmap *gpio,
> +                              unsigned int base, unsigned int offset,
> +                              unsigned int *reg, unsigned int *mask,
> +                              unsigned int *values)
> +{
> +       gpio->reg_mask_xlate(gpio, base, offset, reg, mask);
> +       values[0] = 0;
> +       values[1] = *mask;

This is a fragile and less compile-check prone approach. If you know
the amount of values, make a specific data type for that, or pass the
length of the output buffer..

> +}

...

> +       unsigned int values[2];

> +       return (val & mask) == values[1];

> +       unsigned int values[2];

How will the callee know that it's only 2 available?


> +       regmap_update_bits(gpio->regmap, reg, mask, values[!!val]);

If we have special meaning of the values, perhaps it needs to follow
an enum of some definitions, so everybody will understand how indices
are mapped to the actual data in the array.

> +       unsigned int values[2];

> +       regmap_update_bits(gpio->regmap, reg, mask, values[1]);

> +       unsigned int values[2];

> +       if ((val & mask) == values[invert])

How do you guarantee this won't overflow? (see above comment about
indices mapping)

> +       unsigned int values[2];

As per above comments.

-- 
With Best Regards,
Andy Shevchenko