BGPIOF_READ_OUTPUT_REG_SET problem

PATCH Michael Trimarchi 3 participants 11 messages Dec 16, 2015 – Dec 17, 2015 1 message not in the archive GPIO

Share & tools

Michael Trimarchi

author

2015-12-16 14:05 UTC #0 lore raw

I have get a problem using gpio and imx6q freescale architecture. I don't really understand how should work your change and I have done a quick fix for me. Can you explain what was the idea?

Michael

drivers/gpio/gpio-generic.c +5 -2

index a3f0753..f9c6ff7 100644
@@ -139,11 +139,14 @@ static int bgpio_get_set(struct gpio_chip *gc, unsigned int gpio)
 {
 	struct bgpio_chip *bgc = to_bgpio_chip(gc);
 	unsigned long pinmask = bgc->pin2mask(bgc, gpio);
+	unsigned long reg;
 
 	if (bgc->dir & pinmask)
-		return bgc->read_reg(bgc->reg_set) & pinmask;
+		reg = bgc->read_reg(bgc->reg_set) & pinmask;
 	else
-		return bgc->read_reg(bgc->reg_dat) & pinmask;
+		reg = bgc->read_reg(bgc->reg_dat) & pinmask;
+
+	return !!(reg &  bgc->pin2mask(bgc, gpio));
 }
 
 static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)

Signature · 4 lines

| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Vladimir Zapolskiy

2015-12-16 14:51 UTC #1 lore raw

Hi Michael,

Quoted 3 lines · Michael Trimarchi

Hi
I have get a problem using gpio and imx6q freescale architecture.

what is the problem you experience? What is the version of the kernel (commit hash id) you run?

Quoted 2 lines

I don't really understand how should work your change and I have done a quick fix for me. Can you explain what was the idea?

I believe here you are talking about changes b19e7f51a5 and 442b2494b17d1 , please explain the problem.

With best wishes,
Vladimir

Quoted 24 lines

Michael

drivers/gpio/gpio-generic.c +5 -2

index a3f0753..f9c6ff7 100644
@@ -139,11 +139,14 @@ static int bgpio_get_set(struct gpio_chip *gc, unsigned int gpio)
 {
 	struct bgpio_chip *bgc = to_bgpio_chip(gc);
 	unsigned long pinmask = bgc->pin2mask(bgc, gpio);
+	unsigned long reg;
 
 	if (bgc->dir & pinmask)
-		return bgc->read_reg(bgc->reg_set) & pinmask;
+		reg = bgc->read_reg(bgc->reg_set) & pinmask;
 	else
-		return bgc->read_reg(bgc->reg_dat) & pinmask;
+		reg = bgc->read_reg(bgc->reg_dat) & pinmask;
+
+	return !!(reg &  bgc->pin2mask(bgc, gpio));
 }
 
 static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)

Michael Trimarchi

author

2015-12-16 15:25 UTC #2 lore raw

Quoted 10 lines · Vladimir Zapolskiy

Hi Michael,
Quoted 3 lines · Michael Trimarchi
Hi
I have get a problem using gpio and imx6q freescale architecture.
what is the problem you experience? What is the version of the kernel (commit hash id) you run?

I don't want the mask back when I read the gpio. I want to know if it's 1 or 0

Michael

Quoted 33 lines

Quoted 2 lines
I don't really understand how should work your change and I have done a quick fix for me. Can you explain what was the idea?
I believe here you are talking about changes b19e7f51a5 and 442b2494b17d1 , please explain the problem.
With best wishes,
Vladimir
Quoted 24 lines
Michael
drivers/gpio/gpio-generic.c +5 -2
index a3f0753..f9c6ff7 100644
@@ -139,11 +139,14 @@ static int bgpio_get_set(struct gpio_chip *gc, unsigned int gpio)
 {
 	struct bgpio_chip *bgc = to_bgpio_chip(gc);
 	unsigned long pinmask = bgc->pin2mask(bgc, gpio);
+	unsigned long reg;
 
 	if (bgc->dir & pinmask)
-		return bgc->read_reg(bgc->reg_set) & pinmask;
+		reg = bgc->read_reg(bgc->reg_set) & pinmask;
 	else
-		return bgc->read_reg(bgc->reg_dat) & pinmask;
+		reg = bgc->read_reg(bgc->reg_dat) & pinmask;
+
+	return !!(reg &  bgc->pin2mask(bgc, gpio));
 }
 
 static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)

