[RFC] gpiolib: Fix gpio_direction_* for single direction GPIOs

[RFC] gpiolib: Fix gpio_direction_* for single direction GPIOs

[Ricardo Ribalda Delgado]

GPIOs with no programmable direction are not required to implement
direction_output nor direction_input.

If we try to set an output direction on an output-only GPIO or input
direction on an input-only GPIO simply return 0.

This allows this single direction GPIO to be used by libgpiod.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/gpio/gpiolib.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index af853749e0bb..3c09bf70e7ab 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2517,19 +2517,27 @@ EXPORT_SYMBOL_GPL(gpiochip_free_own_desc);
 int gpiod_direction_input(struct gpio_desc *desc)
 {
 	struct gpio_chip	*chip;
-	int			status = -EINVAL;
+	int			status = 0;
 
 	VALIDATE_DESC(desc);
 	chip = desc->gdev->chip;
 
-	if (!chip->get || !chip->direction_input) {
+	if (!chip->get && chip->direction_input) {
 		gpiod_warn(desc,
-			"%s: missing get() or direction_input() operations\n",
+			"%s: missing get() and direction_input() operations\n",
 			__func__);
 		return -EIO;
 	}
 
-	status = chip->direction_input(chip, gpio_chip_hwgpio(desc));
+	if (chip->direction_input) {
+		status = chip->direction_input(chip, gpio_chip_hwgpio(desc));
+	} else if (chip->get_direction &&
+		  (chip->get_direction(chip, gpio_chip_hwgpio(desc)) != 1)) {
+		gpiod_warn(desc,
+			"%s: missing direction_input() operation\n",
+			__func__);
+		return -EIO;
+	}
 	if (status == 0)
 		clear_bit(FLAG_IS_OUT, &desc->flags);
 
@@ -2551,16 +2559,28 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
 {
 	struct gpio_chip *gc = desc->gdev->chip;
 	int val = !!value;
-	int ret;
+	int ret = 0;
 
-	if (!gc->set || !gc->direction_output) {
+	if (!gc->set && !gc->direction_output) {
 		gpiod_warn(desc,
-		       "%s: missing set() or direction_output() operations\n",
+		       "%s: missing set() and direction_output() operations\n",
 		       __func__);
 		return -EIO;
 	}
 
-	ret = gc->direction_output(gc, gpio_chip_hwgpio(desc), val);
+	if (gc->direction_output) {
+		ret = gc->direction_output(gc, gpio_chip_hwgpio(desc), val);
+	} else {
+		if (gc->get_direction &&
+		    gc->get_direction(gc, gpio_chip_hwgpio(desc))) {
+			gpiod_warn(desc,
+				"%s: missing direction_output() operation\n",
+				__func__);
+			return -EIO;
+		}
+		gc->set(gc, gpio_chip_hwgpio(desc), val);
+	}
+
 	if (!ret)
 		set_bit(FLAG_IS_OUT, &desc->flags);
 	trace_gpio_value(desc_to_gpio(desc), 0, val);
-- 
2.18.0

[PATCH] gpiolib: Show correct direction from the beginning

[Ricardo Ribalda Delgado]

Current code assumes that the direction is input if direction_input
function is set.
This might not be the case on GPIOs with programmable direction.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/gpio/gpiolib.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a57300c1d649..af853749e0bb 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1352,7 +1352,12 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
 		 * it does, and in case chip->get_direction is not set, we may
 		 * expose the wrong direction in sysfs.
 		 */
-		desc->flags = !chip->direction_input ? (1 << FLAG_IS_OUT) : 0;
+		if (chip->get_direction)
+			desc->flags = !chip->get_direction(chip, i) ?
+					(1 << FLAG_IS_OUT) : 0;
+		else
+			desc->flags = !chip->direction_input ?
+					(1 << FLAG_IS_OUT) : 0;
 	}
 
 #ifdef CONFIG_PINCTRL
-- 
2.18.0

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

[Linus Walleij]

On Fri, Sep 14, 2018 at 12:08 AM Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:

> Current code assumes that the direction is input if direction_input
> function is set.
> This might not be the case on GPIOs with programmable direction.
>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/gpio/gpiolib.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index a57300c1d649..af853749e0bb 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1352,7 +1352,12 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
>                  * it does, and in case chip->get_direction is not set, we may
>                  * expose the wrong direction in sysfs.
>                  */
> -               desc->flags = !chip->direction_input ? (1 << FLAG_IS_OUT) : 0;
> +               if (chip->get_direction)
> +                       desc->flags = !chip->get_direction(chip, i) ?
> +                                       (1 << FLAG_IS_OUT) : 0;
> +               else
> +                       desc->flags = !chip->direction_input ?
> +                                       (1 << FLAG_IS_OUT) : 0;

We used to do this.

But it breaks Qualcomm hardware.

commit 1ca2a92b2a99323f666f1b669b7484df4bda05e4
Author: Timur Tabi <timur@codeaurora.org>
Date:   Wed Dec 20 13:10:31 2017 -0600

    Revert "gpio: set up initial state from .get_direction()"

    This reverts commit 72d3200061776264941be1b5a9bb8e926b3b30a5.

    We cannot blindly query the direction of all GPIOs when the pins are
    first registered.  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.

    Signed-off-by: Timur Tabi <timur@codeaurora.org>
    Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
    Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[Ricardo Ribalda Delgado]

Hi Linus
On Wed, Sep 19, 2018 at 12:40 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Sep 14, 2018 at 12:08 AM Ricardo Ribalda Delgado
> <ricardo.ribalda@gmail.com> wrote:
>
> > Current code assumes that the direction is input if direction_input
> > function is set.
> > This might not be the case on GPIOs with programmable direction.
> >
> > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> > ---
> >  drivers/gpio/gpiolib.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index a57300c1d649..af853749e0bb 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1352,7 +1352,12 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
> >                  * it does, and in case chip->get_direction is not set, we may
> >                  * expose the wrong direction in sysfs.
> >                  */
> > -               desc->flags = !chip->direction_input ? (1 << FLAG_IS_OUT) : 0;
> > +               if (chip->get_direction)
> > +                       desc->flags = !chip->get_direction(chip, i) ?
> > +                                       (1 << FLAG_IS_OUT) : 0;
> > +               else
> > +                       desc->flags = !chip->direction_input ?
> > +                                       (1 << FLAG_IS_OUT) : 0;
>
> We used to do this.
>
> But it breaks Qualcomm hardware.

And should't that be tacked in qcom hardware with something like:

if (!priv->initialized)
   return INPUT;

if you or Timur point me to the harware that was crashing I would not
mind looking into that, but the current situations seems to me like a
hack.

Other option would be to implement a new function get_direction_fast()
(the name could be better). I can also try to implement something in
that direction if you want.

Thanks for your review.

>
> commit 1ca2a92b2a99323f666f1b669b7484df4bda05e4
> Author: Timur Tabi <timur@codeaurora.org>
> Date:   Wed Dec 20 13:10:31 2017 -0600
>
>     Revert "gpio: set up initial state from .get_direction()"
>
>     This reverts commit 72d3200061776264941be1b5a9bb8e926b3b30a5.
>
>     We cannot blindly query the direction of all GPIOs when the pins are
>     first registered.  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.
>
>     Signed-off-by: Timur Tabi <timur@codeaurora.org>
>     Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
>     Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
> Yours,
> Linus Walleij



-- 
Ricardo Ribalda

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

[Timur Tabi]

On 9/18/18 11:04 PM, Ricardo Ribalda Delgado wrote:
> And should't that be tacked in qcom hardware with something like:
> 
> if (!priv->initialized)
>     return INPUT;
> 
> if you or Timur point me to the harware that was crashing I would not
> mind looking into that, but the current situations seems to me like a
> hack.

I'd say the previous code was the hack.  My comment about not touching 
the hardware until it is properly claimed is valid, and it applies to 
all platforms.

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

[Ricardo Ribalda Delgado]

Hi Timur
On Wed, Sep 19, 2018 at 1:50 PM Timur Tabi <timur@kernel.org> wrote:
>
> On 9/18/18 11:04 PM, Ricardo Ribalda Delgado wrote:
> > And should't that be tacked in qcom hardware with something like:
> >
> > if (!priv->initialized)
> >     return INPUT;
> >
> > if you or Timur point me to the harware that was crashing I would not
> > mind looking into that, but the current situations seems to me like a
> > hack.
>
> I'd say the previous code was the hack.  My comment about not touching
> the hardware until it is properly claimed is valid, and it applies to
> all platforms.

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@qt5022:/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

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)

There is a function in the API to tell libgpio if a gpio is out our
in. Why not use it?
- 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)


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.

Regards!

-- 
Ricardo Ribalda

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

[Linus Walleij]

On Wed, Sep 19, 2018 at 4:50 AM Timur Tabi <timur@kernel.org> wrote:
> On 9/18/18 11:04 PM, Ricardo Ribalda Delgado wrote:
> > And should't that be tacked in qcom hardware with something like:
> >
> > if (!priv->initialized)
> >     return INPUT;
> >
> > if you or Timur point me to the harware that was crashing I would not
> > mind looking into that, but the current situations seems to me like a
> > hack.
>
> I'd say the previous code was the hack.  My comment about not touching
> the hardware until it is properly claimed is valid, and it applies to
> all platforms.

I am a bit uncertain, I understand the reasoning that unless a GPIO line
has been "claimed" (i.e. .request() was called on it) then we should not
call any of the gpiochip callbacks.

But this is merely a convention, the gpiochip becomes what we want
it to be and it has the semantics we want it to have.

It also makes sense to inquire the direction initially so we know the
state of the hardware in the library.

I think most gpiochips easily survives calling the .get_direction()
early, Qualcomm's stand out here.

Now that we have .valid_mask in the gpiochip could we simply just
add this back, resepecting valid_mask and avoid checking the
direction of precisely these GPIOs?

Yours,
Linus Walleij

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

[Timur Tabi]

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@qt5022:/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.

> 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.

> - 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.

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.

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.

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

[Timur Tabi]

On 9/19/18 10:27 AM, Ricardo Ribalda Delgado wrote:
> "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 

That is definitely not true.

> and should be fixed in your
> driver.

There is no bug in my driver.  The driver reports only a subset of the 
GPIOs, because that's all that are available.  Attempting to access an 
invalid GPIO generates an XPU violation.  The original code was 
attempting to access GPIOs that the driver said don't exist.

The code that we have today is the result of months of discussion, 
negotiation, and trial-and-error.

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

[Timur Tabi]

On 9/20/18 12:23 AM, Linus Walleij wrote:
> I think most gpiochips easily survives calling the .get_direction()
> early, Qualcomm's stand out here.
> 
> Now that we have .valid_mask in the gpiochip could we simply just
> add this back, resepecting valid_mask and avoid checking the
> direction of precisely these GPIOs?

Can you be more specific?  One of the proposals made previously was to 
add a check in msm_gpio_get_direction(), but that was rejected because 
the consensus was the valid_mask checks in gpiolib are sufficient.

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

[Ricardo Ribalda Delgado]

Hi
On Thu, Sep 20, 2018 at 2:20 PM Timur Tabi <timur@tabi.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@qt5022:/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

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

[Linus Walleij]

On Thu, Sep 20, 2018 at 5:35 AM Timur Tabi <timur@kernel.org> wrote:
> On 9/20/18 12:23 AM, Linus Walleij wrote:
> > I think most gpiochips easily survives calling the .get_direction()
> > early, Qualcomm's stand out here.
> >
> > Now that we have .valid_mask in the gpiochip could we simply just
> > add this back, resepecting valid_mask and avoid checking the
> > direction of precisely these GPIOs?
>
> Can you be more specific?  One of the proposals made previously was to
> add a check in msm_gpio_get_direction(), but that was rejected because
> the consensus was the valid_mask checks in gpiolib are sufficient.

What I mean is that $SUBJECT patch might not hurt Qualcomms
GPIOs (not crash the platform) if and only if it is augmented to not
try to get the initial direction from lines masked off in .valid_mask
if .need_valid_mask is true.

Whether it makes sense semantically is a different debate, but it
seems possible to reintroduce calling .get_direction() without
hurting anyone.

Yours,
Linus Walleij

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

[Linus Walleij]

On Thu, Sep 20, 2018 at 7:14 AM Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> On Thu, Sep 20, 2018 at 2:20 PM Timur Tabi <timur@tabi.org> wrote:

> > 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

So what is clear is that your need, I guess in userspace, reliable
information about the direction of the GPIOs at boot.

> > 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.?

There is nothing like such. We would have to establish semantics.
I don't see a point in it, the APIs are for using and understanding,
not for discussing API contracts. I would avoid trying to etch this
API in stone.

> > 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.

Right and that is what the (later introduced) valid_mask
can do.

Any time we call into any of the callbacks that take an offset
we should (theoretically) check the .valid_mask if active.

Since we normally check it when requesting the GPIO, we
can't request invalid GPIOs and normally the other callbacks
would only get called after that, so we are fine.

> > 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.

I would just skip over anything asked off in the valid_mask.

I feel positive of a version of this patch that respect valid_mask
some way, as that should be safe for Qualcomms usecase.

Rough consensus and running code.

Yours,
Linus Walleij

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

[Timur Tabi]

On 9/20/18 5:36 PM, Linus Walleij wrote:
> What I mean is that $SUBJECT patch might not hurt Qualcomms
> GPIOs (not crash the platform) if and only if it is augmented to not
> try to get the initial direction from lines masked off in .valid_mask
> if .need_valid_mask is true.
> 
> Whether it makes sense semantically is a different debate, but it
> seems possible to reintroduce calling .get_direction() without
> hurting anyone.

That means that all the logic for checking valid_mask needs to be added 
to the chip driver's .get_direction() function.  We can add that logic 
to msm_gpio_get_direction (at one point, I had a patch that did that, 
but it was rejected).

My concern is: what if a driver depends on a .request call being made 
(in order to configure muxes, for example) before touching the hardware?

I wonder if this is something that really should be handled in the 
driver's .probe function.  The driver should collect that information 
and pass it to add_data.

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

[Linus Walleij]

On Thu, Sep 20, 2018 at 7:05 PM Timur Tabi <timur@kernel.org> wrote:
> On 9/20/18 5:36 PM, Linus Walleij wrote:
> > What I mean is that $SUBJECT patch might not hurt Qualcomms
> > GPIOs (not crash the platform) if and only if it is augmented to not
> > try to get the initial direction from lines masked off in .valid_mask
> > if .need_valid_mask is true.
> >
> > Whether it makes sense semantically is a different debate, but it
> > seems possible to reintroduce calling .get_direction() without
> > hurting anyone.
>
> That means that all the logic for checking valid_mask needs to be added
> to the chip driver's .get_direction() function.  We can add that logic
> to msm_gpio_get_direction (at one point, I had a patch that did that,
> but it was rejected).

Nah, what is in patch v2 is better, just checking it when we need to.

> My concern is: what if a driver depends on a .request call being made
> (in order to configure muxes, for example) before touching the hardware?

Hm. That is a good question.

I wonder if we have that problem in practice. If this happens, maybe the
driver needs to keep track of stuff a bit. I think if we just loop request
over everything we could disturb other mux set-up.

> I wonder if this is something that really should be handled in the
> driver's .probe function.  The driver should collect that information
> and pass it to add_data.

I see the idea, but it seems complicated compared to just calling
the callbacks. Let's try the v2 patch approach first.

Yours,
Linus Walleij