Fw: [3.18.3] poll() on gpio pins broken

Fw: [3.18.3] poll() on gpio pins broken

[folkert]

Hi,

For timekeeping I wrote a program which waits for interrupts on
gpio-pins and then tells the local ntp daemon the clock offset.
I'm aware of the pps support in recent kernel but that does not work
(yet) on all platforms (eg cubieboard 1).

This has worked for quite some time but no longer.

Until at least kernel 3.12 I could do:

// export gpio pin
// set direction to in
// set direction to rising
int fd = open("/sys.../value", O_RDONLY);
fdset[0].fd = fd;
fdset[0].events = POLLPRI;
fdset[0].revents = 0;
poll(fdset, 1, -1);
// at this point pin went high

So: I would setup a gpio-pin using /sys-files, and then use poll() to
wait for the interrupt triggered by the level of the gpio pin going
high.
This worked fine on all raspberry pi's I tried, a nanos g20, a beagle
bone black and a cubieboard 1.

Since kernel 3.18.3 (.4 as well) this no longer works: 

1422542104.894908947] interrupt #2459, offset -0.105091s
1422542104.895162937] interrupt #2460, offset -0.104837s
1422542104.895408928] interrupt #2461, offset -0.104591s
1422542104.895650919] interrupt #2462, offset -0.104349s
1422542104.896293896] interrupt #2463, offset -0.103706s
...

As you can see suddenly the poll returns immediately.

strace learns me:
poll([{fd=3, events=POLLPRI}], 1, -1)   = 1 ([{fd=3, revents=POLLPRI|POLLERR}])

So apparently somewhere between 3.12 and 3.18 it is no longer
allowed/possible to do poll() on a gpio-pin value device.

Am I right that something broke the abi? Or have I been doing it wrong
and did that became visible?
Please advise.

thanks

Folkert van Heusden

-- 

Re: Fw: [3.18.3] poll() on gpio pins broken

[Michael Welling]

On Thu, Jan 29, 2015 at 04:29:02PM +0100, folkert wrote:
> Hi,
> 
> For timekeeping I wrote a program which waits for interrupts on
> gpio-pins and then tells the local ntp daemon the clock offset.
> I'm aware of the pps support in recent kernel but that does not work
> (yet) on all platforms (eg cubieboard 1).
> 
> This has worked for quite some time but no longer.
> 
> Until at least kernel 3.12 I could do:
> 
> // export gpio pin
> // set direction to in
> // set direction to rising
> int fd = open("/sys.../value", O_RDONLY);
> fdset[0].fd = fd;
> fdset[0].events = POLLPRI;
> fdset[0].revents = 0;
> poll(fdset, 1, -1);
> // at this point pin went high

Try using lseek before reading the data after the poll.

EX.
	if (fdset[0].revents & POLLPRI) {
		lseek(fdset[0].fd, 0, SEEK_SET);
		len = read(fdset[0].fd, buf, MAX_BUF);
		.
		.
	}

See if this helps.

> 
> So: I would setup a gpio-pin using /sys-files, and then use poll() to
> wait for the interrupt triggered by the level of the gpio pin going
> high.
> This worked fine on all raspberry pi's I tried, a nanos g20, a beagle
> bone black and a cubieboard 1.
> 
> Since kernel 3.18.3 (.4 as well) this no longer works: 
> 
> 1422542104.894908947] interrupt #2459, offset -0.105091s
> 1422542104.895162937] interrupt #2460, offset -0.104837s
> 1422542104.895408928] interrupt #2461, offset -0.104591s
> 1422542104.895650919] interrupt #2462, offset -0.104349s
> 1422542104.896293896] interrupt #2463, offset -0.103706s
> ...
> 
> As you can see suddenly the poll returns immediately.
> 
> strace learns me:
> poll([{fd=3, events=POLLPRI}], 1, -1)   = 1 ([{fd=3, revents=POLLPRI|POLLERR}])
> 
> So apparently somewhere between 3.12 and 3.18 it is no longer
> allowed/possible to do poll() on a gpio-pin value device.
> 
> Am I right that something broke the abi? Or have I been doing it wrong
> and did that became visible?
> Please advise.
> 
> thanks
> 
> Folkert van Heusden
> 
> -- 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Fw: [3.18.3] poll() on gpio pins broken

[folkert]

Michael,

> > For timekeeping I wrote a program which waits for interrupts on
> > gpio-pins and then tells the local ntp daemon the clock offset.
> > I'm aware of the pps support in recent kernel but that does not work
> > (yet) on all platforms (eg cubieboard 1).
> > 
> > This has worked for quite some time but no longer.
> > 
> > Until at least kernel 3.12 I could do:
> > 
> > // export gpio pin
> > // set direction to in
> > // set direction to rising
> > int fd = open("/sys.../value", O_RDONLY);
> > fdset[0].fd = fd;
> > fdset[0].events = POLLPRI;
> > fdset[0].revents = 0;
> > poll(fdset, 1, -1);
> > // at this point pin went high
> 
> Try using lseek before reading the data after the poll.
> 
> EX.
> 	if (fdset[0].revents & POLLPRI) {
> 		lseek(fdset[0].fd, 0, SEEK_SET);
> 		len = read(fdset[0].fd, buf, MAX_BUF);
> 		.
> 		.
> 	}
> 
> See if this helps.

Yes, that fixed it!
Great! Thanks!


Folkert van Heusden

-- 
MultiTail na wan makriki wrokosani fu tan luku den logfile nanga san
den commando spiti puru. Piki puru spesrutu sani, wroko nanga difrenti
kroru, tya kon makandra, nanga wan lo moro.
http://www.vanheusden.com/multitail/
----------------------------------------------------------------------
Phone: +31-6-41278122, PGP-key: 1F28D8AE, www.vanheusden.com

Re: Fw: [3.18.3] poll() on gpio pins broken