Signature · 4 lines

| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Michael Trimarchi

author

2015-12-16 18:18 UTC #3 lore raw

Quoted 8 lines · Michael Trimarchi

Hi
Quoted 5 lines · Vladimir Zapolskiy
Hi Michael,
Quoted 2 lines · Michael Trimarchi
Hi

Quoted 9 lines

Quoted 5 lines
Quoted 1 line
I have get a problem using gpio and imx6q freescale architecture.
what is the problem you experience? What is the version of the kernel (commit hash id) you run?
I don't want the mask back when I read the gpio. I want to know if it's 1 or 0

This is my starting commit on my local tree:

dd5ae6818ccd4ab0aa6f7d84e88f60a48af0fb52

Michael

Quoted 41 lines

Michael

Quoted 33 lines

Quoted 2 lines
I don't really understand how should work your change and I have done a quick fix for me. Can you explain what was the idea?
I believe here you are talking about changes b19e7f51a5 and 442b2494b17d1 , please explain the problem.
With best wishes,
Vladimir
Quoted 24 lines
Michael
drivers/gpio/gpio-generic.c +5 -2
index a3f0753..f9c6ff7 100644
@@ -139,11 +139,14 @@ static int bgpio_get_set(struct gpio_chip *gc, unsigned int gpio)
 {
 	struct bgpio_chip *bgc = to_bgpio_chip(gc);
 	unsigned long pinmask = bgc->pin2mask(bgc, gpio);
+	unsigned long reg;
 
 	if (bgc->dir & pinmask)
-		return bgc->read_reg(bgc->reg_set) & pinmask;
+		reg = bgc->read_reg(bgc->reg_set) & pinmask;
 	else
-		return bgc->read_reg(bgc->reg_dat) & pinmask;
+		reg = bgc->read_reg(bgc->reg_dat) & pinmask;
+
+	return !!(reg &  bgc->pin2mask(bgc, gpio));
 }
 
 static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)

Signature · 4 lines

| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Signature · 4 lines

| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Vladimir Zapolskiy

2015-12-16 19:19 UTC #4 lore raw

Hi Michael,

Quoted 19 lines · Michael Trimarchi

Hi
Quoted 16 lines · Michael Trimarchi
Hi
Quoted 10 lines · Vladimir Zapolskiy
Hi Michael,
Quoted 3 lines · Michael Trimarchi
Hi
I have get a problem using gpio and imx6q freescale architecture.
what is the problem you experience? What is the version of the kernel (commit hash id) you run?
I don't want the mask back when I read the gpio. I want to know if it's 1 or 0

now I got what you want, but actually I didn't get completely what is your problem. Please be more descriptive.

Quoted 4 lines

This is my starting commit on my local tree:
dd5ae6818ccd4ab0aa6f7d84e88f60a48af0fb52

Good, so it is on Torvalds' branch and you have a potentially problematic commit e20538b82 applied.

By the way you may try to git-bisect the problem, I may be wrong in my assumptions regarding that commit.

With best wishes,
Vladimir

Quoted 40 lines

Quoted 39 lines

Quoted 33 lines

Quoted 2 lines
I don't really understand how should work your change and I have done a quick fix for me. Can you explain what was the idea?
I believe here you are talking about changes b19e7f51a5 and 442b2494b17d1 , please explain the problem.
With best wishes,
Vladimir
Quoted 24 lines
Michael
drivers/gpio/gpio-generic.c +5 -2
index a3f0753..f9c6ff7 100644
@@ -139,11 +139,14 @@ static int bgpio_get_set(struct gpio_chip *gc, unsigned int gpio)
 {
 	struct bgpio_chip *bgc = to_bgpio_chip(gc);
 	unsigned long pinmask = bgc->pin2mask(bgc, gpio);
+	unsigned long reg;
 
 	if (bgc->dir & pinmask)
-		return bgc->read_reg(bgc->reg_set) & pinmask;
+		reg = bgc->read_reg(bgc->reg_set) & pinmask;
 	else
-		return bgc->read_reg(bgc->reg_dat) & pinmask;
+		reg = bgc->read_reg(bgc->reg_dat) & pinmask;
+
+	return !!(reg &  bgc->pin2mask(bgc, gpio));
 }
 
 static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)

Signature · 4 lines

| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

A message that is not in the archive (sent off-list or not yet received): CAOf5uwmM2T6VVVfBLUED5=a4ZMpY10oJg_r3FCzAW+XkRLxbww@mail.gmail.com

