Discussion on GPIO monitoring and design thoughts.

[Andrew Jeffery]

On Fri, 2017-04-07 at 12:11 +0530, vishwa wrote:
> > On Wed, 2017-04-05 at 13:56 +0530, vishwa wrote:
> > > We had a technical discussion on HOWTO and what is the right thing to do 
> > > handling the GPIO assertion and here is the gist.
> > > 
> > > 1) Use gpio-keys and utilize libevdev wrappers ( 
> > > https://www.freedesktop.org/software/libevdev/doc/latest/ ) to watch for 
> > > data in /dev/input/event.
> > 
> > Okay, I'd like you to step back and detail what problem you're actually
> > trying to solve here. Going from wanting to handle GPIO interrupts to
> > using gpio-keys everywhere needs some explanation. We have at least two
> > other interfaces that would appear to be sufficient: GPIO chardev and
> > sysfs.
>  
> Sure. I realize I should have put the use-case here than at the end.
> 
> Problem: When the host check-stops, we need a way to detect that. 
> The existing mechanism will assert a configured GPIO when that happens.
> Firmware code needs a way of knowing that it has happened and that is why this application.
> 
> I was told that /sysfs is being deprecated and that we use the chardev. We had 2 options then.
> 
> 1/. Use the gpio-keys 
> 2/. Use libgpio ( the support is not in yet in 4.7 per the discussion )
> 
> Brad Bishop recommended that gpio-keys be used over libgpio due to the debouncing support built into gpio-keys implementation.
> Since I am not an expert in this area, I suggest you please talk to Brad if you see there are better ideas.

I've since had a chat with Brad and Vishwa. There's currently no
perfect fit solution to the problem, so given:

1. We're only dealing with input GPIOs
2. gpio-keys enables the description of debounce periods (through
devicetree)
3. At least *some* GPIOs need debouncing

I'm okay if we proceed this way.

Considering the current state of the kernel and alternatives to the
above, if 1. weren't the case then we would have a weird split between
/dev/input and either the GPIO chardev interface or sysfs, so given 1.
we've at least got consistency. Similarly for 2. and 3. we will
probably run into issues if we split input GPIOs between gpio-keys and
the alternative interfaces, as we may in the future find we need to
enable debouncing on a GPIO which would trigger user-space breaking
migration from one approach to the other.

We do have the downside of assigning keycodes to GPIOs. This could be
an arbitrary (e.g. linear, as-needed) assignment, or we could do
something like set the keycode to the GPIO line's index (e.g. GPIOA0 =
0, GPIOZ7 = 215, etc). This would at least give some meaning to the
values and so we hopefully have no reason to change them in the future.
I don't think there are any limitations to the keycode value we can use
(e.g. max 255), so even if the Aspeed chips expand the number of
available GPIOs in the future we should be okay.

As to why we can't configure debounce periods via say the chardev
interface, this is something I will ask upstream about.

> > 
> > > *) The services that are part of the target will then own complete 
> > > handling of the condition instead of the application that monitored the 
> > > GPIO.
> > 
> > Why are we decoupling the handler services from the GPIO? What is the
> > advantage?
> > 
> > The disadvantage is the behaviour of the system is much harder to
> > understand. If we go this route, what tooling will be in place to debug
> > failures of interactions between components?
> > 
> > I would argue that this is already a significant hurdle in
> > understanding integration failures on the BMC, and I would prefer not
> > to add to the problems.
> > 
>  
> I am not following the argument here. Why is the behavior harder to understand when you have defined systemd targets doing what they are
> required to do. Its the job of applications that get into that target to do the right thing of collecting what is necessary to understand
> one specific problem.

I apologise, I had a misunderstand stemming from your description in
2.1. Sorry for that - you weren't planning on doing what I thought.

> > > ---------
> > > 
> > > Here is an example of application that handles the GPIO assertion that 
> > > happens when host does a checkstop.
> > > 
> > > 1) Application's arguments need /dev/input/event<$number> and the 
> > > expected GPIO state as indicated by linux,code in :
> > > 
> > > http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/input/gpio-keys.txt
> > 
> > There is a downside in that you will be creating a kernel ABI by
> > mapping GPIO events to a keycode. This choice appears to me to be
> > arbitrary, and from the kernel's perspective we don't know how
> > userspace is going to treat the value so we lose the flexibility to
> > change it if it's not. This isn't a show-stopper, but it sure feels
> > like a wart that doesn't need to exist.
> > 
>  
> Is it not expected that the binding always pass the input_event struct ?. What kind of changes are you thinking ?
> 
> Are you saying for instance you will change these values and applications may go crazy ?
> 
> > #define KEY_UP			103
> #define KEY_DOWN		108

As you pointed out we need to specify these values in your gpio-keys
subnode via the linux,code property. We mustn't assume anything about
how userspace behaves, e.g. it may very well care about these values
and could react negatively if the value were changed.

You mention above /dev/input/event<$number> - were you planning on
having one or multiple gpio-keys entries in the devicetree? If
multiple, what was your planned strategy for grouping the keys into
gpio-keys nodes?

One approach would be to have one gpio-keys node for each GPIO line.
This would enable userspace not to care about what value was assigned
to linux,code at the cost of knowing *which* /dev/input/event* file to
open. I'm not suggesting this is a good idea (I don't think it is), but
it is possible. It also doesn't change the fact that userspace may
still care about the value regardless. Further, knowing which device to
open may still be a problem anyway if someone plugs in say a USB
keyboard. We probably need more investigation here.

Cheers,

Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20170410/348aefe5/attachment.sig>