| Message ID | 20171020033727.21557-4-andrew@aj.id.au |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | gpio: Expose reset tolerance capability | expand |
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
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
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
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
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
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
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
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(-)