Vladimir Zapolskiy

2015-12-16 20:03 UTC #6 lore raw

Hi Michael,

Quoted 39 lines · Michael Trimarchi

Hi
Quoted 35 lines · On Dec 16, 2015 8:18 PM, "Vladimir Zapolskiy" <[email protected]…
Hi Michael,
Quoted 19 lines · Michael Trimarchi
Hi
Quoted 16 lines · Michael Trimarchi
Hi
Quoted 10 lines · Vladimir Zapolskiy
Hi Michael,
Quoted 3 lines · Michael Trimarchi
Hi
I have get a problem using gpio and imx6q freescale architecture.
what is the problem you experience? What is the version of the kernel (commit hash id) you run?
I don't want the mask back when I read the gpio. I want to know if it's 1 or 0
now I got what you want, but actually I didn't get completely what is your problem. Please be more descriptive.
Quoted 4 lines
This is my starting commit on my local tree:
dd5ae6818ccd4ab0aa6f7d84e88f60a48af0fb52
Good, so it is on Torvalds' branch and you have a potentially problematic commit e20538b82 applied.

Quoted 6 lines

Quoted 3 lines
By the way you may try to git-bisect the problem, I may be wrong in my assumptions regarding that commit.
gc.get pointer should be assigned to function with same semantic. So on my tree semantic of function is different.

Putting aside returned negative values, the problem is gone, if you change the semantics in your tree and assume that gc.get() returns values: low - 0 and high - !0 (not necessary 1)? Because if you apply this logic, you get +18 correctly working gpiochip drivers in your tree.

Quoted 2 lines

I understand your point but we have two problems in mainline. One 0x10000000 and other get function.

Yes, the problem with the Bjorn's commit still exists. As well as another kind of problem, if the commit is not applied.

With best wishes,
Vladimir

Quoted 44 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
I don't really understand how should work your change and I have done
a quick
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
fix for me. Can you explain what was the idea?
I believe here you are talking about changes b19e7f51a5 and 442b2494b17d1 , please explain the problem.
With best wishes,
Vladimir
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/gpio/gpio-generic.c static int bgpio_get_set(struct gpio_chip *gc,
Quoted 7 lines
Michael
drivers/gpio/gpio-generic.c +0 -0
index a3f0753..f9c6ff7 100644
@@ -139,11 +139,14 @@ static int bgpio_get_set(struct gpio_chip *gc,
unsigned int gpio)
Quoted 25 lines
Quoted 24 lines
Quoted 23 lines
Quoted 15 lines
Quoted 15 lines
drivers/gpio/gpio-generic.c +5 -2
 {
   struct bgpio_chip *bgc = to_bgpio_chip(gc);
   unsigned long pinmask = bgc->pin2mask(bgc, gpio);
+  unsigned long reg;
 
   if (bgc->dir & pinmask)
-          return bgc->read_reg(bgc->reg_set) & pinmask;
+          reg = bgc->read_reg(bgc->reg_set) & pinmask;
   else
-          return bgc->read_reg(bgc->reg_dat) & pinmask;
+          reg = bgc->read_reg(bgc->reg_dat) & pinmask;
+
+  return !!(reg &  bgc->pin2mask(bgc, gpio));
 }
 
Quoted 2 lines
drivers/gpio/gpio-generic.c static int bgpio_get_set(struct gpio_chip *gc, unsigned int gpio) · line 152
Quoted 2 lines
drivers/gpio/gpio-generic.c +0 -0
 static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)
--
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Michael Trimarchi

author

2015-12-17 07:31 UTC #7 lore raw

Hi Vladimir

Quoted 54 lines · Vladimir Zapolskiy

