[RFC,3/5] gpio: gpiolib: Add chardev support for maintaining GPIO values on reset

Message ID 20171020033727.21557-4-andrew@aj.id.au
State New
Headers show
Series
  • gpio: Expose reset tolerance capability
Related show

Commit Message

Andrew Jeffery Oct. 20, 2017, 3:37 a.m.
Similar to devicetree support, add flags and mappings to expose reset
tolerance configuration through the chardev interface.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/gpio/gpiolib.c    | 14 +++++++++++++-
 include/uapi/linux/gpio.h | 11 ++++++-----
 2 files changed, 19 insertions(+), 6 deletions(-)

Comments

Linus Walleij Oct. 20, 2017, 7:27 a.m. | #1
I paged Bartosz and Michael on this, they are experts on the use cases for
the character device and their opinions are likely more valuable than mine.

On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> Similar to devicetree support, add flags and mappings to expose reset
> tolerance configuration through the chardev interface.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
(...)

> +                * Unconditionally configure reset tolerance, as it's possible
> +                * that the tolerance flag itself becomes tolerant to resets.
> +                * Thus it could remain set from a previous environment, but
> +                * the current environment may not expect it so.
> +                */
> +               ret = gpiod_set_reset_tolerant(desc,
> +                               !!(lflags & GPIOHANDLE_REQUEST_RESET_TOLERANT));
> +               if (ret < 0)
> +                       goto out_free_descs;

First, as noted in the first patch, IMO we should just go for persistance,
i.e. you want to flag to the system to keep the line persistent in any case,
no matter if the system goes to sleep or resets.

So the usecase is going to be a control system or similar, a makerspace
project, an industrial product of some kind, driving GPIO from userspace.

I don't see it as helpful to give userspace control over whether the line
is persistent or not. It is more reasonable to assume persistance for
userspace use cases, don't you think? Whether the system goes to sleep
or the gpiochip resets should not make a door suddenly close or the
lights in the christmas tree go out, right? I think if the gpiochip supports
persistance of any kind, we should try to use it and not have userspace
provide flags for that.

Yours,
Linus Walleij
Andrew Jeffery Oct. 20, 2017, 9:02 a.m. | #2
On Fri, 2017-10-20 at 09:27 +0200, Linus Walleij wrote:
> I paged Bartosz and Michael on this, they are experts on the use cases for
> the character device and their opinions are likely more valuable than mine.
> 
> > On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> > Similar to devicetree support, add flags and mappings to expose reset
> > tolerance configuration through the chardev interface.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> 
> (...)
> 
> > +                * Unconditionally configure reset tolerance, as it's possible
> > +                * that the tolerance flag itself becomes tolerant to resets.
> > +                * Thus it could remain set from a previous environment, but
> > +                * the current environment may not expect it so.
> > +                */
> > +               ret = gpiod_set_reset_tolerant(desc,
> > +                               !!(lflags & GPIOHANDLE_REQUEST_RESET_TOLERANT));
> > +               if (ret < 0)
> > +                       goto out_free_descs;
> 
> First, as noted in the first patch, IMO we should just go for persistance,
> i.e. you want to flag to the system to keep the line persistent in any case,
> no matter if the system goes to sleep or resets.
> 
> So the usecase is going to be a control system or similar, a makerspace
> project, an industrial product of some kind, driving GPIO from userspace.
> 
> I don't see it as helpful to give userspace control over whether the line
> is persistent or not. It is more reasonable to assume persistance for
> userspace use cases, don't you think? Whether the system goes to sleep
> or the gpiochip resets should not make a door suddenly close or the
> lights in the christmas tree go out, right? I think if the gpiochip supports
> persistance of any kind, we should try to use it and not have userspace
> provide flags for that.

Right. I guess the counter argument to your examples is if the gpio is
controlling any active process that we don't want to continue if we've
lost the capacity to monitor some other inputs (some kind of dead-man's 
switch). But maybe the argument is that should be implemented in the
kernel anyway?

