linux-kernel - Re: [PATCH 6/7] drivers/gpio: Add and export gpiod_lookup[

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]
Date:   Thu, 26 Jan 2017 16:24:21 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Furquan Shaikh <furquan@...omium.org>
Cc:     "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Mark Brown <broonie@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Tony Lindgren <tony@...mide.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Len Brown <lenb@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Hanjun Guo <hanjun.guo@...aro.org>,
        Will Deacon <will.deacon@....com>,
        Rob Herring <robh@...nel.org>,
        Sathyanarayana Nujella <sathyanarayana.nujella@...el.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Adam Thomson <Adam.Thomson.Opensource@...semi.com>,
        Alexandre Courbot <gnurou@...il.com>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Linux-OMAP <linux-omap@...r.kernel.org>,
        Dmitry Torokhov <dtor@...omium.org>
Subject: Re: [PATCH 6/7] drivers/gpio: Add and export gpiod_lookup[_index]

On Wed, Jan 25, 2017 at 1:06 AM, Furquan Shaikh <furquan@...omium.org> wrote:

> Sometimes (as the case with fixed regulator) we only want to look up,
> but not necessarily reserve right away, GPIO. gpiod_lookup() and
> gpiod_lookup_by_index() allow us doing just that.
>
> Signed-off-by: Dmitry Torokhov <dtor@...omium.org>
> Signed-off-by: Furquan Shaikh <furquan@...omium.org>

Overall if this solves the GPIO regulator mess, I'd be happy
to apply it.

But overall it is a bit scary with looking up something struct gpiod *
that does not at all have the same semantics as a gpiod *
you get from gpiod_get().

We need to make sure that a gpiod looked up this way does not
work and tick and cannot perform operations as one obtained
with a proper gpiod_get().

>  /**
> + * gpiod_lookup - look up a GPIO for a given GPIO function
> + * @dev:       GPIO consumer, can be NULL for system-global GPIOs
> + * @con_id:    function within the GPIO consumer
> + *
> + * Return the GPIO descriptor corresponding to the function con_id of device
> + * dev, -ENOENT if no GPIO has been assigned to the requested function, or
> + * another IS_ERR() code.
> + */
> +struct gpio_desc *__must_check gpiod_lookup(struct device *dev,
> +                                           const char *con_id)
> +{
> +       return gpiod_lookup_index(dev, con_id, 0);
> +}
> +EXPORT_SYMBOL_GPL(gpiod_lookup);

Make this a static inline in the <linux/gpio/consumer.h> header instead.
Saves a lot of trouble.

>  /**
> - * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
> + * gpiod_lookup_index - look up a GPIO from a multi-index GPIO function
>   * @dev:       GPIO consumer, can be NULL for system-global GPIOs
>   * @con_id:    function within the GPIO consumer
>   * @idx:       index of the GPIO to obtain in the consumer
> - * @flags:     optional GPIO initialization flags
> - *
> - * This variant of gpiod_get() allows to access GPIOs other than the first
> - * defined one for functions that define several GPIOs.
>   *
> - * Return a valid GPIO descriptor, -ENOENT if no GPIO has been assigned to the
> - * requested function and/or index, or another IS_ERR() code if an error
> - * occurred while trying to acquire the GPIO.
> + * Return a valid GPIO descriptor, or -ENOENT if no GPIO has been assigned to
> + * the requested function and/or index, or another IS_ERR() code.
>   */

What a mess in the patch. Not your fault but format-patch created this
mess.0

> -struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
> -                                              const char *con_id,
> -                                              unsigned int idx,
> -                                              enum gpiod_flags flags)
> +struct gpio_desc *__must_check gpiod_lookup_index(struct device *dev,
> +                                                 const char *con_id,
> +                                                 unsigned int idx)
>  {
>         struct gpio_desc *desc = NULL;
> -       int status;
>         enum gpio_lookup_flags lookupflags = 0;
>
>         dev_dbg(dev, "GPIO lookup for consumer %s\n", con_id);
> @@ -3285,16 +3294,50 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
>                 desc = gpiod_find(dev, con_id, idx, &lookupflags);
>         }
>
> -       if (IS_ERR(desc)) {
> +       if (IS_ERR(desc))
>                 dev_dbg(dev, "lookup for GPIO %s failed\n", con_id);
> +
> +       /*
> +        * Configure static flags based on lookup data (such as
> +        * "active low", "open drain", etc.)
> +        */
> +       gpiod_configure_flags(desc, con_id, lookupflags, 0);

So what about setting some special flag from
gpiolib.h like
set_bit(FLAG_ONLY_LOOKUP, &desc->flags);
here numbing all
operations on the returned gpiod, by a check in
VALIDATE_DESC() and VALIDATE_DESC_VOID()
so that no real operations fall through?

Then that flag will just be forcibly cleared by
gpiod_get[_index]() later.

Yours,
Linus Walleij