[PATCH] gpio: Timestamp events in hardirq handler

[PATCH] gpio: Timestamp events in hardirq handler

[Linus Walleij]

Add a hardirq handler to the GPIO userspace event loop, making
sure to pick up the timestamp there, as close as possible in time
relative to the actual event causing the interrupt.

Tested with a simple pushbutton GPIO on ux500 and seems to work
fine.

Cc: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
Reported-by: Felipe Balbi <felipe.balbi@linux.intel.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index aad84a6306c4..6e006e6df95a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -587,6 +587,9 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
  * @events: KFIFO for the GPIO events
  * @read_lock: mutex lock to protect reads from colliding with adding
  * new events to the FIFO
+ * @timestamp: cache for the timestamp storing it between hardirq
+ * and IRQ thread, used to bring the timestamp close to the actual
+ * event
  */
 struct lineevent_state {
 	struct gpio_device *gdev;
@@ -597,6 +600,7 @@ struct lineevent_state {
 	wait_queue_head_t wait;
 	DECLARE_KFIFO(events, struct gpioevent_data, 16);
 	struct mutex read_lock;
+	s64 timestamp;
 };
 
 #define GPIOEVENT_REQUEST_VALID_FLAGS \
@@ -731,7 +735,7 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p)
 	struct gpioevent_data ge;
 	int ret, level;
 