Andrew
Charles Keepax Oct. 25, 2017, 8:14 a.m. | #3
On Fri, Oct 20, 2017 at 07:32:53PM +1030, Andrew Jeffery wrote:
> On Fri, 2017-10-20 at 09:27 +0200, Linus Walleij wrote:
> > I don't see it as helpful to give userspace control over whether the line
> > is persistent or not. It is more reasonable to assume persistance for
> > userspace use cases, don't you think? Whether the system goes to sleep
> > or the gpiochip resets should not make a door suddenly close or the
> > lights in the christmas tree go out, right? I think if the gpiochip supports
> > persistance of any kind, we should try to use it and not have userspace
> > provide flags for that.
> 
> Right. I guess the counter argument to your examples is if the gpio is
> controlling any active process that we don't want to continue if we've
> lost the capacity to monitor some other inputs (some kind of dead-man's 
> switch). But maybe the argument is that should be implemented in the
> kernel anyway?
> 

To me it certainly feels like decisions like this should live in
the kernel, your talking about things that could cause very weird
hardware behaviour if set wrong, so it makes sense to me to have
that responsibility guarded in the kernel.

Thanks,
Charles
Andrew Jeffery Oct. 26, 2017, 12:05 a.m. | #4
On Wed, 2017-10-25 at 09:14 +0100, Charles Keepax wrote:
> On Fri, Oct 20, 2017 at 07:32:53PM +1030, Andrew Jeffery wrote:
> > On Fri, 2017-10-20 at 09:27 +0200, Linus Walleij wrote:
> > > I don't see it as helpful to give userspace control over whether the line
> > > is persistent or not. It is more reasonable to assume persistance for
> > > userspace use cases, don't you think? Whether the system goes to sleep
> > > or the gpiochip resets should not make a door suddenly close or the
> > > lights in the christmas tree go out, right? I think if the gpiochip supports
> > > persistance of any kind, we should try to use it and not have userspace
> > > provide flags for that.
> > 
> > Right. I guess the counter argument to your examples is if the gpio is
> > controlling any active process that we don't want to continue if we've
> > lost the capacity to monitor some other inputs (some kind of dead-man's 
> > switch). But maybe the argument is that should be implemented in the
> > kernel anyway?
> > 
> 
> To me it certainly feels like decisions like this should live in
> the kernel, your talking about things that could cause very weird
> hardware behaviour if set wrong, so it makes sense to me to have
> that responsibility guarded in the kernel.

I feel that taking this argument to its logical conclusion leads to
never exporting any GPIOs to userspace and doing everything in the
kernel. If userspace has exported the GPIO and is managing its state,
then it can *already* cause very weird hardware behaviour if set wrong.
The fact that userspace is controlling the GPIO state and not the
kernel already says that the kernel doesn't know how to manage it, so
why not expose the option for userspace to set the persistence, given
that it should know what it's doing?

Cheers,

Andrew
Charles Keepax Oct. 26, 2017, 9:10 a.m. | #5
On Thu, Oct 26, 2017 at 10:35:39AM +1030, Andrew Jeffery wrote:
> On Wed, 2017-10-25 at 09:14 +0100, Charles Keepax wrote:
> > On Fri, Oct 20, 2017 at 07:32:53PM +1030, Andrew Jeffery wrote:
> > > On Fri, 2017-10-20 at 09:27 +0200, Linus Walleij wrote:
> > > > I don't see it as helpful to give userspace control over whether the line
> > > > is persistent or not. It is more reasonable to assume persistance for
> > > > userspace use cases, don't you think? Whether the system goes to sleep
> > > > or the gpiochip resets should not make a door suddenly close or the
> > > > lights in the christmas tree go out, right? I think if the gpiochip supports
> > > > persistance of any kind, we should try to use it and not have userspace
> > > > provide flags for that.
> > > 
> > > Right. I guess the counter argument to your examples is if the gpio is
> > > controlling any active process that we don't want to continue if we've
> > > lost the capacity to monitor some other inputs (some kind of dead-man's 
> > > switch). But maybe the argument is that should be implemented in the
> > > kernel anyway?
> > > 
> > 
> > To me it certainly feels like decisions like this should live in
> > the kernel, your talking about things that could cause very weird
> > hardware behaviour if set wrong, so it makes sense to me to have
> > that responsibility guarded in the kernel.
> 
> I feel that taking this argument to its logical conclusion leads to
> never exporting any GPIOs to userspace and doing everything in the
> kernel. If userspace has exported the GPIO and is managing its state,
> then it can *already* cause very weird hardware behaviour if set wrong.
> The fact that userspace is controlling the GPIO state and not the
> kernel already says that the kernel doesn't know how to manage it, so
> why not expose the option for userspace to set the persistence, given
> that it should know what it's doing?

