| Message ID | 20191210021525.13455-1-warthog618@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: gpio-mockup: Fix usage of new GPIO_LINE_DIRECTION | expand |
wt., 10 gru 2019 o 03:15 Kent Gibson <warthog618@gmail.com> napisał(a): > > 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 <warthog618@gmail.com> > --- > > 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(-) > > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > index 56d647a30e3e..c4fdc192ea4e 100644 > --- a/drivers/gpio/gpio-mockup.c > +++ b/drivers/gpio/gpio-mockup.c > @@ -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) > -- > 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
On Tue, Dec 10, 2019 at 03:11:12PM +0100, Bartosz Golaszewski wrote: > wt., 10 gru 2019 o 03:15 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > 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 <warthog618@gmail.com> > > --- > > > > 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(-) > > > > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > > index 56d647a30e3e..c4fdc192ea4e 100644 > > --- a/drivers/gpio/gpio-mockup.c > > +++ b/drivers/gpio/gpio-mockup.c > > @@ -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) > > -- > > 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? > 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 <warthog618@gmail.com> napisał(a): > > On Tue, Dec 10, 2019 at 03:11:12PM +0100, Bartosz Golaszewski wrote: > > wt., 10 gru 2019 o 03:15 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > 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 <warthog618@gmail.com> > > > --- > > > > > > 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(-) > > > > > > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > > > index 56d647a30e3e..c4fdc192ea4e 100644 > > > --- a/drivers/gpio/gpio-mockup.c > > > +++ b/drivers/gpio/gpio-mockup.c > > > @@ -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) > > > -- > > > 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? > > > > 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. > 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. 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. > And I can confirm that patch v2 doesn't isolate watches on different > chip fds. > Yeah, I'll fix this. Bart > Kent.
wt., 10 gru 2019 o 03:15 Kent Gibson <warthog618@gmail.com> napisał(a): > > 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 <warthog618@gmail.com> Please add the Fixes: tag with the commit this patch fixes. See other commits that Cc the stable branch. Bart > ---
On Tue, Dec 10, 2019 at 04:34:21PM +0100, Bartosz Golaszewski wrote: > wt., 10 gru 2019 o 15:55 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > On Tue, Dec 10, 2019 at 03:11:12PM +0100, Bartosz Golaszewski wrote: > > > wt., 10 gru 2019 o 03:15 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > 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 <warthog618@gmail.com> > > > > --- > > > > > > > > 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(-) > > > > > > > > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > > > > index 56d647a30e3e..c4fdc192ea4e 100644 > > > > --- a/drivers/gpio/gpio-mockup.c > > > > +++ b/drivers/gpio/gpio-mockup.c > > > > @@ -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) > > > > -- > > > > 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? > > > > > > > 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. > > > 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. > > 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. > 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. > > 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.
On Tue, Dec 10, 2019 at 10:55:15PM +0800, Kent Gibson wrote: > On Tue, Dec 10, 2019 at 03:11:12PM +0100, Bartosz Golaszewski wrote: > > wt., 10 gru 2019 o 03:15 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > 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 <warthog618@gmail.com> > > > --- > > > > > > 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(-) > > > > > > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > > > index 56d647a30e3e..c4fdc192ea4e 100644 > > > --- a/drivers/gpio/gpio-mockup.c > > > +++ b/drivers/gpio/gpio-mockup.c > > > @@ -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) > > > -- > > > 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? > > > > 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.
diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c index 56d647a30e3e..c4fdc192ea4e 100644 --- a/drivers/gpio/gpio-mockup.c +++ b/drivers/gpio/gpio-mockup.c @@ -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)
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 <warthog618@gmail.com> --- 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(-)