-	ge.timestamp = ktime_get_real_ns();
+	ge.timestamp = le->timestamp;
 	level = gpiod_get_value_cansleep(le->desc);
 
 	if (le->eflags & GPIOEVENT_REQUEST_RISING_EDGE
@@ -759,6 +763,19 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t lineevent_irq_handler(int irq, void *p)
+{
+	struct lineevent_state *le = p;
+
+	/*
+	 * Just store the timestamp in hardirq context so we get it as
+	 * close in time as possible to the actual event.
+	 */
+	le->timestamp = ktime_get_real_ns();
+
+	return IRQ_WAKE_THREAD;
+}
+
 static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 {
 	struct gpioevent_request eventreq;
@@ -851,7 +868,7 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 
 	/* Request a thread to read the events */
 	ret = request_threaded_irq(le->irq,
-			NULL,
+			lineevent_irq_handler,
 			lineevent_irq_thread,
 			irqflags,
 			le->label,
-- 
2.14.3

Re: [PATCH] gpio: Timestamp events in hardirq handler

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3033 bytes --]


Hi,

Linus Walleij <linus.walleij@linaro.org> writes:
> Add a hardirq handler to the GPIO userspace event loop, making
> sure to pick up the timestamp there, as close as possible in time
> relative to the actual event causing the interrupt.
>
> Tested with a simple pushbutton GPIO on ux500 and seems to work
> fine.
>
> Cc: Bartosz Golaszewski <brgl@bgdev.pl>
> Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
> Reported-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/gpiolib.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index aad84a6306c4..6e006e6df95a 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -587,6 +587,9 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
>   * @events: KFIFO for the GPIO events
>   * @read_lock: mutex lock to protect reads from colliding with adding
>   * new events to the FIFO
> + * @timestamp: cache for the timestamp storing it between hardirq
> + * and IRQ thread, used to bring the timestamp close to the actual
> + * event
>   */
>  struct lineevent_state {
>  	struct gpio_device *gdev;
> @@ -597,6 +600,7 @@ struct lineevent_state {
>  	wait_queue_head_t wait;
>  	DECLARE_KFIFO(events, struct gpioevent_data, 16);
>  	struct mutex read_lock;
> +	s64 timestamp;
>  };
>  
>  #define GPIOEVENT_REQUEST_VALID_FLAGS \
> @@ -731,7 +735,7 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p)
>  	struct gpioevent_data ge;
>  	int ret, level;
>  
> -	ge.timestamp = ktime_get_real_ns();
> +	ge.timestamp = le->timestamp;
>  	level = gpiod_get_value_cansleep(le->desc);
>  
>  	if (le->eflags & GPIOEVENT_REQUEST_RISING_EDGE
> @@ -759,6 +763,19 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p)
>  	return IRQ_HANDLED;
>  }
>  
> +static irqreturn_t lineevent_irq_handler(int irq, void *p)
> +{
> +	struct lineevent_state *le = p;
> +
> +	/*
> +	 * Just store the timestamp in hardirq context so we get it as
> +	 * close in time as possible to the actual event.
> +	 */
> +	le->timestamp = ktime_get_real_ns();
> +
> +	return IRQ_WAKE_THREAD;
> +}
> +
>  static int lineevent_create(struct gpio_device *gdev, void __user *ip)
>  {
>  	struct gpioevent_request eventreq;
> @@ -851,7 +868,7 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
>  
>  	/* Request a thread to read the events */
>  	ret = request_threaded_irq(le->irq,
> -			NULL,
> +			lineevent_irq_handler,

now that you have a hardirq handler, do you even need IRQF_ONESHOT? How
about using the hardirq handler to mask $this gpio's IRQ, then run
thread without IRQF_ONESHOT? This would help a in cases where the IRQ
line is shared.

Perhaps that should be done in a separate patch, though?

FWIW:

Reviewed-by: Felipe Balbi <felipe.balbi@linux.intel.com>

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] gpio: Timestamp events in hardirq handler

[Linus Walleij]

On Thu, Nov 30, 2017 at 10:32 AM, Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:

> now that you have a hardirq handler, do you even need IRQF_ONESHOT? How
> about using the hardirq handler to mask $this gpio's IRQ, then run
> thread without IRQF_ONESHOT? This would help a in cases where the IRQ
> line is shared.

Yeah maybe ... I'm a bit uncertain even about this.

We have GPIOs on slow (I2C, SPI) expanders, and they
sometimes need to go out and read that to even see what
IRQ that fired.

With this construction I *think* what happens is that it
timestamps it, then figures out (with some slow bus traffic)
if this IRQ was even ours and then calls the thread, if it was.
If it was not our IRQ the timestamp is just left crufting around.

Without shared IRQF_ONESHOT I am worried that something
else (shared) will come in between and pollute my timestamp.

Yours,
Linus Walleij

Re: [PATCH] gpio: Timestamp events in hardirq handler

[Alexander Stein]

On Thursday, November 30, 2017, 10:26:55 AM CET Linus Walleij wrote:
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -587,6 +587,9 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
>   * @events: KFIFO for the GPIO events
>   * @read_lock: mutex lock to protect reads from colliding with adding
>   * new events to the FIFO
> + * @timestamp: cache for the timestamp storing it between hardirq
> + * and IRQ thread, used to bring the timestamp close to the actual
> + * event
>   */
>  struct lineevent_state {
>  	struct gpio_device *gdev;
> @@ -597,6 +600,7 @@ struct lineevent_state {
>  	wait_queue_head_t wait;
>  	DECLARE_KFIFO(events, struct gpioevent_data, 16);
>  	struct mutex read_lock;
> +	s64 timestamp;
    ^^^
u64? ktime_get_real_ns() returns u64 and struct gpioevent_data
has __u64 for timestamp.

Best regards,
Alexander

Re: [PATCH] gpio: Timestamp events in hardirq handler

[Linus Walleij]

On Thu, Nov 30, 2017 at 10:46 AM, Alexander Stein
<alexander.stein@systec-electronic.com> wrote:
> On Thursday, November 30, 2017, 10:26:55 AM CET Linus Walleij wrote:

>> +     s64 timestamp;
>     ^^^
> u64? ktime_get_real_ns() returns u64 and struct gpioevent_data
> has __u64 for timestamp.

OK sorry, fixing it. Thanks for noticing!

Yours,
Linus Walleij

Re: [PATCH] gpio: Timestamp events in hardirq handler

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 490 bytes --]


Hi,

Linus Walleij <linus.walleij@linaro.org> writes:
> On Thu, Nov 30, 2017 at 10:46 AM, Alexander Stein
> <alexander.stein@systec-electronic.com> wrote:
>> On Thursday, November 30, 2017, 10:26:55 AM CET Linus Walleij wrote:
>
>>> +     s64 timestamp;
>>     ^^^
>> u64? ktime_get_real_ns() returns u64 and struct gpioevent_data
>> has __u64 for timestamp.
>
> OK sorry, fixing it. Thanks for noticing!

This is not on next. Will it miss this merge window?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] gpio: Timestamp events in hardirq handler

[Linus Walleij]

On Tue, Jan 23, 2018 at 1:37 PM, Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
> Linus Walleij <linus.walleij@linaro.org> writes:
>> On Thu, Nov 30, 2017 at 10:46 AM, Alexander Stein
>> <alexander.stein@systec-electronic.com> wrote:
>>> On Thursday, November 30, 2017, 10:26:55 AM CET Linus Walleij wrote:
>>
>>>> +     s64 timestamp;
>>>     ^^^
>>> u64? ktime_get_real_ns() returns u64 and struct gpioevent_data
>>> has __u64 for timestamp.
>>
>> OK sorry, fixing it. Thanks for noticing!
>
> This is not on next. Will it miss this merge window?

I realized yesterday when doing a security fix that it was missing.

Because I didn't get the merge conflict I expected :D

So I merged it. Don't worry. Sorry for the fuzz!

Yours,
Linus Walleij

Re: [PATCH] gpio: Timestamp events in hardirq handler

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 945 bytes --]


Hi,

Linus Walleij <linus.walleij@linaro.org> writes:
> On Tue, Jan 23, 2018 at 1:37 PM, Felipe Balbi
> <felipe.balbi@linux.intel.com> wrote:
>> Linus Walleij <linus.walleij@linaro.org> writes:
>>> On Thu, Nov 30, 2017 at 10:46 AM, Alexander Stein
>>> <alexander.stein@systec-electronic.com> wrote:
>>>> On Thursday, November 30, 2017, 10:26:55 AM CET Linus Walleij wrote:
>>>
>>>>> +     s64 timestamp;
>>>>     ^^^
>>>> u64? ktime_get_real_ns() returns u64 and struct gpioevent_data
>>>> has __u64 for timestamp.
>>>
>>> OK sorry, fixing it. Thanks for noticing!
>>
>> This is not on next. Will it miss this merge window?
>
> I realized yesterday when doing a security fix that it was missing.
>
> Because I didn't get the merge conflict I expected :D
>
> So I merged it. Don't worry. Sorry for the fuzz!

No problem, I'll refetch linux-next tomorrow or the day after and start
testing it.

Thanks

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]