gpio: gpio-mockup: Fix usage of new GPIO_LINE_DIRECTION
Subsystems (2)
PATCH 1/1 gpio: gpio-mockup: Fix usage of new GPIO_LINE_DIRECTION
Restore the external behavior of gpio-mockup to what it was prior to the change to using GPIO_LINE_DIRECTION.
---
Fix a regression introduced in v5.5-rc1.
The change to GPIO_LINE_DIRECTION reversed the polarity of the dir field within gpio-mockup.c, but overlooked inverting the value on initialization and when returned by gpio_mockup_get_direction. The latter is a bug.
The former is a problem for tests which assume initial conditions, specifically the mockup used to initialize chips with all lines as inputs. That superficially appeared to be the case after the previous patch due to the bug in gpio_mockup_get_direction.
drivers/gpio/gpio-mockup.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/gpio/gpio-mockup.c +5 -2
@@ -226,7 +226,7 @@ static int gpio_mockup_get_direction(struct gpio_chip *gc, unsigned int offset)int direction;mutex_lock (&chip->lock); - direction = !chip->lines[offset].dir; + direction = chip->lines[offset].dir;mutex_unlock (&chip->lock);return direction;
@@ -395,7 +395,7 @@ static int gpio_mockup_probe(struct platform_device *pdev)struct gpio_chip *gc;struct device *dev;const char *name; -int rv, base; +int rv, base, i;u16 ngpio; dev = &pdev->dev;
@@ -447,6 +447,9 @@ static int gpio_mockup_probe(struct platform_device *pdev)if (!chip->lines)return -ENOMEM ; +for (i =0 ; i < gc->ngpio; i++) + chip->lines[i].dir =GPIO_LINE_DIRECTION_IN ; +if (device_property_read_bool (dev,"named-gpio-lines" )) { rv =gpio_mockup_name_lines (dev, chip);if (rv)
Signature · 1 line
2.24.0
wt., 10 gru 2019 o 03:15 Kent Gibson <[email protected]> napisał(a):
Quoted 54 lines
Restore the external behavior of gpio-mockup to what it was prior to the change to using
GPIO_LINE_DIRECTION.Signed-off-by Kent Gibson <[email protected]>---
Fix a regression introduced in v5.5-rc1.
The change to
GPIO_LINE_DIRECTIONreversed the polarity of the dir field withingpio-mockup.c, but overlooked inverting the value on initialization and when returned bygpio_mockup_get_direction. The latter is a bug.
The former is a problem for tests which assume initial conditions, specifically the mockup used to initialize chips with all lines as inputs. That superficially appeared to be the case after the previous patch due to the bug ingpio_mockup_get_direction.drivers/gpio/gpio-mockup.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)drivers/gpio/gpio-mockup.c +5
-3@@ -226,7 +226,7 @@ static int gpio_mockup_get_direction(struct gpio_chip *gc, unsigned int offset)int direction;mutex_lock (&chip->lock); - direction = !chip->lines[offset].dir; + direction = chip->lines[offset].dir;mutex_unlock (&chip->lock);return direction;@@ -395,7 +395,7 @@ static int gpio_mockup_probe(struct platform_device *pdev)struct gpio_chip *gc;struct device *dev;const char *name; -int rv, base; +int rv, base, i;u16 ngpio; dev = &pdev->dev;@@ -447,6 +447,9 @@ static int gpio_mockup_probe(struct platform_device *pdev)if (!chip->lines)return -ENOMEM ; +for (i =0 ; i < gc->ngpio; i++) + chip->lines[i].dir =GPIO_LINE_DIRECTION_IN ; +if (device_property_read_bool (dev,"named-gpio-lines" )) { rv =gpio_mockup_name_lines (dev, chip);if (rv) --
Quoted 2 lines
2.24.0
Hi Kent,
I was applying and testing your libgpiod series and noticed that the gpio-tools tests fail after applying patches 16 & 17 (with linux v5.5-rc1). Is this fix related to this?
Bart
Quoted 60 lines · Bartosz Golaszewski
wt., 10 gru 2019 o 03:15 Kent Gibson <[email protected]> napisał(a):
Quoted 54 lines
Restore the external behavior of gpio-mockup to what it was prior to the change to using
GPIO_LINE_DIRECTION.Signed-off-by Kent Gibson <[email protected]>---
Fix a regression introduced in v5.5-rc1.
The change to
GPIO_LINE_DIRECTIONreversed the polarity of the dir field withingpio-mockup.c, but overlooked inverting the value on initialization and when returned bygpio_mockup_get_direction. The latter is a bug.
The former is a problem for tests which assume initial conditions, specifically the mockup used to initialize chips with all lines as inputs. That superficially appeared to be the case after the previous patch due to the bug ingpio_mockup_get_direction.drivers/gpio/gpio-mockup.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)drivers/gpio/gpio-mockup.c +5
-3@@ -226,7 +226,7 @@ static int gpio_mockup_get_direction(struct gpio_chip *gc, unsigned int offset)int direction;mutex_lock (&chip->lock); - direction = !chip->lines[offset].dir; + direction = chip->lines[offset].dir;mutex_unlock (&chip->lock);return direction;@@ -395,7 +395,7 @@ static int gpio_mockup_probe(struct platform_device *pdev)struct gpio_chip *gc;struct device *dev;const char *name; -int rv, base; +int rv, base, i;u16 ngpio; dev = &pdev->dev;@@ -447,6 +447,9 @@ static int gpio_mockup_probe(struct platform_device *pdev)if (!chip->lines)return -ENOMEM ; +for (i =0 ; i < gc->ngpio; i++) + chip->lines[i].dir =GPIO_LINE_DIRECTION_IN ; +if (device_property_read_bool (dev,"named-gpio-lines" )) { rv =gpio_mockup_name_lines (dev, chip);if (rv) --Quoted 2 lines
2.24.0
Hi Kent,
Quoted 4 lines
I was applying and testing your libgpiod series and noticed that the gpio-tools tests fail after applying patches 16 & 17 (with linux v5.5-rc1). Is this fix related to this?
I don't think so. I've only been able to trip this problem with a couple of corner cases in my Go uapi test suite.
I have been unable to reproduce it with the tools as it requires multiple requests with the same chip fd, including an as-is, to trip.
And running the libgpiod tests against v5.5-rc1 works for me. Can you provide more details as to the errors you are seeing?
Btw, I was writing tests for your LINEINFO_WATCH patch v2, which I was applying to v5.5-rc1, when I ran across this. That works ok if I __packed the changed struct.
And I can confirm that patch v2 doesn't isolate watches on different chip fds.
Kent.
wt., 10 gru 2019 o 15:55 Kent Gibson <[email protected]> napisał(a):
Quoted 67 lines · Bartosz Golaszewski
Quoted 60 lines
wt., 10 gru 2019 o 03:15 Kent Gibson <[email protected]> napisał(a):
Quoted 54 lines
Restore the external behavior of gpio-mockup to what it was prior to the change to using
GPIO_LINE_DIRECTION.Signed-off-by Kent Gibson <[email protected]>---
Fix a regression introduced in v5.5-rc1.
The change to
GPIO_LINE_DIRECTIONreversed the polarity of the dir field withingpio-mockup.c, but overlooked inverting the value on initialization and when returned bygpio_mockup_get_direction. The latter is a bug.
The former is a problem for tests which assume initial conditions, specifically the mockup used to initialize chips with all lines as inputs. That superficially appeared to be the case after the previous patch due to the bug ingpio_mockup_get_direction.drivers/gpio/gpio-mockup.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)drivers/gpio/gpio-mockup.c +5
-3@@ -226,7 +226,7 @@ static int gpio_mockup_get_direction(struct gpio_chip *gc, unsigned int offset)int direction;mutex_lock (&chip->lock); - direction = !chip->lines[offset].dir; + direction = chip->lines[offset].dir;mutex_unlock (&chip->lock);return direction;@@ -395,7 +395,7 @@ static int gpio_mockup_probe(struct platform_device *pdev)struct gpio_chip *gc;struct device *dev;const char *name; -int rv, base; +int rv, base, i;u16 ngpio; dev = &pdev->dev;@@ -447,6 +447,9 @@ static int gpio_mockup_probe(struct platform_device *pdev)if (!chip->lines)return -ENOMEM ; +for (i =0 ; i < gc->ngpio; i++) + chip->lines[i].dir =GPIO_LINE_DIRECTION_IN ; +if (device_property_read_bool (dev,"named-gpio-lines" )) { rv =gpio_mockup_name_lines (dev, chip);if (rv) --Quoted 2 lines
2.24.0
Hi Kent,
Quoted 4 lines
I was applying and testing your libgpiod series and noticed that the gpio-tools tests fail after applying patches 16 & 17 (with linux v5.5-rc1). Is this fix related to this?
Quoted 8 lines
I don't think so. I've only been able to trip this problem with a couple of corner cases in my Go uapi test suite.
I have been unable to reproduce it with the tools as it requires multiple requests with the same chip fd, including an as-is, to trip.And running the libgpiod tests against v5.5-rc1 works for me. Can you provide more details as to the errors you are seeing?
Hmm whatever that was, it's gone now. Must have been some leftovers from previous builds. All works now.
Quoted 3 lines
Btw, I was writing tests for your
LINEINFO_WATCHpatch v2, which I was applying to v5.5-rc1, when I ran across this. That works ok if I__packedthe changed struct.
These things can still change, so don't spend too much time on it yet. :)
Since the lineinfo struct is not __packed, I'd prefer to not use it for any struct embedding it. I'll just add appropriate padding for 64-bit alignment.
Quoted 3 lines
And I can confirm that patch v2 doesn't isolate watches on different chip fds.
Yeah, I'll fix this.
Bart
Quoted 1 line
Kent.
Quoted 77 lines · Bartosz Golaszewski
wt., 10 gru 2019 o 15:55 Kent Gibson <[email protected]> napisał(a):
Quoted 67 lines · Bartosz Golaszewski
Quoted 60 lines
wt., 10 gru 2019 o 03:15 Kent Gibson <[email protected]> napisał(a):
Quoted 54 lines
Restore the external behavior of gpio-mockup to what it was prior to the change to using
GPIO_LINE_DIRECTION.Signed-off-by Kent Gibson <[email protected]>---
Fix a regression introduced in v5.5-rc1.
The change to
GPIO_LINE_DIRECTIONreversed the polarity of the dir field withingpio-mockup.c, but overlooked inverting the value on initialization and when returned bygpio_mockup_get_direction. The latter is a bug.
The former is a problem for tests which assume initial conditions, specifically the mockup used to initialize chips with all lines as inputs. That superficially appeared to be the case after the previous patch due to the bug ingpio_mockup_get_direction.drivers/gpio/gpio-mockup.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)drivers/gpio/gpio-mockup.c +5
-3@@ -226,7 +226,7 @@ static int gpio_mockup_get_direction(struct gpio_chip *gc, unsigned int offset)int direction;mutex_lock (&chip->lock); - direction = !chip->lines[offset].dir; + direction = chip->lines[offset].dir;mutex_unlock (&chip->lock);return direction;@@ -395,7 +395,7 @@ static int gpio_mockup_probe(struct platform_device *pdev)struct gpio_chip *gc;struct device *dev;const char *name; -int rv, base; +int rv, base, i;u16 ngpio; dev = &pdev->dev;@@ -447,6 +447,9 @@ static int gpio_mockup_probe(struct platform_device *pdev)if (!chip->lines)return -ENOMEM ; +for (i =0 ; i < gc->ngpio; i++) + chip->lines[i].dir =GPIO_LINE_DIRECTION_IN ; +if (device_property_read_bool (dev,"named-gpio-lines" )) { rv =gpio_mockup_name_lines (dev, chip);if (rv) --Quoted 2 lines
2.24.0
Hi Kent,
Quoted 4 lines
I was applying and testing your libgpiod series and noticed that the gpio-tools tests fail after applying patches 16 & 17 (with linux v5.5-rc1). Is this fix related to this?
Quoted 8 lines
I don't think so. I've only been able to trip this problem with a couple of corner cases in my Go uapi test suite.
I have been unable to reproduce it with the tools as it requires multiple requests with the same chip fd, including an as-is, to trip.And running the libgpiod tests against v5.5-rc1 works for me. Can you provide more details as to the errors you are seeing?
Quoted 9 lines
Hmm whatever that was, it's gone now. Must have been some leftovers from previous builds. All works now.
Quoted 3 lines
Btw, I was writing tests for your
LINEINFO_WATCHpatch v2, which I was applying to v5.5-rc1, when I ran across this. That works ok if I__packedthe changed struct.These things can still change, so don't spend too much time on it yet. :)
Absolutely. But as this is an ABI addition I wanted to have something to give it a decent workout before it gets applied.
So far the only problems I've found are the alignment and isolation issues already mentioned.
Quoted 4 lines
Since the lineinfo struct is not
__packed, I'd prefer to not use it for any struct embedding it. I'll just add appropriate padding for 64-bit alignment.
Explicit padding for 64-bit alignment makes sense to me.
Quoted 5 lines
Quoted 3 lines
And I can confirm that patch v2 doesn't isolate watches on different chip fds.
Yeah, I'll fix this.
One more thing - since it is possible to lose change events due to fifo overflow, how about adding a seqnum? And if you do end up doing a v2 of the event ABI to fix its alignment, adding one there as well.
Kent.
Quoted 66 lines · Kent Gibson
Quoted 60 lines · Bartosz Golaszewski
wt., 10 gru 2019 o 03:15 Kent Gibson <[email protected]> napisał(a):
Quoted 54 lines
Restore the external behavior of gpio-mockup to what it was prior to the change to using
GPIO_LINE_DIRECTION.Signed-off-by Kent Gibson <[email protected]>---
Fix a regression introduced in v5.5-rc1.
The change to
GPIO_LINE_DIRECTIONreversed the polarity of the dir field withingpio-mockup.c, but overlooked inverting the value on initialization and when returned bygpio_mockup_get_direction. The latter is a bug.
The former is a problem for tests which assume initial conditions, specifically the mockup used to initialize chips with all lines as inputs. That superficially appeared to be the case after the previous patch due to the bug ingpio_mockup_get_direction.drivers/gpio/gpio-mockup.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)drivers/gpio/gpio-mockup.c +5
-3@@ -226,7 +226,7 @@ static int gpio_mockup_get_direction(struct gpio_chip *gc, unsigned int offset)int direction;mutex_lock (&chip->lock); - direction = !chip->lines[offset].dir; + direction = chip->lines[offset].dir;mutex_unlock (&chip->lock);return direction;@@ -395,7 +395,7 @@ static int gpio_mockup_probe(struct platform_device *pdev)struct gpio_chip *gc;struct device *dev;const char *name; -int rv, base; +int rv, base, i;u16 ngpio; dev = &pdev->dev;@@ -447,6 +447,9 @@ static int gpio_mockup_probe(struct platform_device *pdev)if (!chip->lines)return -ENOMEM ; +for (i =0 ; i < gc->ngpio; i++) + chip->lines[i].dir =GPIO_LINE_DIRECTION_IN ; +if (device_property_read_bool (dev,"named-gpio-lines" )) { rv =gpio_mockup_name_lines (dev, chip);if (rv) --Quoted 2 lines
2.24.0
Hi Kent,
Quoted 4 lines
I was applying and testing your libgpiod series and noticed that the gpio-tools tests fail after applying patches 16 & 17 (with linux v5.5-rc1). Is this fix related to this?
Quoted 5 lines
I don't think so. I've only been able to trip this problem with a couple of corner cases in my Go uapi test suite.
I have been unable to reproduce it with the tools as it requires multiple requests with the same chip fd, including an as-is, to trip.
It turns out that I can reproduce the bug with my gpiod tools:
root@firefly:/home/kent/gpiod/cmd/gpiodctl# uname -a Linux firefly 5.5.0-rc1 #23 SMP Mon Dec 9 16:26:33 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux root@firefly:/home/kent/gpiod/cmd/gpiodctl# modprobe gpio-mockup gpio_mockup_ranges=-1,4 root@firefly:/home/kent/gpiod/cmd/gpiodctl# ./gpiodctl get gpiochip0 1 0 root@firefly:/home/kent/gpiod/cmd/gpiodctl# ./gpiodctl info gpiochip0 - 4 lines: line 0: unnamed unused input active-high line 1: unnamed unused input active-high line 2: unnamed unused input active-high line 3: unnamed unused input active-high root@firefly:/home/kent/gpiod/cmd/gpiodctl# ./gpiodctl get --as-is gpiochip0 1 0 root@firefly:/home/kent/gpiod/cmd/gpiodctl# ./gpiodctl info gpiochip0 - 4 lines: line 0: unnamed unused input active-high line 1: unnamed unused output active-high line 2: unnamed unused input active-high line 3: unnamed unused input active-high
Note that the line 1 direction has flipped for no reason.
With the patched kernel that doesn't happen:
root@firefly:/home/kent/gpiod/cmd/gpiodctl# uname -a Linux firefly 5.5.0-rc1+ #27 SMP Tue Dec 10 01:07:59 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux root@firefly:/home/kent/gpiod/cmd/gpiodctl# modprobe gpio-mockup gpio_mockup_ranges=-1,4 root@firefly:/home/kent/gpiod/cmd/gpiodctl# ./gpiodctl get gpiochip0 1 0 root@firefly:/home/kent/gpiod/cmd/gpiodctl# ./gpiodctl info gpiochip0 - 4 lines: line 0: unnamed unused input active-high line 1: unnamed unused input active-high line 2: unnamed unused input active-high line 3: unnamed unused input active-high root@firefly:/home/kent/gpiod/cmd/gpiodctl# ./gpiodctl get --as-is gpiochip0 1 0 root@firefly:/home/kent/gpiod/cmd/gpiodctl# ./gpiodctl info gpiochip0 - 4 lines: line 0: unnamed unused input active-high line 1: unnamed unused input active-high line 2: unnamed unused input active-high line 3: unnamed unused input active-high
I would prefer to demonstrate this with the libgpiod tools, but they don't support as-is on gets. I recall suggesting adding it and you asking why - who would need it. This is a concrete example of my response at the time - so you can exercise the full API for testing.
Cheers,
Kent.
wt., 10 gru 2019 o 03:15 Kent Gibson <[email protected]> napisał(a):
Quoted 5 lines
Restore the external behavior of gpio-mockup to what it was prior to the change to using
GPIO_LINE_DIRECTION.Signed-off-by Kent Gibson <[email protected]>
Please add the Fixes: tag with the commit this patch fixes. See other commits that Cc the stable branch.
Bart
Quoted 1 line
---