[Alexandre Courbot]

On Sat, Jan 31, 2015 at 9:33 AM, folkert <folkert@vanheusden.com> wrote:
> Michael,
>
>> > For timekeeping I wrote a program which waits for interrupts on
>> > gpio-pins and then tells the local ntp daemon the clock offset.
>> > I'm aware of the pps support in recent kernel but that does not work
>> > (yet) on all platforms (eg cubieboard 1).
>> >
>> > This has worked for quite some time but no longer.
>> >
>> > Until at least kernel 3.12 I could do:
>> >
>> > // export gpio pin
>> > // set direction to in
>> > // set direction to rising
>> > int fd = open("/sys.../value", O_RDONLY);
>> > fdset[0].fd = fd;
>> > fdset[0].events = POLLPRI;
>> > fdset[0].revents = 0;
>> > poll(fdset, 1, -1);
>> > // at this point pin went high
>>
>> Try using lseek before reading the data after the poll.
>>
>> EX.
>>       if (fdset[0].revents & POLLPRI) {
>>               lseek(fdset[0].fd, 0, SEEK_SET);
>>               len = read(fdset[0].fd, buf, MAX_BUF);
>>               .
>>               .
>>       }
>>
>> See if this helps.
>
> Yes, that fixed it!

Still, shouldn't we consider this as a regression, especially if not
using lseek worked for kernel 3.12 and before?

Linus, what do you think?

Re: Fw: [3.18.3] poll() on gpio pins broken

[folkert]

> >> > For timekeeping I wrote a program which waits for interrupts on
> >> > gpio-pins and then tells the local ntp daemon the clock offset.
> >> > I'm aware of the pps support in recent kernel but that does not work
> >> > (yet) on all platforms (eg cubieboard 1).
> >> >
> >> > This has worked for quite some time but no longer.
> >> >
> >> > Until at least kernel 3.12 I could do:
> >> >
> >> > // export gpio pin
> >> > // set direction to in
> >> > // set direction to rising
> >> > int fd = open("/sys.../value", O_RDONLY);
> >> > fdset[0].fd = fd;
> >> > fdset[0].events = POLLPRI;
> >> > fdset[0].revents = 0;
> >> > poll(fdset, 1, -1);
> >> > // at this point pin went high
> >>
> >> Try using lseek before reading the data after the poll.
> >>
> >> EX.
> >>       if (fdset[0].revents & POLLPRI) {
> >>               lseek(fdset[0].fd, 0, SEEK_SET);
> >>               len = read(fdset[0].fd, buf, MAX_BUF);
> >>               .
> >>               .
> >>       }
> >>
> >> See if this helps.
> >
> > Yes, that fixed it!
> 
> Still, shouldn't we consider this as a regression, especially if not
> using lseek worked for kernel 3.12 and before?
> 
> Linus, what do you think?

