linux-kernel - Re: [PATCH] gpiolib: Show correct direction from the beginning

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: <CAPybu_2ovB3T1fOD2gP9rM46z37WVuOuzu5FyKamfyMWoVg=yw@mail.gmail.com>
Date:   Thu, 20 Sep 2018 16:14:00 +0200
From:   Ricardo Ribalda Delgado <ricardo.ribalda@...il.com>
To:     timur@...i.org
Cc:     Linus Walleij <linus.walleij@...aro.org>, swboyd@...omium.org,
        linux-gpio@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] gpiolib: Show correct direction from the beginning

Hi
On Thu, Sep 20, 2018 at 2:20 PM Timur Tabi <timur@...i.org> wrote:
>
>
>
> On 09/19/2018 10:27 AM, Ricardo Ribalda Delgado wrote:
> > Let me explain my current setup
> >
> > I have a board with input and output gpios, the direction is defined
> > via pdata. When I run gpioinfo all the gpios are shown as input,
> > regardless if they are input or outputs: Eg:
> >
> > root@...022:/tmp# ./gpioinfo
> >
> > gpiochip0 - 16 lines:
> >          line   0:     "PROG_B"       unused   input  active-high
> >          line   1:         "M0"       unused   input  active-high
> >          line   2:         "M1"       unused   input  active-high
> >          line   3:         "M2"       unused   input  active-high
> >          line   4:        "DIN"       unused   input  active-high
> >          line   5:       "CCLK"       unused   input  active-high
> >          line   6:      unnamed       unused   input  active-high
> >          line   7:      unnamed       unused   input  active-high
> >          line   8:       "DONE"       unused   input  active-high
> >          line   9:     "INIT_B"       unused   input  active-high
> >          line  10:      unnamed       unused   input  active-high
> >          line  11:      unnamed       unused   input  active-high
> >          line  12:      unnamed       unused   input  active-high
> >          line  13:      unnamed       unused   input  active-high
> >          line  14:      unnamed       unused   input  active-high
> >          line  15:      unnamed       unused   input  active-high
>
> Yes, this is a known problem that should be fixed.
>
> > That is wrong and very confusing to the user, it can also lead to a
> > mayor fuckup if the user decides to connect two output gpio pins
> > because he expects that both are input. (This is the programming port,
> > but I also have 24 V -high current GPIOs)
>
> Users are expected to program the direction for every GPIO they want to
> use, regardless of whatever it's set to before they open it.

I do not agree that the user should program the direction of a GPIO
which direction cannot be used.

Also I am not talking about programming a gpio, I am talking about an
technician  connecting portA to portB and burning something because
the system provided erroneous information

>
> > There is a function in the API to tell libgpio if a gpio is out our
> > in. Why not use it?
>
> Because calling that API before properly claiming the GPIO is a
> programming error.

Is there a place where this API is defined?. Which functions require
to be defined.? What is the correct order.?

>
> > - If the configuration is hardcoded, the driver will return a fixed value
> > - If it is cheap to query the hardware, the driver will query the hardware,
> > - If it is expensive to query the hardware the driver can either
> > return a cached value or a fake value (current situation)
>
> The reason why the Qualcomm driver is impacted the most is because on
> ACPI platforms, the GPIO map is "sparse".  That is, not every GPIO
> between 0 and n-1 actually exists.  So reading a GPIO that doesn't exist
> is invalid.

Why are we adding GPIOs that are invalid?
If you can figure out that a GPIO is invalid when the user claims a
gpio, you can also figure it out when the user asks the direction.

>
> The way to protect against that is to claim the GPIO first.  If the
> claim is rejected, then you know that you can't access that GPIO.
>
> The bug is that the original code that I deleted (and that you're trying
> to put back) doesn't claim the GPIO first.
>
> >>From my point of view:  "The get_direction callback normally triggers
> > a  read/write to hardware, but we shouldn't be touching the hardware
> > for   an individual GPIO until after it's been properly claimed." is
> > an statement specific for your platform and should be fixed in your
> > driver.
> >
> > Either that, or I have completely missunderstund the purpouse of gpiod
> > :), and that could easily be the case.
>
> It's not a platform-specific statement.  It applies to all drivers.  In
> some drivers, the get_direction function had side-effects (like
> programming muxes, IIRC) that no one really cared about but was
> technically wrong.

A get operation should not set any functionality..., it should return
a cached value or query safely the hardware.


>
> I'm not sure how to properly fix this, but I wonder if we need some kind
> of late-stage initialization where gpiolib scans all the GPIOs by
> claiming them first, reading the directions, and then releasing them.

That sounds like a good compromise. Or returning
-unconfigured / unknown

is also an option.


-- 
Ricardo Ribalda