linux-kernel - Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200429014917.GA11974@sol>
Date:   Wed, 29 Apr 2020 09:49:17 +0800
From:   Kent Gibson <warthog618@...il.com>
To:     Hector Bujanda <hector.bujanda@...i.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Drew Fustini <drew@...7.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

On Tue, Apr 28, 2020 at 12:35:05PM +0200, Linus Walleij wrote:
> Hi Hector,
> 
> thanks for your patch!
> 
> On Sun, Apr 19, 2020 at 2:22 AM Hector Bujanda <hector.bujanda@...i.com> wrote:
> 
> > This allows calling gpiod_set_debounce function through char device ioctl.
> >
> > Signed-off-by: Hector Bujanda <hector.bujanda@...i.com>
> 
> (...)
> > +/**
> > + * struct gpioline_debounce - GPIO line debounce
> > + * @line_offset: the local offset on this GPIO device, fill this in when
> > + * requesting the line information from the kernel
> > + * @debounce_usec: debounce in uSeconds to set for this line
> > + */
> > +struct gpioline_debounce {
> > +       __u32 line_offset;
> > +       __u32 debounce_usec;
> > +};
> (...)
> > @@ -154,5 +165,6 @@ struct gpioevent_data {
> >  #define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info)
> >  #define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
> >  #define GPIO_GET_LINEEVENT_IOCTL _IOWR(0xB4, 0x04, struct gpioevent_request)
> > +#define GPIO_SET_DEBOUNCE_IOCTL _IOW(0xB4, 0x05, struct gpioline_debounce)
> 
> Please do not define a new ioctl for this: since of commit
> e588bb1eae31be73fbec2b731be986a7c09635a4
> "gpio: add new SET_CONFIG ioctl() to gpio chardev"
> by Kent Gibson we have this:
> 
> /**
>  * struct gpiohandle_config - Configuration for a GPIO handle request
>  * @flags: updated flags for the requested GPIO lines, such as
>  * GPIOHANDLE_REQUEST_OUTPUT, GPIOHANDLE_REQUEST_ACTIVE_LOW etc, OR:ed
>  * together
>  * @default_values: if the GPIOHANDLE_REQUEST_OUTPUT is set in flags,
>  * this specifies the default output value, should be 0 (low) or
>  * 1 (high), anything else than 0 or 1 will be interpreted as 1 (high)
>  * @padding: reserved for future use and should be zero filled
>  */
> struct gpiohandle_config {
>         __u32 flags;
>         __u8 default_values[GPIOHANDLES_MAX];
>         __u32 padding[4]; /* padding for future use */
> };
> 
> #define GPIOHANDLE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0a, struct gpiohandle_config)
> 
> Setting debounce is just another type of config, so please use Kent's
> work as a base for this.
> 

A few things you might want to keep in mind:

For ABI backward compatibility, a zeroed field should be treated as "don't change".
As a 0 is also used to disable debounce you are going to need a flag
indicating whether the debounce field is present.  I don't think that can
be added to the flags field, as those values are shared with the
gpiohandle_request, but I could well be wrong on that (maybe it can just
be ignored in the request case??).

You might want to add a flag to the GPIOLINE_FLAGs to indicate if
debounce is set.

The GPIOHANDLE_SET_CONFIG_IOCTL/gpiohandle_config only applies to 
gpiohandle_requests, so it does not provide a means to set debounce for 
gpioevent_requests, and you really want debounce for those, right?
I would extend that ioctl to the gpioevent_requests, but I would run it
past Bart or LinusW first - they may have better ideas.

Cheers,
Kent.