Admittedly yes, I guess it really comes down to use-cases.  There
are fairly strong use-cases to control GPIOs from user-space
that justify the risks. The use-cases for being able to set
non-persistent GPIOs from user-space seem less clear to me, but
if they exist I certainly don't have any objection.

Thanks,
Charles
Linus Walleij Oct. 31, 2017, 9:59 a.m. | #6
On Thu, Oct 26, 2017 at 2:05 AM, Andrew Jeffery <andrew@aj.id.au> wrote:

> I feel that taking this argument to its logical conclusion leads to
> never exporting any GPIOs to userspace and doing everything in the
> kernel.

That is very much how I feel about things anyways.
In a recent presentation:
https://dflund.se/~triad/papers/GPIO-for-Engineers-and-Makers.pdf

I had the following text:

The Rules of Linux Userspace GPIO
1. You do not access GPIOs from userspace
2. YOU DO NOT ACCESS GPIOS FROM USERSPACE
3. Read Documentation/gpio/drivers-on-gpio.txt
4. Use the character device

> If userspace has exported the GPIO and is managing its state,
> then it can *already* cause very weird hardware behaviour if set wrong.
> The fact that userspace is controlling the GPIO state and not the
> kernel already says that the kernel doesn't know how to manage it, so
> why not expose the option for userspace to set the persistence, given
> that it should know what it's doing?

People do need to access GPIOs from userspace for things
like one-off makerspace projects, relays on factory lines,
fire alarms, door openers etc etc.

One-off projects is fine, the user likely has an idea about the
whole system that is comprehensive. They use the random
raspberry Pi (etc) development board for this. OK.

When we are talking about adding GPIO in mass-market goods
such as phones and tablets and laptops userspace GPIO
access become more and more dubious.

Yours,
Linus Walleij

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6b4c5df10e84..442ee5ceee08 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -357,7 +357,8 @@  struct linehandle_state {
 	GPIOHANDLE_REQUEST_OUTPUT | \
 	GPIOHANDLE_REQUEST_ACTIVE_LOW | \
 	GPIOHANDLE_REQUEST_OPEN_DRAIN | \
-	GPIOHANDLE_REQUEST_OPEN_SOURCE)
+	GPIOHANDLE_REQUEST_OPEN_SOURCE | \
+	GPIOHANDLE_REQUEST_RESET_TOLERANT)
 
 static long linehandle_ioctl(struct file *filep, unsigned int cmd,
 			     unsigned long arg)
@@ -498,6 +499,17 @@  static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 			set_bit(FLAG_OPEN_SOURCE, &desc->flags);
 
 		/*
+		 * Unconditionally configure reset tolerance, as it's possible
+		 * that the tolerance flag itself becomes tolerant to resets.
+		 * Thus it could remain set from a previous environment, but
+		 * the current environment may not expect it so.
+		 */
+		ret = gpiod_set_reset_tolerant(desc,
+				!!(lflags & GPIOHANDLE_REQUEST_RESET_TOLERANT));
+		if (ret < 0)
+			goto out_free_descs;
+
+		/*
 		 * Lines have to be requested explicitly for input
 		 * or output, else the line will be treated "as is".
 		 */
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 333d3544c964..1b1ce1af8653 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -56,11 +56,12 @@  struct gpioline_info {
 #define GPIOHANDLES_MAX 64
 
 /* Linerequest flags */
-#define GPIOHANDLE_REQUEST_INPUT	(1UL << 0)
-#define GPIOHANDLE_REQUEST_OUTPUT	(1UL << 1)
-#define GPIOHANDLE_REQUEST_ACTIVE_LOW	(1UL << 2)
-#define GPIOHANDLE_REQUEST_OPEN_DRAIN	(1UL << 3)
-#define GPIOHANDLE_REQUEST_OPEN_SOURCE	(1UL << 4)
+#define GPIOHANDLE_REQUEST_INPUT		(1UL << 0)
+#define GPIOHANDLE_REQUEST_OUTPUT		(1UL << 1)
+#define GPIOHANDLE_REQUEST_ACTIVE_LOW		(1UL << 2)
+#define GPIOHANDLE_REQUEST_OPEN_DRAIN		(1UL << 3)
+#define GPIOHANDLE_REQUEST_OPEN_SOURCE		(1UL << 4)
+#define GPIOHANDLE_REQUEST_RESET_TOLERANT	(1UL << 5)
 
 /**
  * struct gpiohandle_request - Information about a GPIO handle request