Hi Michael,
Quoted 39 lines · Michael Trimarchi
Hi
Quoted 35 lines · On Dec 16, 2015 8:18 PM, "Vladimir Zapolskiy" <[email protected]…
Hi Michael,
Quoted 19 lines · Michael Trimarchi
Hi
Quoted 16 lines · Michael Trimarchi
Hi
Quoted 10 lines · Vladimir Zapolskiy
Hi Michael,
Quoted 3 lines · Michael Trimarchi
Hi
I have get a problem using gpio and imx6q freescale architecture.
what is the problem you experience? What is the version of the kernel (commit hash id) you run?
I don't want the mask back when I read the gpio. I want to know if it's 1 or 0
now I got what you want, but actually I didn't get completely what is your problem. Please be more descriptive.
Quoted 4 lines
This is my starting commit on my local tree:
dd5ae6818ccd4ab0aa6f7d84e88f60a48af0fb52
Good, so it is on Torvalds' branch and you have a potentially problematic commit e20538b82 applied.
Quoted 6 lines
Quoted 3 lines
By the way you may try to git-bisect the problem, I may be wrong in my assumptions regarding that commit.
gc.get pointer should be assigned to function with same semantic. So on my tree semantic of function is different.
Putting aside returned negative values, the problem is gone, if you change the semantics in your tree and assume that gc.get() returns values: low - 0 and high - !0 (not necessary 1)? Because if you apply this logic, you get +18 correctly working gpiochip drivers in your tree.

Quoted 6 lines

Quoted 2 lines
I understand your point but we have two problems in mainline. One 0x10000000 and other get function.
Yes, the problem with the Bjorn's commit still exists. As well as another kind of problem, if the commit is not applied.

It can be fine if >get() return 0 or 1 to the corresponding gpio and error in case of failure (like in gpio expander for example). My patch make the implementation
of bgpio_get_set equal to bgpio_get. I don't find any sense to let the bgpio_get_set to return
the mask because ofcourse 31 gpio can not work. Revert does not solve the problem because
we need to have int return value in general and not boolean

Michael

Quoted 48 lines

With best wishes,
Vladimir
Quoted 44 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
I don't really understand how should work your change and I have done
a quick
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
fix for me. Can you explain what was the idea?
I believe here you are talking about changes b19e7f51a5 and 442b2494b17d1 , please explain the problem.
With best wishes,
Vladimir
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/gpio/gpio-generic.c static int bgpio_get_set(struct gpio_chip *gc,
Quoted 7 lines
Michael
drivers/gpio/gpio-generic.c +0 -0
index a3f0753..f9c6ff7 100644
@@ -139,11 +139,14 @@ static int bgpio_get_set(struct gpio_chip *gc,
unsigned int gpio)
Quoted 25 lines
Quoted 24 lines
Quoted 23 lines
Quoted 15 lines
Quoted 15 lines
drivers/gpio/gpio-generic.c +5 -2
 {
   struct bgpio_chip *bgc = to_bgpio_chip(gc);
   unsigned long pinmask = bgc->pin2mask(bgc, gpio);
+  unsigned long reg;
 
   if (bgc->dir & pinmask)
-          return bgc->read_reg(bgc->reg_set) & pinmask;
+          reg = bgc->read_reg(bgc->reg_set) & pinmask;
   else
-          return bgc->read_reg(bgc->reg_dat) & pinmask;
+          reg = bgc->read_reg(bgc->reg_dat) & pinmask;
+
+  return !!(reg &  bgc->pin2mask(bgc, gpio));
 }
 
Quoted 2 lines
drivers/gpio/gpio-generic.c static int bgpio_get_set(struct gpio_chip *gc, · line 152
Quoted 2 lines
drivers/gpio/gpio-generic.c +0 -0
 static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)
--
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Signature · 4 lines

| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Vladimir Zapolskiy

2015-12-16 19:00 UTC #8 lore raw

In reply to Michael Trimarchi

Hi Michael,

Quoted 8 lines · Michael Trimarchi

Hi
Quoted 5 lines · Vladimir Zapolskiy
Hi Michael,
Quoted 2 lines · Michael Trimarchi
Hi

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
I have get a problem using gpio and imx6q freescale architecture.
what is the problem you experience? What is the version of the kernel (commit hash id) you run?
I don't want the mask back when I read the gpio. I want to know if it's 1 or 0

I hope I figured out what is your problem.

At the time when I submitted b19e7f51a5 (dated Apr 29, 2015) return type of _gpiod_get_raw_value() was bool, so it did not matter if you return mask or 0/1 value.

Later on Bjorn's commit e20538b82 ("gpio: Propagate errors from chip->get()") (dated Aug 28, 2015) changed the return type from bool to int, so you get the error now.

I would like to mention some points.

a) the Bjorn's commit brings the same problem to more GPIO drivers, this is a short and not totally verified list (only function names):

* da9055_gpio_get()
* davinci_gpio_get()
* em_gio_get()
* intel_gpio_get()
* ttl_get_value()
* max732x_gpio_get_value()
* ioh_gpio_get()
* mpc8572_gpio_get()
* mpc8xxx_gpio_get()
* msic_gpio_get()
* pcf857x_get()
* pch_gpio_get()
* sa1100_gpio_get()
* gsta_gpio_get()
* tc3589x_gpio_get()
* tz1090_gpio_get()
* tz1090_pdc_gpio_get()

