gpio: mockup: misc updates

PATCH In review Bartosz Golaszewski 3 participants 24 messages Nov 8, 2018 – Nov 20, 2018

Subsystems (2)

GPIO GPIO mockup driver

lore

Share & tools

lore

Bartosz Golaszewski

author

2018-11-08 16:52 UTC #0 lore raw

cover gpio: mockup: misc updates

The first patch is a fix that should go in for 4.20.

Next two patches introduce features for 4.21 - lock protection of the dummy chip structures and implementation of the get_multiple() callback.

Bartosz Golaszewski (3):
gpio: mockup: fix indicated direction
gpio: mockup: add locking
gpio: mockup: implement get_multiple()

 drivers/gpio/gpio-mockup.c | 71 +++++++++++++++++++++++++++++++++-----
 1 file changed, 62 insertions(+), 9 deletions(-)

Signature · 1 line

2.19.1

Bartosz Golaszewski

author

2018-11-08 16:52 UTC #1 lore raw

PATCH 1/3 gpio: mockup: fix indicated direction

Commit 3edfb7bd76bd ("gpiolib: Show correct direction from the beginning") fixed an existing issue but broke libgpiod tests by changing the default direction of dummy lines to output.

We don't break user-space so make gpio-mockup behave as before.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/gpio/gpio-mockup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/gpio/gpio-mockup.c +3 -3