In case it is relevant, my code with 3.12 was:

	for(;;)
        {
                  char dummy;
  
                  read(gpio_pps_in_fd, &dummy, 1);
  
                  fdset[0].fd = gpio_pps_in_fd;
                  fdset[0].events = POLLPRI;
                  fdset[0].revents = 0;
  
                  if (poll(fdset, 1, -1) <= 0)
                          error_exit("poll() failed");
  
                  if (likely(fdset[0].revents & POLLPRI))
		  { /* notify ntp */ {
	}

Now with 3.18 I added an lseek before that read and all is fine.


Folkert van Heusden

-- 

Re: Fw: [3.18.3] poll() on gpio pins broken

[Michael Welling]

On Sat, Jan 31, 2015 at 02:39:03PM +0100, Alexandre Courbot wrote:
> On Sat, Jan 31, 2015 at 9:33 AM, folkert <folkert@vanheusden.com> wrote:
> > Michael,
> >
> >> > For timekeeping I wrote a program which waits for interrupts on
> >> > gpio-pins and then tells the local ntp daemon the clock offset.
> >> > I'm aware of the pps support in recent kernel but that does not work
> >> > (yet) on all platforms (eg cubieboard 1).
> >> >
> >> > This has worked for quite some time but no longer.
> >> >
> >> > Until at least kernel 3.12 I could do:
> >> >
> >> > // export gpio pin
> >> > // set direction to in
> >> > // set direction to rising
> >> > int fd = open("/sys.../value", O_RDONLY);
> >> > fdset[0].fd = fd;
> >> > fdset[0].events = POLLPRI;
> >> > fdset[0].revents = 0;
> >> > poll(fdset, 1, -1);
> >> > // at this point pin went high
> >>
> >> Try using lseek before reading the data after the poll.
> >>
> >> EX.
> >>       if (fdset[0].revents & POLLPRI) {
> >>               lseek(fdset[0].fd, 0, SEEK_SET);
> >>               len = read(fdset[0].fd, buf, MAX_BUF);
> >>               .
> >>               .
> >>       }
> >>
> >> See if this helps.
> >
> > Yes, that fixed it!
> 
> Still, shouldn't we consider this as a regression, especially if not
> using lseek worked for kernel 3.12 and before?

Perhaps this is a side effect of the sysfs to kernfs change over to happened in
3.14.

Looking at 'kernfs_fop_poll' my guess is that 'kernfs_get_active' is returning NULL
immediately returning from the poll command and causing described behavior.

http://lxr.free-electrons.com/source/fs/kernfs/file.c#L763

> 
> Linus, what do you think?

Re: Fw: [3.18.3] poll() on gpio pins broken

[Linus Walleij]

On Tue, Feb 3, 2015 at 5:03 PM, Michael Welling <mwelling@ieee.org> wrote:
> On Sat, Jan 31, 2015 at 02:39:03PM +0100, Alexandre Courbot wrote:
>> On Sat, Jan 31, 2015 at 9:33 AM, folkert <folkert@vanheusden.com> wrote:
>> > Michael,
>> >
>> >> > For timekeeping I wrote a program which waits for interrupts on
>> >> > gpio-pins and then tells the local ntp daemon the clock offset.
>> >> > I'm aware of the pps support in recent kernel but that does not work
>> >> > (yet) on all platforms (eg cubieboard 1).
>> >> >
>> >> > This has worked for quite some time but no longer.
>> >> >
>> >> > Until at least kernel 3.12 I could do:
>> >> >
>> >> > // export gpio pin
>> >> > // set direction to in
>> >> > // set direction to rising
>> >> > int fd = open("/sys.../value", O_RDONLY);
>> >> > fdset[0].fd = fd;
>> >> > fdset[0].events = POLLPRI;
>> >> > fdset[0].revents = 0;
>> >> > poll(fdset, 1, -1);
>> >> > // at this point pin went high
>> >>
>> >> Try using lseek before reading the data after the poll.
>> >>
>> >> EX.
>> >>       if (fdset[0].revents & POLLPRI) {
>> >>               lseek(fdset[0].fd, 0, SEEK_SET);
>> >>               len = read(fdset[0].fd, buf, MAX_BUF);
>> >>               .
>> >>               .
>> >>       }
>> >>
>> >> See if this helps.
>> >
>> > Yes, that fixed it!
>>
>> Still, shouldn't we consider this as a regression, especially if not
>> using lseek worked for kernel 3.12 and before?
>
> Perhaps this is a side effect of the sysfs to kernfs change over to happened in
> 3.14.
>
> Looking at 'kernfs_fop_poll' my guess is that 'kernfs_get_active' is returning NULL
> immediately returning from the poll command and causing described behavior.
>
> http://lxr.free-electrons.com/source/fs/kernfs/file.c#L763

I think this looks plausible.

Folkert what do you say about this?

Yours,
Linus Walleij

Re: Fw: [3.18.3] poll() on gpio pins broken

[folkert]

> >> >> Try using lseek before reading the data after the poll.
> >> >>
> >> >> EX.
> >> >>       if (fdset[0].revents & POLLPRI) {
> >> >>               lseek(fdset[0].fd, 0, SEEK_SET);
> >> >>               len = read(fdset[0].fd, buf, MAX_BUF);
> >> >>               .
> >> >>               .
> >> >>       }
> >> >>
> >> >> See if this helps.
> >> >
> >> > Yes, that fixed it!
> >>
> >> Still, shouldn't we consider this as a regression, especially if not
> >> using lseek worked for kernel 3.12 and before?
> >
> > Perhaps this is a side effect of the sysfs to kernfs change over to happened in
> > 3.14.
> >
> > Looking at 'kernfs_fop_poll' my guess is that 'kernfs_get_active' is returning NULL
> > immediately returning from the poll command and causing described behavior.
> >
> > http://lxr.free-electrons.com/source/fs/kernfs/file.c#L763
> 
> I think this looks plausible.
> Folkert what do you say about this?

That looks indeed to be the current situation. It worked different
before though.

I also think that this interface is cumbersome. I did not measure it(!)
but I think adding this open/seek + read construction may add all kinds
of overhead. Especially since my use-case requires the lowest latency
possible. Not to forget that conversion of the measured value to ascii
and back.


Folkert van Heusden

-- 
Multitail es una herramienta flexible que permite visualizar los "log
file" y seguir la ejecución de comandos. Permite filtrar, añadir
colores, combinar archivos, la visualización de diferencias (diff-
view), etc.  http://www.vanheusden.com/multitail/
----------------------------------------------------------------------
Phone: +31-6-41278122, PGP-key: 1F28D8AE, www.vanheusden.com
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Fw: [3.18.3] poll() on gpio pins broken

[Linus Walleij]

On Thu, Feb 19, 2015 at 9:53 AM, folkert <folkert@vanheusden.com> wrote:

> I also think that this interface is cumbersome. I did not measure it(!)
> but I think adding this open/seek + read construction may add all kinds
> of overhead. Especially since my use-case requires the lowest latency
> possible. Not to forget that conversion of the measured value to ascii
> and back.

Yes this interface sucks.

The tentative long-term plan is to replace it with something like
a char device that can handle a quick context switch and also
issuing calls to set multiple pins at once, as we now have an
in-kernel interface for that (that I totally refuse to support from
sysfs).

Yours,
Linus Walleij

Re: Fw: [3.18.3] poll() on gpio pins broken

[Alexandre Courbot]

On Fri, Feb 20, 2015 at 1:52 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Feb 19, 2015 at 9:53 AM, folkert <folkert@vanheusden.com> wrote:
>
>> I also think that this interface is cumbersome. I did not measure it(!)
>> but I think adding this open/seek + read construction may add all kinds
>> of overhead. Especially since my use-case requires the lowest latency
>> possible. Not to forget that conversion of the measured value to ascii
>> and back.
>
> Yes this interface sucks.
>
> The tentative long-term plan is to replace it with something like
> a char device that can handle a quick context switch and also
> issuing calls to set multiple pins at once, as we now have an
> in-kernel interface for that (that I totally refuse to support from
> sysfs).

Mmm, I was thinking it would be nice to have a (new, redesigned) sysfs
interface for this. :P

Aren't we going to make things less accessible if we use a char device?

Re: Fw: [3.18.3] poll() on gpio pins broken

[Alexandre Courbot]

On Thu, Feb 19, 2015 at 5:53 PM, folkert <folkert@vanheusden.com> wrote:
>> >> >> Try using lseek before reading the data after the poll.
>> >> >>
>> >> >> EX.
>> >> >>       if (fdset[0].revents & POLLPRI) {
>> >> >>               lseek(fdset[0].fd, 0, SEEK_SET);
>> >> >>               len = read(fdset[0].fd, buf, MAX_BUF);
>> >> >>               .
>> >> >>               .
>> >> >>       }
>> >> >>
>> >> >> See if this helps.
>> >> >
>> >> > Yes, that fixed it!
>> >>
>> >> Still, shouldn't we consider this as a regression, especially if not
>> >> using lseek worked for kernel 3.12 and before?
>> >
>> > Perhaps this is a side effect of the sysfs to kernfs change over to happened in
>> > 3.14.
>> >
>> > Looking at 'kernfs_fop_poll' my guess is that 'kernfs_get_active' is returning NULL
>> > immediately returning from the poll command and causing described behavior.
>> >
>> > http://lxr.free-electrons.com/source/fs/kernfs/file.c#L763
>>
>> I think this looks plausible.
>> Folkert what do you say about this?
>
> That looks indeed to be the current situation. It worked different
> before though.
>
> I also think that this interface is cumbersome. I did not measure it(!)
> but I think adding this open/seek + read construction may add all kinds
> of overhead. Especially since my use-case requires the lowest latency
> possible. Not to forget that conversion of the measured value to ascii
> and back.

Folkert, since you have code that can reproduce the issue, could you
see whether you could come with a patch that restores the original
behavior? Maybe by introducing a in-kernel lseek() as soon as the file
is opened?

Re: Fw: [3.18.3] poll() on gpio pins broken

[Linus Walleij]

On Thu, Feb 26, 2015 at 11:27 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Fri, Feb 20, 2015 at 1:52 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Thu, Feb 19, 2015 at 9:53 AM, folkert <folkert@vanheusden.com> wrote:
>>
>>> I also think that this interface is cumbersome. I did not measure it(!)
>>> but I think adding this open/seek + read construction may add all kinds
>>> of overhead. Especially since my use-case requires the lowest latency
>>> possible. Not to forget that conversion of the measured value to ascii
>>> and back.
>>
>> Yes this interface sucks.
>>
>> The tentative long-term plan is to replace it with something like
>> a char device that can handle a quick context switch and also
>> issuing calls to set multiple pins at once, as we now have an
>> in-kernel interface for that (that I totally refuse to support from
>> sysfs).
>
> Mmm, I was thinking it would be nice to have a (new, redesigned) sysfs
> interface for this. :P
>
> Aren't we going to make things less accessible if we use a char device?

Since sysfs has a "one value per file" paradigm, it also has a
"one context switch per operation" paradigm, meaning any
efficiency-oriented use cases where a lot of stuff needs to be
changed in one context switch are by the very construction
not suitable for sysfs IMO. That is the use case for ioctl()
operations that can pass an entire struct of stuff over.

And things like bit-banging a clock+data line which would in
a sysfs case involve two context switches (one per value, since
that is one file per GPIO line) in an ioctl() case it would be
just one, already 50% less context switches for a very basic
use case.

But I may be just so wrong ... input welcome.

Yours,
Linus Walleij

Re: Fw: [3.18.3] poll() on gpio pins broken

[folkert]

> > Mmm, I was thinking it would be nice to have a (new, redesigned) sysfs
> > interface for this. :P
> >
> > Aren't we going to make things less accessible if we use a char device?
> 
> Since sysfs has a "one value per file" paradigm, it also has a
> "one context switch per operation" paradigm, meaning any
> efficiency-oriented use cases where a lot of stuff needs to be
> changed in one context switch are by the very construction
> not suitable for sysfs IMO. That is the use case for ioctl()
> operations that can pass an entire struct of stuff over.
> 
> And things like bit-banging a clock+data line which would in
> a sysfs case involve two context switches (one per value, since
> that is one file per GPIO line) in an ioctl() case it would be
> just one, already 50% less context switches for a very basic
> use case.
> 
> But I may be just so wrong ... input welcome.

Another bad thing of the current sysfs versions: it is ascii.
So I need to convert a=1 to a='1' first before write()ing it to the
file. Very inconvenient because it adds boilerplate code.
On the other hand, the sysfs is very much extensible.


Folkert van Heusden

-- 
You've probably gotten really fed up with never winning in the Mega-
Millions lottery. Well, weep no longer: www.smartwinning.info tells
you everything that might help you deciding what numbers to choose.
With nice graphs and pretty animations!

Re: Fw: [3.18.3] poll() on gpio pins broken

[Alexandre Courbot]

On Fri, Feb 27, 2015 at 10:15 PM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Thu, Feb 26, 2015 at 11:27 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
>> On Fri, Feb 20, 2015 at 1:52 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Thu, Feb 19, 2015 at 9:53 AM, folkert <folkert@vanheusden.com> wrote:
>>>
>>>> I also think that this interface is cumbersome. I did not measure it(!)
>>>> but I think adding this open/seek + read construction may add all kinds
>>>> of overhead. Especially since my use-case requires the lowest latency
>>>> possible. Not to forget that conversion of the measured value to ascii
>>>> and back.
>>>
>>> Yes this interface sucks.
>>>
>>> The tentative long-term plan is to replace it with something like
>>> a char device that can handle a quick context switch and also
>>> issuing calls to set multiple pins at once, as we now have an
>>> in-kernel interface for that (that I totally refuse to support from
>>> sysfs).
>>
>> Mmm, I was thinking it would be nice to have a (new, redesigned) sysfs
>> interface for this. :P
>>
>> Aren't we going to make things less accessible if we use a char device?
>
> Since sysfs has a "one value per file" paradigm, it also has a
> "one context switch per operation" paradigm, meaning any
> efficiency-oriented use cases where a lot of stuff needs to be
> changed in one context switch are by the very construction
> not suitable for sysfs IMO. That is the use case for ioctl()
> operations that can pass an entire struct of stuff over.
>
> And things like bit-banging a clock+data line which would in
> a sysfs case involve two context switches (one per value, since
> that is one file per GPIO line) in an ioctl() case it would be
> just one, already 50% less context switches for a very basic
> use case.

These are absolutely legitimate concerns, but couldn't the desired
result be achieved using an interface like this (warning: raw and
dirty draft):

    cd /sys/class/gpio
    echo "gpiochip0:12 gpiochip0:13 gpiochip1:7 lcd_gpios" >export   #
exports GPIOs 12 and 13 of chip0 and 7 of chip 1 under the node
"lcd_gpios"
    echo 5 >lcd_gpios/value # sets gpiochip0:12 and gpiochip1:7

... or we could request the user to export each GPIO individually,
then have a "group" node allowing to create a group of GPIOs from
already-exported ones. This idea might actually work better as it
would allow finer control over the properties of individual GPIO (e.g.
active-low status).

I understand that even this way sysfs might be slower than a character
device, but when you need that level of performance don't you just
need a dedicated driver? The ioctls might also get complex as it also
needs to be able to set all the GPIO properties.

Independently of this issue, I think it would be worth to create
alternative "export" nodes at the chip-level, like the pwm subsystem
has:

    pwm/pwmchip0/export
    pwm/pwmchip0/pwm1
    pwm/pwmchip0/pwm1/duty_cycle

This is much saner and would provide a sysfs interface that does not
rely on the global GPIO numberspace, which cannot be relied upon as
shown by countless evidence.

IMHO this is needed regardless of whether we decide to provide a
character device as well, so I thought we might also add the ability
to set multiple GPIOs in one call to it. Maintaining two user-space
ABIs (sysfs and char device) sounds a little bit overkill for
something as simple as GPIOs.

Re: Fw: [3.18.3] poll() on gpio pins broken

[Alexandre Courbot]

On Fri, Feb 27, 2015 at 10:19 PM, folkert <folkert@vanheusden.com> wrote:
>> > Mmm, I was thinking it would be nice to have a (new, redesigned) sysfs
>> > interface for this. :P
>> >
>> > Aren't we going to make things less accessible if we use a char device?
>>
>> Since sysfs has a "one value per file" paradigm, it also has a
>> "one context switch per operation" paradigm, meaning any
>> efficiency-oriented use cases where a lot of stuff needs to be
>> changed in one context switch are by the very construction
>> not suitable for sysfs IMO. That is the use case for ioctl()
>> operations that can pass an entire struct of stuff over.
>>
>> And things like bit-banging a clock+data line which would in
>> a sysfs case involve two context switches (one per value, since
>> that is one file per GPIO line) in an ioctl() case it would be
>> just one, already 50% less context switches for a very basic
>> use case.
>>
>> But I may be just so wrong ... input welcome.
>
> Another bad thing of the current sysfs versions: it is ascii.
> So I need to convert a=1 to a='1' first before write()ing it to the
> file. Very inconvenient because it adds boilerplate code.
> On the other hand, the sysfs is very much extensible.

write(fd, a == 0 ? '0' : '1', 1);

That doesn't look like too much boilerplate to me. And it can easily
be hidden under a dedicated macro/function.

Re: Fw: [3.18.3] poll() on gpio pins broken

[Michael Welling]

On Mon, Mar 02, 2015 at 03:16:06PM +0900, Alexandre Courbot wrote:
> On Fri, Feb 27, 2015 at 10:15 PM, Linus Walleij
> <linus.walleij@linaro.org> wrote:
> > On Thu, Feb 26, 2015 at 11:27 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> >> On Fri, Feb 20, 2015 at 1:52 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> >>> On Thu, Feb 19, 2015 at 9:53 AM, folkert <folkert@vanheusden.com> wrote:
> >>>
> >>>> I also think that this interface is cumbersome. I did not measure it(!)
> >>>> but I think adding this open/seek + read construction may add all kinds
> >>>> of overhead. Especially since my use-case requires the lowest latency
> >>>> possible. Not to forget that conversion of the measured value to ascii
> >>>> and back.

I may have opened this can of worms so I figured I should chime in.

The context switches probably of more concern than string conversion.

> >>>
> >>> Yes this interface sucks.
> >>>
> >>> The tentative long-term plan is to replace it with something like
> >>> a char device that can handle a quick context switch and also
> >>> issuing calls to set multiple pins at once, as we now have an
> >>> in-kernel interface for that (that I totally refuse to support from
> >>> sysfs).
> >>
> >> Mmm, I was thinking it would be nice to have a (new, redesigned) sysfs
> >> interface for this. :P
> >>
> >> Aren't we going to make things less accessible if we use a char device?
> >
> > Since sysfs has a "one value per file" paradigm, it also has a
> > "one context switch per operation" paradigm, meaning any
> > efficiency-oriented use cases where a lot of stuff needs to be
> > changed in one context switch are by the very construction
> > not suitable for sysfs IMO. That is the use case for ioctl()
> > operations that can pass an entire struct of stuff over.
> >
> > And things like bit-banging a clock+data line which would in
> > a sysfs case involve two context switches (one per value, since
> > that is one file per GPIO line) in an ioctl() case it would be
> > just one, already 50% less context switches for a very basic
> > use case.
> 
> These are absolutely legitimate concerns, but couldn't the desired
> result be achieved using an interface like this (warning: raw and
> dirty draft):
> 
>     cd /sys/class/gpio
>     echo "gpiochip0:12 gpiochip0:13 gpiochip1:7 lcd_gpios" >export   #
> exports GPIOs 12 and 13 of chip0 and 7 of chip 1 under the node
> "lcd_gpios"
>     echo 5 >lcd_gpios/value # sets gpiochip0:12 and gpiochip1:7
> 
> ... or we could request the user to export each GPIO individually,
> then have a "group" node allowing to create a group of GPIOs from
> already-exported ones. This idea might actually work better as it
> would allow finer control over the properties of individual GPIO (e.g.
> active-low status).
> 

It may be beneficial and easier to perform the grouping in the device tree.
Then the system could come up with the groups pre-exported in /sys/class/gpio.

The GPIOs would be self consumed like with the proposed pin hogging mechanism
but then available as a single group in the sysfs layer.

What do you guys think?

> I understand that even this way sysfs might be slower than a character
> device, but when you need that level of performance don't you just
> need a dedicated driver? The ioctls might also get complex as it also
> needs to be able to set all the GPIO properties.
> 
> Independently of this issue, I think it would be worth to create
> alternative "export" nodes at the chip-level, like the pwm subsystem
> has:
> 
>     pwm/pwmchip0/export
>     pwm/pwmchip0/pwm1
>     pwm/pwmchip0/pwm1/duty_cycle
> 
> This is much saner and would provide a sysfs interface that does not
> rely on the global GPIO numberspace, which cannot be relied upon as
> shown by countless evidence.
> 
> IMHO this is needed regardless of whether we decide to provide a
> character device as well, so I thought we might also add the ability
> to set multiple GPIOs in one call to it. Maintaining two user-space
> ABIs (sysfs and char device) sounds a little bit overkill for
> something as simple as GPIOs.

Perhaps an ioctl would be overkill seeing as the right way to emulate a bus 
or whatever timing critical would be in a kernel driver.

String conversion sucks but I could deal with it if more IOs were acessible
with a single system call.

As for the lseek thing I am not sure of the solution yet.

Re: Fw: [3.18.3] poll() on gpio pins broken

[Alexandre Courbot]

On Mon, Mar 2, 2015 at 4:27 PM, Michael Welling <mwelling@ieee.org> wrote:
> On Mon, Mar 02, 2015 at 03:16:06PM +0900, Alexandre Courbot wrote:
>> On Fri, Feb 27, 2015 at 10:15 PM, Linus Walleij
>> <linus.walleij@linaro.org> wrote:
>> > On Thu, Feb 26, 2015 at 11:27 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
>> >> On Fri, Feb 20, 2015 at 1:52 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> >>> On Thu, Feb 19, 2015 at 9:53 AM, folkert <folkert@vanheusden.com> wrote:
>> >>>
>> >>>> I also think that this interface is cumbersome. I did not measure it(!)
>> >>>> but I think adding this open/seek + read construction may add all kinds
>> >>>> of overhead. Especially since my use-case requires the lowest latency
>> >>>> possible. Not to forget that conversion of the measured value to ascii
>> >>>> and back.
>
> I may have opened this can of worms so I figured I should chime in.
>
> The context switches probably of more concern than string conversion.

I concur that point.

>
>> >>>
>> >>> Yes this interface sucks.
>> >>>
>> >>> The tentative long-term plan is to replace it with something like
>> >>> a char device that can handle a quick context switch and also
>> >>> issuing calls to set multiple pins at once, as we now have an
>> >>> in-kernel interface for that (that I totally refuse to support from
>> >>> sysfs).
>> >>
>> >> Mmm, I was thinking it would be nice to have a (new, redesigned) sysfs
>> >> interface for this. :P
>> >>
>> >> Aren't we going to make things less accessible if we use a char device?
>> >
>> > Since sysfs has a "one value per file" paradigm, it also has a
>> > "one context switch per operation" paradigm, meaning any
>> > efficiency-oriented use cases where a lot of stuff needs to be
>> > changed in one context switch are by the very construction
>> > not suitable for sysfs IMO. That is the use case for ioctl()
>> > operations that can pass an entire struct of stuff over.
>> >
>> > And things like bit-banging a clock+data line which would in
>> > a sysfs case involve two context switches (one per value, since
>> > that is one file per GPIO line) in an ioctl() case it would be
>> > just one, already 50% less context switches for a very basic
>> > use case.
>>
>> These are absolutely legitimate concerns, but couldn't the desired
>> result be achieved using an interface like this (warning: raw and
>> dirty draft):
>>
>>     cd /sys/class/gpio
>>     echo "gpiochip0:12 gpiochip0:13 gpiochip1:7 lcd_gpios" >export   #
>> exports GPIOs 12 and 13 of chip0 and 7 of chip 1 under the node
>> "lcd_gpios"
>>     echo 5 >lcd_gpios/value # sets gpiochip0:12 and gpiochip1:7
>>
>> ... or we could request the user to export each GPIO individually,
>> then have a "group" node allowing to create a group of GPIOs from
>> already-exported ones. This idea might actually work better as it
>> would allow finer control over the properties of individual GPIO (e.g.
>> active-low status).
>>
>
> It may be beneficial and easier to perform the grouping in the device tree.
> Then the system could come up with the groups pre-exported in /sys/class/gpio.
>
> The GPIOs would be self consumed like with the proposed pin hogging mechanism
> but then available as a single group in the sysfs layer.
>
> What do you guys think?

That's tempting as a convenience, but since there are many obscure
use-cases involving GPIOs I'd like to have a more generic and flexible
way before anything else. Not all GPIO users are backed by
device-tree.

>
>> I understand that even this way sysfs might be slower than a character
>> device, but when you need that level of performance don't you just
>> need a dedicated driver? The ioctls might also get complex as it also
>> needs to be able to set all the GPIO properties.
>>
>> Independently of this issue, I think it would be worth to create
>> alternative "export" nodes at the chip-level, like the pwm subsystem
>> has:
>>
>>     pwm/pwmchip0/export
>>     pwm/pwmchip0/pwm1
>>     pwm/pwmchip0/pwm1/duty_cycle
>>
>> This is much saner and would provide a sysfs interface that does not
>> rely on the global GPIO numberspace, which cannot be relied upon as
>> shown by countless evidence.
>>
>> IMHO this is needed regardless of whether we decide to provide a
>> character device as well, so I thought we might also add the ability
>> to set multiple GPIOs in one call to it. Maintaining two user-space
>> ABIs (sysfs and char device) sounds a little bit overkill for
>> something as simple as GPIOs.
>
> Perhaps an ioctl would be overkill seeing as the right way to emulate a bus
> or whatever timing critical would be in a kernel driver.
>
> String conversion sucks but I could deal with it if more IOs were acessible
> with a single system call.

It really comes down to how user-space wants to access GPIOs. I
suspect the majority of sysfs accesses is done by scripts and other
simple programs. If we introduce a char device that takes requires
ioctls, it is then customary to add a small user-space library to
abstract that (for both convenience and safety - think libdrm). Do we
want to maintain libgpio? If not, aren't we going to frustrate users
who will wonder why on earth they need to use an ioctl just to change
the value of a GPIO?

sysfs requires to manipulate and sometimes build strings indeed, but
as Michael pointed out this is a neglectible overhead and it is much
more accessible. open, write, close, you're done.

Re: Fw: [3.18.3] poll() on gpio pins broken

[Linus Walleij]

On Tue, Mar 3, 2015 at 9:27 AM, Alexandre Courbot <gnurou@gmail.com> wrote:

> It really comes down to how user-space wants to access GPIOs. I
> suspect the majority of sysfs accesses is done by scripts and other
> simple programs. If we introduce a char device that takes requires
> ioctls, it is then customary to add a small user-space library to
> abstract that (for both convenience and safety - think libdrm). Do we
> want to maintain libgpio?

Good point.

We have no clue about how the majority out there use the GPIO
sysfs, but I have heard of mission-critical systems just hammering
GPIOs from userspace.

Sadly many of these industrial users are "I just want it to work, now"
types and they don't step forward much on these mailing lists.
(Learned from private conversations...)

Maybe if noone voice their opinion and offer to help with this, we can
assume they don't exist (well obviously a community does not exist)
and their specific needs be ignored until they put their money where
their mouth is.

Yours,
Linus Walleij

Re: Fw: [3.18.3] poll() on gpio pins broken

[Alexandre Courbot]

On Tue, Mar 3, 2015 at 7:31 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Mar 3, 2015 at 9:27 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
>
>> It really comes down to how user-space wants to access GPIOs. I
>> suspect the majority of sysfs accesses is done by scripts and other
>> simple programs. If we introduce a char device that takes requires
>> ioctls, it is then customary to add a small user-space library to
>> abstract that (for both convenience and safety - think libdrm). Do we
>> want to maintain libgpio?
>
> Good point.
>
> We have no clue about how the majority out there use the GPIO
> sysfs, but I have heard of mission-critical systems just hammering
> GPIOs from userspace.
>
> Sadly many of these industrial users are "I just want it to work, now"
> types and they don't step forward much on these mailing lists.
> (Learned from private conversations...)
>
> Maybe if noone voice their opinion and offer to help with this, we can
> assume they don't exist (well obviously a community does not exist)
> and their specific needs be ignored until they put their money where
> their mouth is.

That's the only way we can handle the situation if people don't
manifest their needs. But does this mean that you would agree with a
cleaner, multi-GPIO friendly sysfs-based solution, or I am
misunderstanding you?

Re: Fw: [3.18.3] poll() on gpio pins broken

[Linus Walleij]

On Wed, Mar 4, 2015 at 1:43 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Tue, Mar 3, 2015 at 7:31 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Tue, Mar 3, 2015 at 9:27 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
>>
>>> It really comes down to how user-space wants to access GPIOs. I
>>> suspect the majority of sysfs accesses is done by scripts and other
>>> simple programs. If we introduce a char device that takes requires
>>> ioctls, it is then customary to add a small user-space library to
>>> abstract that (for both convenience and safety - think libdrm). Do we
>>> want to maintain libgpio?
>>
>> Good point.
>>
>> We have no clue about how the majority out there use the GPIO
>> sysfs, but I have heard of mission-critical systems just hammering
>> GPIOs from userspace.
>>
>> Sadly many of these industrial users are "I just want it to work, now"
>> types and they don't step forward much on these mailing lists.
>> (Learned from private conversations...)
>>
>> Maybe if noone voice their opinion and offer to help with this, we can
>> assume they don't exist (well obviously a community does not exist)
>> and their specific needs be ignored until they put their money where
>> their mouth is.
>
> That's the only way we can handle the situation if people don't
> manifest their needs. But does this mean that you would agree with a
> cleaner, multi-GPIO friendly sysfs-based solution, or I am
> misunderstanding you?

I guess I'm just a bit grumpy.

Whoever comes up with a cleaner sysfs or a clean device interface
will win the argument and lock the path for the other approach.
It's like a forking path with no going back or something.

Yours,
Linus Walleij

Re: Fw: [3.18.3] poll() on gpio pins broken

[folkert]

> Whoever comes up with a cleaner sysfs or a clean device interface
> will win the argument and lock the path for the other approach.
> It's like a forking path with no going back or something.

Can't we just do it a bit like v4l2 does it?
E.g. an open /dev/gpio and then an ioctl which queries all chips with
io-pins, then after you select such a chip (e.g. the cubieboard has
chip a-f iirc), an ioctl which lists the number of pins and then one
that retrieves the number of parameters and then for each parameter
another ioctl that 
well:

- open /dev/gpio
- ioctl to obtain the number of gpio chips
- ioctl to iterate the chips
  - this could return a symbol name, e.g. "A"-"F" or e.g. "north",
    whatever
  - description
  - if it supports hardware interrupts(!)
  - get number of parameters for the chip
  - iterate parameters of the chip
    - get
    - set
  - get maximum number of pins that can be high (?)
- ioctl to select a chip
- ioctl to retrieve number of pins per chip
- ioctl to iterate the pins of a chip
- ioctl to select a pin
- ioctl to get the number of knobs on a pin
- ioctl to iterate the parameters
  they could be
	- set direction
	- set value
	- set trigger
	- has hardware interrupt
	- enable pps (!), preferably with a name override because
	  /dev/ppsX is not helpful

Now after you've selected a pin and selected a trigger you could do a
select()/poll() that waits until the trigger is triggered with
auto-reset of that trigger (or maybe make that selectable as well).
POLLIN then says; we had a trigger, POLLERR the chip was removed (or
POLLHUP?).

Oh and please as low latency for the select()/poll() as possible :-)


Folkert van Heusden

-- 
You've probably gotten really fed up with never winning in the Mega-
Millions lottery. Well, weep no longer: www.smartwinning.info tells
you everything that might help you deciding what numbers to choose.
With nice graphs and pretty animations!

Re: Fw: [3.18.3] poll() on gpio pins broken

[Michael Welling]

On Mon, Mar 09, 2015 at 04:52:26PM +0100, Linus Walleij wrote:
> On Wed, Mar 4, 2015 at 1:43 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> > On Tue, Mar 3, 2015 at 7:31 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> >> On Tue, Mar 3, 2015 at 9:27 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> >>
> >>> It really comes down to how user-space wants to access GPIOs. I
> >>> suspect the majority of sysfs accesses is done by scripts and other
> >>> simple programs. If we introduce a char device that takes requires
> >>> ioctls, it is then customary to add a small user-space library to
> >>> abstract that (for both convenience and safety - think libdrm). Do we
> >>> want to maintain libgpio?
> >>
> >> Good point.
> >>
> >> We have no clue about how the majority out there use the GPIO
> >> sysfs, but I have heard of mission-critical systems just hammering
> >> GPIOs from userspace.
> >>
> >> Sadly many of these industrial users are "I just want it to work, now"
> >> types and they don't step forward much on these mailing lists.
> >> (Learned from private conversations...)
> >>
> >> Maybe if noone voice their opinion and offer to help with this, we can
> >> assume they don't exist (well obviously a community does not exist)
> >> and their specific needs be ignored until they put their money where
> >> their mouth is.
> >
> > That's the only way we can handle the situation if people don't
> > manifest their needs. But does this mean that you would agree with a
> > cleaner, multi-GPIO friendly sysfs-based solution, or I am
> > misunderstanding you?
> 
> I guess I'm just a bit grumpy.
> 
> Whoever comes up with a cleaner sysfs or a clean device interface
> will win the argument and lock the path for the other approach.
> It's like a forking path with no going back or something.

There is no need to fork and in fact it would probably be a bad idea.

At EMAC we support both sysfs and character device simultaneously.
Sysfs for the ease of use and ioctl for real time advantages.

Not saying that it is a good reference but the two interfaces "could" co-exist.

> 
> Yours,
> Linus Walleij

Re: Fw: [3.18.3] poll() on gpio pins broken

[Linus Walleij]

On Mon, Mar 9, 2015 at 9:22 PM, Michael Welling <mwelling@ieee.org> wrote:
> On Mon, Mar 09, 2015 at 04:52:26PM +0100, Linus Walleij wrote:

>> Whoever comes up with a cleaner sysfs or a clean device interface
>> will win the argument and lock the path for the other approach.
>> It's like a forking path with no going back or something.
>
> There is no need to fork and in fact it would probably be a bad idea.

For the record I am *NOT* talking about this:
https://en.wikipedia.org/wiki/Fork_%28software_development%29

> At EMAC we support both sysfs and character device simultaneously.
> Sysfs for the ease of use and ioctl for real time advantages.

What is EMAC?

> Not saying that it is a good reference but the two interfaces "could" co-exist.

Hm....

I would more think about deprecating the sysfs in favor of the dev
node.

But this is getting terribly academic since we're just talking, noone is
really implementing anything.

Yours,
Linus Walleij

Re: Fw: [3.18.3] poll() on gpio pins broken

[Michael Welling]

On Tue, Mar 17, 2015 at 05:39:01PM +0100, Linus Walleij wrote:
> On Mon, Mar 9, 2015 at 9:22 PM, Michael Welling <mwelling@ieee.org> wrote:
> > On Mon, Mar 09, 2015 at 04:52:26PM +0100, Linus Walleij wrote:
> 
> >> Whoever comes up with a cleaner sysfs or a clean device interface
> >> will win the argument and lock the path for the other approach.
> >> It's like a forking path with no going back or something.
> >
> > There is no need to fork and in fact it would probably be a bad idea.
> 
> For the record I am *NOT* talking about this:
> https://en.wikipedia.org/wiki/Fork_%28software_development%29
>

Okay.
 
> > At EMAC we support both sysfs and character device simultaneously.
> > Sysfs for the ease of use and ioctl for real time advantages.
> 
> What is EMAC?

It is the company that I am currently working for.
www.emacinc.com

> 
> > Not saying that it is a good reference but the two interfaces "could" co-exist.
> 
> Hm....
> 
> I would more think about deprecating the sysfs in favor of the dev
> node.

What happens to all of the users of the sysfs interface when this happens?

> 
> But this is getting terribly academic since we're just talking, noone is
> really implementing anything.

Without a specification nothing is ever going to be implemented.

If not here, where will we be able to discuss the implementation details?

> 
> Yours,
> Linus Walleij

Re: Fw: [3.18.3] poll() on gpio pins broken

[Linus Walleij]

On Tue, Mar 17, 2015 at 5:47 PM, Michael Welling <mwelling@ieee.org> wrote:
> On Tue, Mar 17, 2015 at 05:39:01PM +0100, Linus Walleij wrote:

>> I would more think about deprecating the sysfs in favor of the dev
>> node.
>
> What happens to all of the users of the sysfs interface when this happens?

They keep using a deprecated interface instead of an encouraged
interface.

>> But this is getting terribly academic since we're just talking, noone is
>> really implementing anything.
>
> Without a specification nothing is ever going to be implemented.

This is not a case of either/or but a case of both/and.

Yours,
Linus Walleij