The list of gc.get() functions which may return a negative value today is much shorter, so counting gpiochip drivers commit e20538b82 brings more bugs than fixes.

I believe the right decision will be to fix all currently broken gpiochip .get() functions, most probably returning a negative error was done on purpose, Linus, please confirm.

b) from Bjorn's commit:

-static bool _gpiod_get_raw_value(const struct gpio_desc *desc) +static int _gpiod_get_raw_value(const struct gpio_desc *desc)
{
struct gpio_chip *chip;
- bool value;
int offset;
+ int value;

chip = desc->chip;
offset = gpio_chip_hwgpio(desc);
- value = chip->get ? chip->get(chip, offset) : false;
+ value = chip->get ? chip->get(chip, offset) : -EIO;
+ value = value < 0 ? value : !!value;

^^^ this !! is a naive attempt to imitate backward compatible behaviour, however it is broken, because if some !0 value is returned, then it might happen that (value < 0) branch is selected.

Michael, is it your case? Do you receive an error, when reading a GPIO[x*32+31] line status?

 	trace_gpio_value(desc_to_gpio(desc), 1, value);
	return value;

}

c) here are the results of my testing on iMX6 SabreSD (edb42dc7bc0 on Torvald's branch with a slightly modified DTS):

output (red LED D3):

% echo 2 > /sys/class/gpio/export
% echo out > /sys/class/gpio/gpio2/direction
% echo 1 > /sys/class/gpio/gpio2/value
% cat /sys/class/gpio/gpio2/value
1

input (vol dn button)
% echo 5 > /sys/class/gpio/export
% echo in > /sys/class/gpio/gpio5/direction
% cat /sys/class/gpio/gpio5/value
1

Everything seems to be ok with gpios other than 31 + x*32, but the problem of course exists.

Michael, try to revert e20538b82 and retest on your end, I would appreciate to know the results.

Linus, I missed any relative discussion on the mailing list, what is the final decision, can _gpiod_get_raw_value() return errors or not? In other words which one of two gpiochip classes is broken, the one which returns errors as negative values in .get() or the one which returns masks? I believe it should be easier to fix the second one, even if it is larger though :)

With best wishes,
Vladimir

Quoted 34 lines

Quoted 33 lines
Quoted 2 lines
I don't really understand how should work your change and I have done a quick fix for me. Can you explain what was the idea?
I believe here you are talking about changes b19e7f51a5 and 442b2494b17d1 , please explain the problem.
With best wishes,
Vladimir
Quoted 24 lines
Michael
drivers/gpio/gpio-generic.c +5 -2
index a3f0753..f9c6ff7 100644
@@ -139,11 +139,14 @@ static int bgpio_get_set(struct gpio_chip *gc, unsigned int gpio)
 {
 	struct bgpio_chip *bgc = to_bgpio_chip(gc);
 	unsigned long pinmask = bgc->pin2mask(bgc, gpio);
+	unsigned long reg;
 
 	if (bgc->dir & pinmask)
-		return bgc->read_reg(bgc->reg_set) & pinmask;
+		reg = bgc->read_reg(bgc->reg_set) & pinmask;
 	else
-		return bgc->read_reg(bgc->reg_dat) & pinmask;
+		reg = bgc->read_reg(bgc->reg_dat) & pinmask;
+
+	return !!(reg &  bgc->pin2mask(bgc, gpio));
 }
 
 static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)

Linus Walleij

2015-12-17 09:09 UTC #9 lore raw

Quoted 3 lines · Vladimir Zapolskiy

Later on Bjorn's commit e20538b82 ("gpio: Propagate errors from chip->get()") (dated Aug 28, 2015) changed the return type from bool to int, so you get the error now.

Right.

Quoted 3 lines

I believe the right decision will be to fix all currently broken gpiochip .get() functions, most probably returning a negative error was done on purpose, Linus, please confirm.

On (A): yes the right thing to do is to fix all broken .get() functions. Help appreciated.

Yes, that is described in a response in the mail chain: we once believed that get/set would not return negative so I was about to actually make them bool. I was thinking it's just a register write or something. gpiolib was once invented for that simple usecase.