index 8269cffc2967..6a50f9f59c90 100644
@@ -35,8 +35,8 @@
 #define gpio_mockup_err(...)	pr_err(GPIO_MOCKUP_NAME ": " __VA_ARGS__)
 
 enum {
-	GPIO_MOCKUP_DIR_OUT = 0,
-	GPIO_MOCKUP_DIR_IN = 1,
+	GPIO_MOCKUP_DIR_IN = 0,
+	GPIO_MOCKUP_DIR_OUT = 1,
 };
 
 /*

@@ -131,7 +131,7 @@ static int gpio_mockup_get_direction(struct gpio_chip *gc, unsigned int offset)
 {
 	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
 
-	return chip->lines[offset].dir;
+	return !chip->lines[offset].dir;
 }
 
 static int gpio_mockup_to_irq(struct gpio_chip *gc, unsigned int offset)

Signature · 1 line

2.19.1

Uwe Kleine-König

2018-11-08 20:35 UTC #2 lore raw

Re: [PATCH 1/3] gpio: mockup: fix indicated direction

Hello Bartosz,

Quoted 3 lines · Bartosz Golaszewski

Commit 3edfb7bd76bd ("gpiolib: Show correct direction from the beginning") fixed an existing issue but broke libgpiod tests by changing the default direction of dummy lines to output.

The indicated commit only changed what was shown in debugfs, but didn't touch the actual direction of a GPIO, doesn't it? If someone called gpiod_get_direction before it would have returned "output", too, unless I miss something.

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Bartosz Golaszewski

author

2018-11-09 11:13 UTC #3 lore raw

Re: [PATCH 1/3] gpio: mockup: fix indicated direction

czw., 8 lis 2018 o 21:35 Uwe Kleine-König
<[email protected]> napisał(a):

Quoted 4 lines

Hello Bartosz,
On Thu, Nov 08, 2018 at 05:52:53PM +0100, Bartosz Golaszewski wrote:

Quoted 9 lines

Quoted 3 lines
Commit 3edfb7bd76bd ("gpiolib: Show correct direction from the beginning") fixed an existing issue but broke libgpiod tests by changing the default direction of dummy lines to output.
The indicated commit only changed what was shown in debugfs, but didn't touch the actual direction of a GPIO, doesn't it? If someone called gpiod_get_direction before it would have returned "output", too, unless I miss something.

This commit (3edfb7bd76bd) sets the correct direction of the line by actually calling get_direction() instead of assuming input if direction_input is not NULL. It just so happened that previously the default direction of gpio-mockup lines was output but it would be displayed as input due to this inconsistency.

My commit fixes it by simply setting the value of GPIO_MOCKUP_DIR_IN to 0 so that when we kzalloc the line structure, we'll get the right direction automatically.

Bart

Uwe Kleine-König

2018-11-09 11:54 UTC #4 lore raw

Re: [PATCH 1/3] gpio: mockup: fix indicated direction

Hello Bartosz,

Quoted 13 lines · Bartosz Golaszewski

czw., 8 lis 2018 o 21:35 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 10 lines · Bartosz Golaszewski
Quoted 3 lines
Commit 3edfb7bd76bd ("gpiolib: Show correct direction from the beginning") fixed an existing issue but broke libgpiod tests by changing the default direction of dummy lines to output.
The indicated commit only changed what was shown in debugfs, but didn't touch the actual direction of a GPIO, doesn't it? If someone called gpiod_get_direction before it would have returned "output", too, unless I miss something.

Quoted 5 lines

This commit (3edfb7bd76bd) sets the correct direction of the line by actually calling get_direction() instead of assuming input if direction_input is not NULL. It just so happened that previously the default direction of gpio-mockup lines was output but it would be displayed as input due to this inconsistency.

Does the test that fails since 3edfb7bd76bd use debugfs/gpio to find out the direction of a gpio? I'd say it's a bad idea to depend on this as (AFAICT) debugfs is only a debug aid and you shouldn't depend on stuff contained in it.

I'd weaken the commit log a bit to not claim that the commit broke libgpiod but that it only made the inconsistency visible. After all this commit didn't "change the default direction of dummy lines to output", they were an output already before.

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Bartosz Golaszewski

author

2018-11-09 12:24 UTC #5 lore raw

Re: [PATCH 1/3] gpio: mockup: fix indicated direction

pt., 9 lis 2018 o 12:54 Uwe Kleine-König
<[email protected]> napisał(a):

Quoted 23 lines

Hello Bartosz,
Quoted 13 lines · Bartosz Golaszewski
czw., 8 lis 2018 o 21:35 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 10 lines · Bartosz Golaszewski
Quoted 3 lines
Commit 3edfb7bd76bd ("gpiolib: Show correct direction from the beginning") fixed an existing issue but broke libgpiod tests by changing the default direction of dummy lines to output.
The indicated commit only changed what was shown in debugfs, but didn't touch the actual direction of a GPIO, doesn't it? If someone called gpiod_get_direction before it would have returned "output", too, unless I miss something.
Quoted 5 lines
This commit (3edfb7bd76bd) sets the correct direction of the line by actually calling get_direction() instead of assuming input if direction_input is not NULL. It just so happened that previously the default direction of gpio-mockup lines was output but it would be displayed as input due to this inconsistency.

Quoted 5 lines

Does the test that fails since 3edfb7bd76bd use debugfs/gpio to find out the direction of a gpio? I'd say it's a bad idea to depend on this as (AFAICT) debugfs is only a debug aid and you shouldn't depend on stuff contained in it.

No, this is what we get from the LINEINFO ioctl(). I only use debugfs for triggering dummy interrupts.

Quoted 5 lines

I'd weaken the commit log a bit to not claim that the commit broke libgpiod but that it only made the inconsistency visible. After all this commit didn't "change the default direction of dummy lines to output", they were an output already before.

Well it did break the tests you know. ;)

I guess no harm in rewording the message.

Bart

Uwe Kleine-König

2018-11-09 13:10 UTC #6 lore raw

Re: [PATCH 1/3] gpio: mockup: fix indicated direction

Hello,

Quoted 22 lines · Bartosz Golaszewski

pt., 9 lis 2018 o 12:54 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 20 lines · Bartosz Golaszewski
Quoted 18 lines
czw., 8 lis 2018 o 21:35 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 10 lines · Bartosz Golaszewski
Quoted 3 lines
Commit 3edfb7bd76bd ("gpiolib: Show correct direction from the beginning") fixed an existing issue but broke libgpiod tests by changing the default direction of dummy lines to output.
The indicated commit only changed what was shown in debugfs, but didn't touch the actual direction of a GPIO, doesn't it? If someone called gpiod_get_direction before it would have returned "output", too, unless I miss something.
This commit (3edfb7bd76bd) sets the correct direction of the line by actually calling get_direction() instead of assuming input if direction_input is not NULL. It just so happened that previously the default direction of gpio-mockup lines was output but it would be displayed as input due to this inconsistency.

Quoted 7 lines

Quoted 4 lines
Does the test that fails since 3edfb7bd76bd use debugfs/gpio to find out the direction of a gpio? I'd say it's a bad idea to depend on this as (AFAICT) debugfs is only a debug aid and you shouldn't depend on stuff contained in it.
No, this is what we get from the LINEINFO ioctl(). I only use debugfs for triggering dummy interrupts.

A right, gpio_ioctl uses these flags, too.

Quoted 6 lines

Quoted 4 lines
I'd weaken the commit log a bit to not claim that the commit broke libgpiod but that it only made the inconsistency visible. After all this commit didn't "change the default direction of dummy lines to output", they were an output already before.
Well it did break the tests you know. ;)

Which test failed exactly?

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Bartosz Golaszewski

author

2018-11-09 13:53 UTC #7 lore raw

Re: [PATCH 1/3] gpio: mockup: fix indicated direction

pt., 9 lis 2018 o 14:10 Uwe Kleine-König
<[email protected]> napisał(a):

Quoted 36 lines

Hello,
Quoted 22 lines · Bartosz Golaszewski
pt., 9 lis 2018 o 12:54 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 20 lines · Bartosz Golaszewski
Quoted 18 lines
czw., 8 lis 2018 o 21:35 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 10 lines · Bartosz Golaszewski
Quoted 3 lines
Commit 3edfb7bd76bd ("gpiolib: Show correct direction from the beginning") fixed an existing issue but broke libgpiod tests by changing the default direction of dummy lines to output.
The indicated commit only changed what was shown in debugfs, but didn't touch the actual direction of a GPIO, doesn't it? If someone called gpiod_get_direction before it would have returned "output", too, unless I miss something.
This commit (3edfb7bd76bd) sets the correct direction of the line by actually calling get_direction() instead of assuming input if direction_input is not NULL. It just so happened that previously the default direction of gpio-mockup lines was output but it would be displayed as input due to this inconsistency.
Quoted 7 lines
Quoted 4 lines
Does the test that fails since 3edfb7bd76bd use debugfs/gpio to find out the direction of a gpio? I'd say it's a bad idea to depend on this as (AFAICT) debugfs is only a debug aid and you shouldn't depend on stuff contained in it.
No, this is what we get from the LINEINFO ioctl(). I only use debugfs for triggering dummy interrupts.
A right, gpio_ioctl uses these flags, too.

Quoted 9 lines

Quoted 6 lines
Quoted 4 lines
I'd weaken the commit log a bit to not claim that the commit broke libgpiod but that it only made the inconsistency visible. After all this commit didn't "change the default direction of dummy lines to output", they were an output already before.
Well it did break the tests you know. ;)
Which test failed exactly?

All gpioinfo tests that check the output of this command and expect to see input as line direction.

Bart

Uwe Kleine-König

2018-11-09 14:39 UTC #8 lore raw

Re: [PATCH 1/3] gpio: mockup: fix indicated direction

Quoted 43 lines · Bartosz Golaszewski

pt., 9 lis 2018 o 14:10 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 41 lines
Hello,
Quoted 22 lines · Bartosz Golaszewski
pt., 9 lis 2018 o 12:54 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 20 lines · Bartosz Golaszewski
Quoted 18 lines
czw., 8 lis 2018 o 21:35 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 10 lines · Bartosz Golaszewski
Quoted 3 lines
Commit 3edfb7bd76bd ("gpiolib: Show correct direction from the beginning") fixed an existing issue but broke libgpiod tests by changing the default direction of dummy lines to output.
The indicated commit only changed what was shown in debugfs, but didn't touch the actual direction of a GPIO, doesn't it? If someone called gpiod_get_direction before it would have returned "output", too, unless I miss something.
This commit (3edfb7bd76bd) sets the correct direction of the line by actually calling get_direction() instead of assuming input if direction_input is not NULL. It just so happened that previously the default direction of gpio-mockup lines was output but it would be displayed as input due to this inconsistency.
Quoted 7 lines
Quoted 4 lines
Does the test that fails since 3edfb7bd76bd use debugfs/gpio to find out the direction of a gpio? I'd say it's a bad idea to depend on this as (AFAICT) debugfs is only a debug aid and you shouldn't depend on stuff contained in it.
No, this is what we get from the LINEINFO ioctl(). I only use debugfs for triggering dummy interrupts.
A right, gpio_ioctl uses these flags, too.
Quoted 5 lines
Quoted 4 lines
I'd weaken the commit log a bit to not claim that the commit broke libgpiod but that it only made the inconsistency visible. After all this commit didn't "change the default direction of dummy lines to output", they were an output already before.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Well it did break the tests you know. ;)
Which test failed exactly?
All gpioinfo tests that check the output of this command and expect to see input as line direction.

This wasn't the answer I expected. The background of my question was:
This failing test seems to expect that a given GPIO is an input. If that expectation already exists after the gpio is only requested, then the test is broken and a fix is necessary there.

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Bartosz Golaszewski

author

2018-11-09 15:24 UTC #9 lore raw

Re: [PATCH 1/3] gpio: mockup: fix indicated direction

pt., 9 lis 2018 o 15:39 Uwe Kleine-König
<[email protected]> napisał(a):

Quoted 50 lines · Bartosz Golaszewski

Quoted 48 lines
pt., 9 lis 2018 o 14:10 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 45 lines
Hello,
Quoted 22 lines · Bartosz Golaszewski
pt., 9 lis 2018 o 12:54 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 20 lines · Bartosz Golaszewski
Quoted 18 lines
czw., 8 lis 2018 o 21:35 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 10 lines · Bartosz Golaszewski
Quoted 3 lines
Commit 3edfb7bd76bd ("gpiolib: Show correct direction from the beginning") fixed an existing issue but broke libgpiod tests by changing the default direction of dummy lines to output.
The indicated commit only changed what was shown in debugfs, but didn't touch the actual direction of a GPIO, doesn't it? If someone called gpiod_get_direction before it would have returned "output", too, unless I miss something.
This commit (3edfb7bd76bd) sets the correct direction of the line by actually calling get_direction() instead of assuming input if direction_input is not NULL. It just so happened that previously the default direction of gpio-mockup lines was output but it would be displayed as input due to this inconsistency.
Quoted 7 lines
Quoted 4 lines
Does the test that fails since 3edfb7bd76bd use debugfs/gpio to find out the direction of a gpio? I'd say it's a bad idea to depend on this as (AFAICT) debugfs is only a debug aid and you shouldn't depend on stuff contained in it.
No, this is what we get from the LINEINFO ioctl(). I only use debugfs for triggering dummy interrupts.
A right, gpio_ioctl uses these flags, too.
Quoted 6 lines
Quoted 4 lines
I'd weaken the commit log a bit to not claim that the commit broke libgpiod but that it only made the inconsistency visible. After all this commit didn't "change the default direction of dummy lines to output", they were an output already before.
Well it did break the tests you know. ;)
Which test failed exactly?

Quoted 8 lines

Quoted 2 lines
All gpioinfo tests that check the output of this command and expect to see input as line direction.
This wasn't the answer I expected. The background of my question was:
This failing test seems to expect that a given GPIO is an input. If that expectation already exists after the gpio is only requested, then the test is broken and a fix is necessary there.

The test is only expected to work with gpio-mockup which is a dummy testing module. I believe its behavior should be as deterministic as possible to the point where newly created chips always have the same direction.

Bart

Uwe Kleine-König

2018-11-09 17:03 UTC #10 lore raw

Re: [PATCH 1/3] gpio: mockup: fix indicated direction

Hello Bartosz,

Quoted 15 lines · Bartosz Golaszewski

pt., 9 lis 2018 o 15:39 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 5 lines · Bartosz Golaszewski
Quoted 4 lines
pt., 9 lis 2018 o 14:10 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 1 line
Which test failed exactly?
Quoted 7 lines
Quoted 2 lines
All gpioinfo tests that check the output of this command and expect to see input as line direction.
This wasn't the answer I expected. The background of my question was:
This failing test seems to expect that a given GPIO is an input. If that expectation already exists after the gpio is only requested, then the test is broken and a fix is necessary there.

Quoted 4 lines

The test is only expected to work with gpio-mockup which is a dummy testing module. I believe its behavior should be as deterministic as possible to the point where newly created chips always have the same direction.

Given that the initial direction of a GPIO isn't fixed in my eyes the test should be able to cope with both possibilities. I'd say it's a bug in the test if it doesn't.

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Bartosz Golaszewski

author

2018-11-09 17:23 UTC #11 lore raw

Re: [PATCH 1/3] gpio: mockup: fix indicated direction

pt., 9 lis 2018 o 18:03 Uwe Kleine-König
<[email protected]> napisał(a):

Quoted 19 lines

Hello Bartosz,
Quoted 15 lines · Bartosz Golaszewski
pt., 9 lis 2018 o 15:39 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 12 lines · Bartosz Golaszewski
Quoted 6 lines
pt., 9 lis 2018 o 14:10 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 1 line
Which test failed exactly?
All gpioinfo tests that check the output of this command and expect to see input as line direction.
This wasn't the answer I expected. The background of my question was:
This failing test seems to expect that a given GPIO is an input. If that expectation already exists after the gpio is only requested, then the test is broken and a fix is necessary there.

Quoted 9 lines

Quoted 4 lines
The test is only expected to work with gpio-mockup which is a dummy testing module. I believe its behavior should be as deterministic as possible to the point where newly created chips always have the same direction.
Given that the initial direction of a GPIO isn't fixed in my eyes the test should be able to cope with both possibilities. I'd say it's a bug in the test if it doesn't.

As I said before: it is and should be fixed in this specific case. This isn't real hardware.

Bart

Uwe Kleine-König

2018-11-11 16:59 UTC #12 lore raw

Re: [PATCH 1/3] gpio: mockup: fix indicated direction

Quoted 26 lines · Bartosz Golaszewski

pt., 9 lis 2018 o 18:03 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 24 lines
Hello Bartosz,
Quoted 19 lines · Bartosz Golaszewski
pt., 9 lis 2018 o 15:39 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 5 lines · Bartosz Golaszewski
Quoted 4 lines
pt., 9 lis 2018 o 14:10 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 1 line
Which test failed exactly?
Quoted 7 lines
Quoted 2 lines
All gpioinfo tests that check the output of this command and expect to see input as line direction.
This wasn't the answer I expected. The background of my question was:
This failing test seems to expect that a given GPIO is an input. If that expectation already exists after the gpio is only requested, then the test is broken and a fix is necessary there.
The test is only expected to work with gpio-mockup which is a dummy testing module. I believe its behavior should be as deterministic as possible to the point where newly created chips always have the same direction.

Quoted 7 lines

Quoted 4 lines
Given that the initial direction of a GPIO isn't fixed in my eyes the test should be able to cope with both possibilities. I'd say it's a bug in the test if it doesn't.
As I said before: it is and should be fixed in this specific case. This isn't real hardware.

Not sure if we agree here yet. What do you want to fix? The driver or the test?

In my eyes test driven development is great. But if something breaks because the test is wrong, please don't "fix" the system to repair the test, but modify the test to be able to handle reality.

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Bartosz Golaszewski

author

2018-11-12 14:14 UTC #13 lore raw

Re: [PATCH 1/3] gpio: mockup: fix indicated direction

niedz., 11 lis 2018 o 17:59 Uwe Kleine-König
<[email protected]> napisał(a):

Quoted 36 lines · Bartosz Golaszewski

Quoted 26 lines
pt., 9 lis 2018 o 18:03 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 24 lines
Hello Bartosz,
Quoted 19 lines · Bartosz Golaszewski
pt., 9 lis 2018 o 15:39 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 5 lines · Bartosz Golaszewski
Quoted 4 lines
pt., 9 lis 2018 o 14:10 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 1 line
Which test failed exactly?
Quoted 7 lines
Quoted 2 lines
All gpioinfo tests that check the output of this command and expect to see input as line direction.
This wasn't the answer I expected. The background of my question was:
This failing test seems to expect that a given GPIO is an input. If that expectation already exists after the gpio is only requested, then the test is broken and a fix is necessary there.
The test is only expected to work with gpio-mockup which is a dummy testing module. I believe its behavior should be as deterministic as possible to the point where newly created chips always have the same direction.
Quoted 7 lines
Quoted 4 lines
Given that the initial direction of a GPIO isn't fixed in my eyes the test should be able to cope with both possibilities. I'd say it's a bug in the test if it doesn't.
As I said before: it is and should be fixed in this specific case. This isn't real hardware.

Quoted 7 lines

Not sure if we agree here yet. What do you want to fix? The driver or the test?
In my eyes test driven development is great. But if something breaks because the test is wrong, please don't "fix" the system to repair the test, but modify the test to be able to handle reality.

No, we don't have an agreement. You think I should fix the test, I think the dummy driver should continue behaving like before.

Given that there's no real hardware behind, the direction of newly created dummy lines has always been deterministic - input. Certain tests have been relying on it. I want to keep on doing it. There's no harm. It's not broken logic as the very purpose of this module is to allow for easy testing of the UAPI.

So unless something *else* is wrong with this patch, I intend to push it upstream.

Best regards,
Bartosz Golaszewski

Uwe Kleine-König

2018-11-13 06:57 UTC #14 lore raw

Re: [PATCH 1/3] gpio: mockup: fix indicated direction

Hello Bartosz,

Quoted 2 lines · Bartosz Golaszewski

Quoted 2 lines
Quoted 2 lines
As I said before: it is and should be fixed in this specific case. This isn't real hardware.

Not being "real hardware" is hardly an argument that matters here.

Quoted 10 lines

Quoted 7 lines
Not sure if we agree here yet. What do you want to fix? The driver or the test?
In my eyes test driven development is great. But if something breaks because the test is wrong, please don't "fix" the system to repair the test, but modify the test to be able to handle reality.
No, we don't have an agreement. You think I should fix the test, I think the dummy driver should continue behaving like before.

It's arguable if after your patch the driver behaves as before. IMHO the initial direction was output from the start before. This was just shadowed by the an inconsitency that was fixed some time ago.

Quoted 8 lines

Given that there's no real hardware behind, the direction of newly created dummy lines has always been deterministic - input. Certain tests have been relying on it. I want to keep on doing it. There's no harm. It's not broken logic as the very purpose of this module is to allow for easy testing of the UAPI.
So unless something *else* is wrong with this patch, I intend to push it upstream.

Note I don't oppose to the patch as is. Just the motiviation is wrong and I'd do both: Modify the mockup driver to start with direction=input and modify the tests to not expect this.

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Linus Walleij

2018-11-16 21:38 UTC #15 lore raw

Re: [PATCH 1/3] gpio: mockup: fix indicated direction

In reply to Bartosz Golaszewski

Quoted 7 lines · Bartosz Golaszewski

Commit 3edfb7bd76bd ("gpiolib: Show correct direction from the beginning") fixed an existing issue but broke libgpiod tests by changing the default direction of dummy lines to output.
We don't break user-space so make gpio-mockup behave as before.
Signed-off-by Bartosz Golaszewski <[email protected]>

Patch applied for fixes.

I browsed the debate here. I think this should be applied for fixes, then if the test should be changed, we can do that for devel in that case.

Yours,
Linus Walleij

Bartosz Golaszewski

author

2018-11-08 16:52 UTC #16 lore raw

PATCH 2/3 gpio: mockup: add locking

In reply to Bartosz Golaszewski

While no user reported any race condition problems with gpio-mockup, let's be on the safe side and use a mutex when performing any changes on the dummy chip structures.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/gpio/gpio-mockup.c | 50 ++++++++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 7 deletions(-)

drivers/gpio/gpio-mockup.c +43 -7

index 6a50f9f59c90..3cd92912c414 100644
@@ -54,6 +54,7 @@ struct gpio_mockup_chip {
 	struct gpio_mockup_line_status *lines;
 	struct irq_sim irqsim;
 	struct dentry *dbg_dir;
+	struct mutex lock;
 };
 
 struct gpio_mockup_dbgfs_private {

@@ -82,29 +83,53 @@ static int gpio_mockup_range_ngpio(unsigned int index)
 	return gpio_mockup_ranges[index * 2 + 1];
 }
 
-static int gpio_mockup_get(struct gpio_chip *gc, unsigned int offset)
+static int __gpio_mockup_get(struct gpio_chip *gc, unsigned int offset)
 {
 	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
 
 	return chip->lines[offset].value;
 }
 
-static void gpio_mockup_set(struct gpio_chip *gc,
-			    unsigned int offset, int value)
+static int gpio_mockup_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
+	int val;
+
+	mutex_lock(&chip->lock);
+	val = __gpio_mockup_get(gc, offset);
+	mutex_unlock(&chip->lock);
+
+	return val;
+}
+
+static void __gpio_mockup_set(struct gpio_chip *gc,
+			      unsigned int offset, int value)
 {
 	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
 
 	chip->lines[offset].value = !!value;
 }
 
+static void gpio_mockup_set(struct gpio_chip *gc,
+			   unsigned int offset, int value)
+{
+	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
+
+	mutex_lock(&chip->lock);
+	__gpio_mockup_set(gc, offset, value);
+	mutex_unlock(&chip->lock);
+}
+
 static void gpio_mockup_set_multiple(struct gpio_chip *gc,
 				     unsigned long *mask, unsigned long *bits)
 {
+	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
 	unsigned int bit;
 
+	mutex_lock(&chip->lock);
 	for_each_set_bit(bit, mask, gc->ngpio)
-		gpio_mockup_set(gc, bit, test_bit(bit, bits));
-
+		__gpio_mockup_set(gc, bit, test_bit(bit, bits));
+	mutex_unlock(&chip->lock);
 }
 
 static int gpio_mockup_dirout(struct gpio_chip *gc,

@@ -112,8 +137,10 @@ static int gpio_mockup_dirout(struct gpio_chip *gc,
 {
 	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
 
-	gpio_mockup_set(gc, offset, value);
+	mutex_lock(&chip->lock);
+	__gpio_mockup_set(gc, offset, value);
 	chip->lines[offset].dir = GPIO_MOCKUP_DIR_OUT;
+	mutex_unlock(&chip->lock);
 
 	return 0;
 }

@@ -122,7 +149,9 @@ static int gpio_mockup_dirin(struct gpio_chip *gc, unsigned int offset)
 {
 	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
 
+	mutex_lock(&chip->lock);
 	chip->lines[offset].dir = GPIO_MOCKUP_DIR_IN;
+	mutex_unlock(&chip->lock);
 
 	return 0;
 }

@@ -130,8 +159,13 @@ static int gpio_mockup_dirin(struct gpio_chip *gc, unsigned int offset)
 static int gpio_mockup_get_direction(struct gpio_chip *gc, unsigned int offset)
 {
 	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
+	int direction;
 
-	return !chip->lines[offset].dir;
+	mutex_lock(&chip->lock);
+	direction = !chip->lines[offset].dir;
+	mutex_unlock(&chip->lock);
+
+	return direction;
 }
 
 static int gpio_mockup_to_irq(struct gpio_chip *gc, unsigned int offset)

@@ -283,6 +317,8 @@ static int gpio_mockup_probe(struct platform_device *pdev)
 			return -ENOMEM;
 	}
 
+	mutex_init(&chip->lock);
+
 	gc = &chip->gc;
 	gc->base = base;
 	gc->ngpio = ngpio;

Signature · 1 line

2.19.1

Uwe Kleine-König

Acked-by on v1 2/3

2018-11-08 21:13 UTC #17 lore raw

Re: [PATCH 2/3] gpio: mockup: add locking

Quoted 37 lines · Bartosz Golaszewski

While no user reported any race condition problems with gpio-mockup, let's be on the safe side and use a mutex when performing any changes on the dummy chip structures.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/gpio/gpio-mockup.c | 50 ++++++++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 7 deletions(-)

drivers/gpio/gpio-mockup.c +3 -3

index 6a50f9f59c90..3cd92912c414 100644
@@ -54,6 +54,7 @@ struct gpio_mockup_chip {
 	struct gpio_mockup_line_status *lines;
 	struct irq_sim irqsim;
 	struct dentry *dbg_dir;
+	struct mutex lock;
 };
 
 struct gpio_mockup_dbgfs_private {

@@ -82,29 +83,53 @@ static int gpio_mockup_range_ngpio(unsigned int index)
 	return gpio_mockup_ranges[index * 2 + 1];
 }
 
-static int gpio_mockup_get(struct gpio_chip *gc, unsigned int offset)
+static int __gpio_mockup_get(struct gpio_chip *gc, unsigned int offset)
 {
 	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
 
 	return chip->lines[offset].value;
 }
 
-static void gpio_mockup_set(struct gpio_chip *gc,
-			    unsigned int offset, int value)
+static int gpio_mockup_get(struct gpio_chip *gc, unsigned int offset)

drivers/gpio/gpio-mockup.c static int gpio_mockup_range_ngpio(unsigned int index) · line 103

Quoted 10 lines

drivers/gpio/gpio-mockup.c +10 -0

+{
+	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
+	int val;
+
+	mutex_lock(&chip->lock);
+	val = __gpio_mockup_get(gc, offset);
+	mutex_unlock(&chip->lock);
+
+	return val;
+}

I think this function doesn't need locking. I returns a single value and if there is a race and some other process currently changes the value it matters little if the return value is zero or not.

But even with this kept unchanged the patch looks good.

Acked-by Uwe Kleine-König <[email protected]>

Thanks
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Linus Walleij

2018-11-16 21:43 UTC #18 lore raw

Re: [PATCH 2/3] gpio: mockup: add locking

In reply to Bartosz Golaszewski

Quoted 6 lines · Bartosz Golaszewski

While no user reported any race condition problems with gpio-mockup, let's be on the safe side and use a mutex when performing any changes on the dummy chip structures.
Suggested-by Uwe Kleine-König <[email protected]>
Signed-off-by Bartosz Golaszewski <[email protected]>

I tried to apply this but it failed, does it require patch 1?

I can pull in the next -rc after I merged the fix in that case and we can apply on top.

__gpio_*
I tend to dislike __underscore_notation because I feel it is semantically ambguous. I prefer a proper name, even to the point that I prefer inner_function_foo over __foo, but it's your driver and I might be a bit grumpy. :)

Yours,
Linus Walleij

Bartosz Golaszewski

author

2018-11-19 09:09 UTC #19 lore raw

Re: [PATCH 2/3] gpio: mockup: add locking

pt., 16 lis 2018 o 22:43 Linus Walleij <[email protected]> napisał(a):

Quoted 12 lines · Bartosz Golaszewski

Quoted 7 lines
While no user reported any race condition problems with gpio-mockup, let's be on the safe side and use a mutex when performing any changes on the dummy chip structures.
Suggested-by Uwe Kleine-König <[email protected]>
Signed-off-by Bartosz Golaszewski <[email protected]>
I tried to apply this but it failed, does it require patch 1?

Yes, because of the change in get_direction().

Quoted 3 lines

I can pull in the next -rc after I merged the fix in that case and we can apply on top.

This is fine, it's aimed for 4.21 anyway.

Quoted 6 lines

__gpio_*
I tend to dislike __underscore_notation because I feel it is semantically ambguous. I prefer a proper name, even to the point that I prefer inner_function_foo over __foo, but it's your driver and I might be a bit grumpy. :)

I think this is a common and intuitive pattern in the kernel codebase. Many subsystems and drivers use '__' to mark functions that execute internal logic and expect certain locks to be held etc.

If you don't mind, I'd like to leave it like this.

Bart

Linus Walleij

2018-11-20 08:46 UTC #20 lore raw

Re: [PATCH 2/3] gpio: mockup: add locking

Quoted 11 lines · Bartosz Golaszewski

pt., 16 lis 2018 o 22:43 Linus Walleij <[email protected]> napisał(a):
Quoted 5 lines
__gpio_*
I tend to dislike __underscore_notation because I feel it is semantically ambguous. I prefer a proper name, even to the point that I prefer inner_function_foo over __foo, but it's your driver and I might be a bit grumpy. :)
I think this is a common and intuitive pattern in the kernel codebase. Many subsystems and drivers use '__' to mark functions that execute internal logic and expect certain locks to be held etc.

You say it yourself: interpretation depends on context.

I might be especially stupid for being unable to discern meaning from context in these cases and so what is intuitive for some is just not intuitive for me.

Example:
set_bit() vs __set_bit()

Apparently some kernel developers think it is completely obvious that the latter is the unlocked non-atomic version of set_bit(). However I was confused for years with no idea as to what the difference was.

Had it simply been named set_bit_nonatomic(), at the cost of a few characters, confusion on my part would be avoided and at least to me the world would be a better place.

Quoted 1 line

If you don't mind, I'd like to leave it like this.

No big deal, keep it as is :)

Yours,
Linus Walleij

Bartosz Golaszewski

author

2018-11-08 16:52 UTC #21 lore raw

PATCH 3/3 gpio: mockup: implement get_multiple()

In reply to Bartosz Golaszewski

We already support set_multiple(). Implement get_multiple() as well.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/gpio/gpio-mockup.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

drivers/gpio/gpio-mockup.c +17 -0

index 3cd92912c414..a4c054cf9c5f 100644
@@ -102,6 +102,22 @@ static int gpio_mockup_get(struct gpio_chip *gc, unsigned int offset)
 	return val;
 }
 
+static int gpio_mockup_get_multiple(struct gpio_chip *gc,
+				    unsigned long *mask, unsigned long *bits)
+{
+	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
+	unsigned int bit, val;
+
+	mutex_lock(&chip->lock);
+	for_each_set_bit(bit, mask, gc->ngpio) {
+		val = __gpio_mockup_get(gc, bit);
+		__assign_bit(bit, bits, val);
+	}
+	mutex_unlock(&chip->lock);
+
+	return 0;
+}
+
 static void __gpio_mockup_set(struct gpio_chip *gc,
 			      unsigned int offset, int value)
 {

@@ -327,6 +343,7 @@ static int gpio_mockup_probe(struct platform_device *pdev)
 	gc->parent = dev;
 	gc->get = gpio_mockup_get;
 	gc->set = gpio_mockup_set;
+	gc->get_multiple = gpio_mockup_get_multiple;
 	gc->set_multiple = gpio_mockup_set_multiple;
 	gc->direction_output = gpio_mockup_dirout;
 	gc->direction_input = gpio_mockup_dirin;

Signature · 1 line

2.19.1

Uwe Kleine-König

Acked-by on v1 3/3

2018-11-08 20:36 UTC #22 lore raw

Re: [PATCH 3/3] gpio: mockup: implement get_multiple()

Quoted 3 lines · Bartosz Golaszewski

We already support set_multiple(). Implement get_multiple() as well.
Signed-off-by Bartosz Golaszewski <[email protected]>

Acked-by Uwe Kleine-König <[email protected]>

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Linus Walleij

2018-11-16 21:44 UTC #23 lore raw

Re: [PATCH 3/3] gpio: mockup: implement get_multiple()

In reply to Bartosz Golaszewski

Quoted 3 lines · Bartosz Golaszewski

We already support set_multiple(). Implement get_multiple() as well.
Signed-off-by Bartosz Golaszewski <[email protected]>

Waiting to see how I should apply patch 1+2 but all looks good to me!

Yours,
Linus Walleij