But then we realized that e.g. a GPIO expander can fail an I2C transaction for whatever reason (hardware failure for example) and we need to handle that, so Björn's fix is the right one and we need to fix the drivers.

I'm sorry for not realizing the scope of the problem :(

Quoted 9 lines

        chip = desc->chip;
        offset = gpio_chip_hwgpio(desc);
(quoted diff) +2 -1
-       value = chip->get ? chip->get(chip, offset) : false;
+       value = chip->get ? chip->get(chip, offset) : -EIO;
+       value = value < 0 ? value : !!value;
^^^ this !! is a naive attempt to imitate backward compatible behaviour, however it is broken, because if some !0 value is returned, then it might happen that (value < 0) branch is selected.

Hm. But we still need to propagate errors.

Maybe we should patch this back to not propagate errors for the v4.4-rcS and then revert that and do the real fix, proofreading all vulnerable drivers for v4.5, what do you say?

(I'll send a patch.)

Quoted 2 lines

Linus, I missed any relative discussion on the mailing list, what is the final decision, can _gpiod_get_raw_value() return errors or not?

What do you think about my proposal to revert to the old behaviour for v4.4 and we'll go over all drivers and provide a deeper fix in v4.5?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Linus Walleij

2015-12-17 09:00 UTC #10 lore raw

In reply to Michael Trimarchi

Quoted 15 lines · Michael Trimarchi

I have get a problem using gpio and imx6q freescale architecture. I don't really understand how should work your change and I have done a quick fix for me. Can you explain what was the idea?
Michael
drivers/gpio/gpio-generic.c +1 -0
index a3f0753..f9c6ff7 100644
@@ -139,11 +139,14 @@ static int bgpio_get_set(struct gpio_chip *gc, unsigned int gpio)
 {
        struct bgpio_chip *bgc = to_bgpio_chip(gc);
        unsigned long pinmask = bgc->pin2mask(bgc, gpio);
+       unsigned long reg;

drivers/gpio/gpio-generic.c static int bgpio_get_set(struct gpio_chip *gc, unsigned int gpio) · line 150

Quoted 10 lines

drivers/gpio/gpio-generic.c +4 -2

 
        if (bgc->dir & pinmask)
-               return bgc->read_reg(bgc->reg_set) & pinmask;
+               reg = bgc->read_reg(bgc->reg_set) & pinmask;
        else
-               return bgc->read_reg(bgc->reg_dat) & pinmask;
+               reg = bgc->read_reg(bgc->reg_dat) & pinmask;
+
+       return !!(reg &  bgc->pin2mask(bgc, gpio));
 }

This looks identical to this fix I have queued for fixes:
https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/commit/?h=fixes&id=21f2856498ff09a03ebc8f9ff7581451a221d81b

Yours,
Linus Walleij

Michael Trimarchi

author

2015-12-17 09:04 UTC #11 lore raw

Quoted 19 lines · Linus Walleij

Quoted 16 lines · Michael Trimarchi
I have get a problem using gpio and imx6q freescale architecture. I don't really understand how should work your change and I have done a quick fix for me. Can you explain what was the idea?
Michael
drivers/gpio/gpio-generic.c +1 -0
index a3f0753..f9c6ff7 100644
@@ -139,11 +139,14 @@ static int bgpio_get_set(struct gpio_chip *gc, unsigned int gpio)
 {
        struct bgpio_chip *bgc = to_bgpio_chip(gc);
        unsigned long pinmask = bgc->pin2mask(bgc, gpio);
+       unsigned long reg;
 

Quoted 13 lines

drivers/gpio/gpio-generic.c static int bgpio_get_set(struct gpio_chip *gc, unsigned int gpio) · line 150
Quoted 9 lines
drivers/gpio/gpio-generic.c +4 -2
        if (bgc->dir & pinmask)
-               return bgc->read_reg(bgc->reg_set) & pinmask;
+               reg = bgc->read_reg(bgc->reg_set) & pinmask;
        else
-               return bgc->read_reg(bgc->reg_dat) & pinmask;
+               reg = bgc->read_reg(bgc->reg_dat) & pinmask;
+
+       return !!(reg &  bgc->pin2mask(bgc, gpio));
 }
This looks identical to this fix I have queued for fixes:
https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/commit/?h=fixes&id=21f2856498ff09a03ebc8f9ff7581451a221d81b

Good, so I can confirm that it's urgent

Michael

Quoted 2 lines

Yours,
Linus Walleij

Signature · 4 lines

| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |