[PATCH 1/2] poller: allow safely remove pollers in any time

[PATCH 1/2] poller: allow safely remove pollers in any time

[Aleksey Kuleshov]

---
 common/poller.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/common/poller.c b/common/poller.c
index 32795b6..7cec5d4 100644
--- a/common/poller.c
+++ b/common/poller.c
@@ -15,6 +15,7 @@
 
 static LIST_HEAD(poller_list);
 static int poller_active;
+static struct poller_struct *next;
 
 int poller_register(struct poller_struct *poller)
 {
@@ -33,6 +34,10 @@ int poller_unregister(struct poller_struct *poller)
 		return -ENODEV;
 
 
+	if (&next->list == &poller->list) {
+		next = list_entry(next->list.next, struct poller_struct, list);
+	}
+
 	list_del(&poller->list);
 	poller->registered = 0;
 
@@ -107,14 +112,14 @@ int poller_async_unregister(struct poller_async *pa)
 
 void poller_call(void)
 {
-	struct poller_struct *poller, *tmp;
+	struct poller_struct *poller;
 
 	if (poller_active)
 		return;
 
 	poller_active = 1;
 
-	list_for_each_entry_safe(poller, tmp, &poller_list, list)
+	list_for_each_entry_safe(poller, next, &poller_list, list)
 		poller->func(poller);
 
 	poller_active = 0;
-- 
2.6.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/2] input: make the Input Core be the last in the poller queue

[Aleksey Kuleshov]

This prevents the case when Input Core and event providers
have to run "at one time" so Input Core will work with non-relevant
data since it will be called first.
---
 drivers/input/input.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 31a9c22..1a8efc6 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -178,11 +178,13 @@ static void input_console_notify(struct input_notifier *in,
 	if (ev->value) {
 		kfifo_putc(ic->fifo, ascii);
 		ic->current_key = ascii;
+		poller_async_register(&ic->poller);
 		poller_call_async(&ic->poller, 400 * MSECOND,
 				  input_console_repeat, ic);
 	} else {
 		ic->current_key = 0;
 		poller_async_cancel(&ic->poller);
+		poller_async_unregister(&ic->poller);
 	}
 }
 
@@ -199,7 +201,6 @@ static int input_init(void)
 	ic->fifo = kfifo_alloc(32);
 	ic->notifier.notify = input_console_notify;
 	input_register_notfier(&ic->notifier);
-	poller_async_register(&ic->poller);
 
 	return console_register(&ic->console);
 }
-- 
2.6.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] input: make the Input Core be the last in the poller queue

[Sascha Hauer]

On Thu, Mar 03, 2016 at 06:17:10PM +0300, Aleksey Kuleshov wrote:
> This prevents the case when Input Core and event providers
> have to run "at one time" so Input Core will work with non-relevant
> data since it will be called first.
> ---
>  drivers/input/input.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

I tested the approach to add a .poll callback into struct input_device
and to let the input core register one poller for all input devices
rather than one poller for each device. This also gives the input core
better control when the input devices are polled. Would that approach go
into the right direction?
I have no ready-to-post patch for this approach, partly because it doesn't
solve the double-return problem and I currently don't understand why.

Sascha

> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 31a9c22..1a8efc6 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -178,11 +178,13 @@ static void input_console_notify(struct input_notifier *in,
>  	if (ev->value) {
>  		kfifo_putc(ic->fifo, ascii);
>  		ic->current_key = ascii;
> +		poller_async_register(&ic->poller);
>  		poller_call_async(&ic->poller, 400 * MSECOND,
>  				  input_console_repeat, ic);
>  	} else {
>  		ic->current_key = 0;
>  		poller_async_cancel(&ic->poller);
> +		poller_async_unregister(&ic->poller);
>  	}
>  }
>  
> @@ -199,7 +201,6 @@ static int input_init(void)
>  	ic->fifo = kfifo_alloc(32);
>  	ic->notifier.notify = input_console_notify;
>  	input_register_notfier(&ic->notifier);
> -	poller_async_register(&ic->poller);
>  
>  	return console_register(&ic->console);
>  }
> -- 
> 2.6.2
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] input: make the Input Core be the last in the poller queue

[Aleksey Kuleshov]

Hi Sascha,

04.03.2016, 10:19, "Sascha Hauer" <s.hauer@pengutronix.de>:
> On Thu, Mar 03, 2016 at 06:17:10PM +0300, Aleksey Kuleshov wrote:
>>  This prevents the case when Input Core and event providers
>>  have to run "at one time" so Input Core will work with non-relevant
>>  data since it will be called first.
>>  ---
>>   drivers/input/input.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> I tested the approach to add a .poll callback into struct input_device
> and to let the input core register one poller for all input devices
> rather than one poller for each device. This also gives the input core
> better control when the input devices are polled. Would that approach go
> into the right direction?

I don't understand the rules of "drivers/input/*".

All drivers register themselves as pollers? It seems yes.

All drivers should be regular polled? No, because, for example, USB keyboard
should be polled once per N ms.

All drivers could be polled per some-driver-specific N ms? Yes.
For example, one USB keyboard asks for 10 ms, the other - for 5 ms.

What about GPIO keyboards? Can they be polled once per N ms?
It seems "why not?".

So we have some pollers with, for example, 5ms, 10ms and regular callings.
Do you want to handle this inside the Input Core?

--------------------------------
Or may be it will be enough to poll every driver per 10 ms? Actually, why not?
If that's the case, then your approach is in the right direction.

Anyway, you will still have to do async call for repeating keys - and that's
400ms for the first press event, and then 40ms for subsuquent.

So, I guess, you will have two "poller_register" in Input Core (input.c):
1) for 10ms polling drivers - "driver poller";
2) for key repeating - "repeat poller".

Just make sure that "driver poller" registers first and after it you register "repeat poller".
--------------------------------


> I have no ready-to-post patch for this approach, partly because it doesn't
> solve the double-return problem and I currently don't understand why.

The whole problem of double-return is here:

static void input_console_repeat(void *ctx)
{
        struct input_console *ic = ctx;

        if (ic->current_key) {
                kfifo_putc(ic->fifo, ic->current_key);
                poller_call_async(&ic->poller, 40 * MSECOND,
                                  input_console_repeat, ic);
        }
}

This function is ASYNC. All what you have to do is to make sure that
this function gets called AFTER all input-driver-providers will be polled.
Or in other words make sure that the state is relevant when this function
gets called.
OR in other words there should be priority queue in which input_console_repeat
will have the lowest priority in respect to input-driver-providers.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] input: make the Input Core be the last in the poller queue

[Sascha Hauer]

On Fri, Mar 04, 2016 at 01:18:08PM +0300, Aleksey Kuleshov wrote:
> Hi Sascha,
> 
> 04.03.2016, 10:19, "Sascha Hauer" <s.hauer@pengutronix.de>:
> > On Thu, Mar 03, 2016 at 06:17:10PM +0300, Aleksey Kuleshov wrote:
> >>  This prevents the case when Input Core and event providers
> >>  have to run "at one time" so Input Core will work with non-relevant
> >>  data since it will be called first.
> >>  ---
> >>   drivers/input/input.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > I tested the approach to add a .poll callback into struct input_device
> > and to let the input core register one poller for all input devices
> > rather than one poller for each device. This also gives the input core
> > better control when the input devices are polled. Would that approach go
> > into the right direction?
> 
> I don't understand the rules of "drivers/input/*".
> 
> All drivers register themselves as pollers? It seems yes.

Yes, currently they do. Since all drivers do this and all drivers *must*
do it as there is no interrupt support, this could equally well be done
by the input core instead of the drivers.

> 
> All drivers should be regular polled? No, because, for example, USB keyboard
> should be polled once per N ms.
> 
> All drivers could be polled per some-driver-specific N ms? Yes.
> For example, one USB keyboard asks for 10 ms, the other - for 5 ms.

That's some adhoc value. Could be 5ms, 10ms, there's probably no good
reason for one or the other. It must be somewhere between not-too-often
to not slow down barebox and not-too-seldom to not introduce noticable
delays in the reaction to the keys.

> 
> What about GPIO keyboards? Can they be polled once per N ms?
> It seems "why not?".
> 
> So we have some pollers with, for example, 5ms, 10ms and regular callings.
> Do you want to handle this inside the Input Core?

Yes, but all should have the same timeout.

> 
> --------------------------------
> Or may be it will be enough to poll every driver per 10 ms? Actually, why not?
> If that's the case, then your approach is in the right direction.

ok

> 
> Anyway, you will still have to do async call for repeating keys - and that's
> 400ms for the first press event, and then 40ms for subsuquent.
> 
> So, I guess, you will have two "poller_register" in Input Core (input.c):
> 1) for 10ms polling drivers - "driver poller";
> 2) for key repeating - "repeat poller".

Not necessarily. It could be one poller which does something like:

	if (is_timeout(last_time, 10 * MSECOND))
		/* Not too often */
		return;

	last_time = get_time_ns();

	for_each_input_device(idev) {
		if (status_changed(idev)) {
			/* first key press */
			idev->repeat_timeout = 400 * MSECOND;
			idev->repeat = get_time_ns();
			send_key(idev);
		} else if (is_timeout(idev->repeat, idev->repeat_timeout)) {
			/* repeat key press */
			send_key(idev);
			idev->repeat = get_time_ns();
		}
	}

> 
> Just make sure that "driver poller" registers first and after it you register "repeat poller".
> --------------------------------
> 
> 
> > I have no ready-to-post patch for this approach, partly because it doesn't
> > solve the double-return problem and I currently don't understand why.
> 
> The whole problem of double-return is here:
> 
> static void input_console_repeat(void *ctx)
> {
>         struct input_console *ic = ctx;
> 
>         if (ic->current_key) {
>                 kfifo_putc(ic->fifo, ic->current_key);
>                 poller_call_async(&ic->poller, 40 * MSECOND,
>                                   input_console_repeat, ic);
>         }
> }
> 
> This function is ASYNC. All what you have to do is to make sure that
> this function gets called AFTER all input-driver-providers will be polled.

I thought that aswell and thought I should have fixed this issue in my
patch, but it didn't work as expected so there's still something wrong,
either in my thoughts or in my implementation.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] input: make the Input Core be the last in the poller queue

[Aleksey Kuleshov]

>>  This function is ASYNC. All what you have to do is to make sure that
>>  this function gets called AFTER all input-driver-providers will be polled.
>
> I thought that aswell and thought I should have fixed this issue in my
> patch, but it didn't work as expected so there's still something wrong,
> either in my thoughts or in my implementation.

Well, until we all see this patch nobody can help :)

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] input: make the Input Core be the last in the poller queue

[Sascha Hauer]

On Mon, Mar 07, 2016 at 12:16:46PM +0300, Aleksey Kuleshov wrote:
> >>  This function is ASYNC. All what you have to do is to make sure that
> >>  this function gets called AFTER all input-driver-providers will be polled.
> >
> > I thought that aswell and thought I should have fixed this issue in my
> > patch, but it didn't work as expected so there's still something wrong,
> > either in my thoughts or in my implementation.
> 
> Well, until we all see this patch nobody can help :)

Indeed not ;)

Here it is. It now works, presumable because of your recent change to
the interrupt transactions (ehci-hcd: preserve DTC in QH for interrupt
transactions). As the commit log says the patch is not yet ready.

Sascha

----------------------------8<---------------------------------

From e0bfdea8b65572f5119fe3b47c3c04cc6ef91fc2 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Thu, 3 Mar 2016 09:04:19 +0100
Subject: [PATCH] input: Let input core poll devices (wip)

So far each input device registers its own poller. This is not
necessary: We do not have interrupt support, so every input device
will need to poll. Let the core register one poller and drop the
ones from the individual devices. This also has the advantage that
we can handle the key repetition in the core.

This is not ready yet, only the USB keyboard driver is converted, the
other drivers need fixup aswell.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/input/input.c   | 47 ++++++++++++++++++++++++++++-------------------
 drivers/input/usb_kbd.c | 26 +++++++-------------------
 include/input/input.h   |  4 ++++
 3 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 31a9c22..f59bd27 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -36,7 +36,7 @@ void input_unregister_notfier(struct input_notifier *in)
 
 void input_report_key_event(struct input_device *idev, unsigned int code, int value)
 {
-	struct input_event event;
+	struct input_event *event = &idev->event;
 	struct input_notifier *in;
 
 	if (code > KEY_MAX)
@@ -47,11 +47,13 @@ void input_report_key_event(struct input_device *idev, unsigned int code, int va
 	else
 		clear_bit(code, &idev->keys);
 
-	event.code = code;
-	event.value = value;
+	event->code = code;
+	event->value = value;
+	idev->start = get_time_ns();
+	idev->timeout = 400 * MSECOND;
 
 	list_for_each_entry(in, &input_consumers, list)
-		in->notify(in, &event);
+		in->notify(in, event);
 }
 
 static LIST_HEAD(input_devices);
@@ -107,17 +109,31 @@ static int input_console_getc(struct console_device *cdev)
 	return c;
 }
 
-static void input_console_repeat(void *ctx)
+static void input_devices_poller_func(struct poller_struct *poller)
 {
-	struct input_console *ic = ctx;
+	struct input_device *idev;
+	static uint64_t last_poll;
+	struct input_notifier *in;
+
+	if (!is_timeout(last_poll, 10 * MSECOND))
+		return;
 
-	if (ic->current_key) {
-		kfifo_putc(ic->fifo, ic->current_key);
-		poller_call_async(&ic->poller, 40 * MSECOND,
-				  input_console_repeat, ic);
+	list_for_each_entry(idev, &input_devices, list) {
+		idev->poll(idev);
+
+		if (idev->event.value && is_timeout(idev->start, idev->timeout)) {
+			list_for_each_entry(in, &input_consumers, list)
+				in->notify(in, &idev->event);
+			idev->timeout = 40 * MSECOND;
+			idev->start = get_time_ns();
+		}
 	}
 }
 
+struct poller_struct input_poller = {
+	.func = input_devices_poller_func,
+};
+
 struct keycode {
 	unsigned char key;
 	unsigned char ascii;
@@ -175,15 +191,8 @@ static void input_console_notify(struct input_notifier *in,
 
 	pr_debug("map %02x KEY: 0x%04x code: %d\n", modstate, ascii, ev->code);
 
-	if (ev->value) {
+	if (ev->value)
 		kfifo_putc(ic->fifo, ascii);
-		ic->current_key = ascii;
-		poller_call_async(&ic->poller, 400 * MSECOND,
-				  input_console_repeat, ic);
-	} else {
-		ic->current_key = 0;
-		poller_async_cancel(&ic->poller);
-	}
 }
 
 static struct input_console input_console;
@@ -199,7 +208,7 @@ static int input_init(void)
 	ic->fifo = kfifo_alloc(32);
 	ic->notifier.notify = input_console_notify;
 	input_register_notfier(&ic->notifier);
-	poller_async_register(&ic->poller);
+	poller_register(&input_poller);
 
 	return console_register(&ic->console);
 }
diff --git a/drivers/input/usb_kbd.c b/drivers/input/usb_kbd.c
index 2acc95d..a0e3120 100644
--- a/drivers/input/usb_kbd.c
+++ b/drivers/input/usb_kbd.c
@@ -48,7 +48,7 @@ struct usb_kbd_pdata;
 struct usb_kbd_pdata {
 	uint8_t		*new;
 	uint8_t		old[USB_KBD_BOOT_REPORT_SIZE];
-	struct poller_async	poller;
+	uint8_t		flags;
 	struct usb_device	*usbdev;
 	unsigned long	intpipe;
 	int		intpktsize;
@@ -91,9 +91,10 @@ static const unsigned char usb_kbd_keycode[256] = {
 	150,158,159,128,136,177,178,176,142,152,173,140
 };
 
-static void usb_kbd_poll(void *arg)
+static void usb_kbd_poll(struct input_device *input)
 {
-	struct usb_kbd_pdata *data = arg;
+	struct usb_kbd_pdata *data = container_of(input,
+						  struct usb_kbd_pdata, input);
 	struct usb_device *usbdev = data->usbdev;
 	int ret, i;
 
@@ -138,7 +139,7 @@ static void usb_kbd_poll(void *arg)
 	memcpy(data->old, data->new, USB_KBD_BOOT_REPORT_SIZE);
 
 exit:
-	poller_call_async(&data->poller, data->intinterval * MSECOND, usb_kbd_poll, data);
+	return;
 }
 
 static int usb_kbd_probe(struct usb_device *usbdev,
@@ -188,28 +189,15 @@ static int usb_kbd_probe(struct usb_device *usbdev,
 	} else
 		dev_dbg(&usbdev->dev, "poll keyboard via int ep\n");
 
-	ret = input_device_register(&data->input);
-	if (ret) {
-		dev_err(&usbdev->dev, "can't register input\n");
-		return ret;
-	}
-
-	ret = poller_async_register(&data->poller);
-	if (ret) {
-		dev_err(&usbdev->dev, "can't setup poller\n");
-		return ret;
-	}
-
-	poller_call_async(&data->poller, data->intinterval * MSECOND, usb_kbd_poll, data);
+	data->input.poll = usb_kbd_poll;
 
-	return 0;
+	return input_device_register(&data->input);
 }
 
 static void usb_kbd_disconnect(struct usb_device *usbdev)
 {
 	struct usb_kbd_pdata *data = usbdev->drv_data;
 
-	poller_async_unregister(&data->poller);
 	input_device_unregister(&data->input);
 	dma_free(data->new);
 	free(data);
diff --git a/include/input/input.h b/include/input/input.h
index dbf3e7f..44280af 100644
--- a/include/input/input.h
+++ b/include/input/input.h
@@ -13,6 +13,10 @@ struct input_event {
 struct input_device {
 	struct list_head list;
 	DECLARE_BITMAP(keys, KEY_CNT);
+	void (*poll)(struct input_device *);
+	struct input_event event;
+	uint64_t start;
+	uint64_t timeout;
 };
 
 void input_report_key_event(struct input_device *idev, unsigned int code, int value);
-- 
2.7.0

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] input: make the Input Core be the last in the poller queue

[Aleksey Kuleshov]

> Here it is. It now works, presumable because of your recent change to
> the interrupt transactions (ehci-hcd: preserve DTC in QH for interrupt
> transactions).

I don't know what you mean, but this patch almost worked. I slightly changed it
and I get working patch: I tested 4 keyboards, only one of them (some Genius)
behave strangely, but this is not related to my recent patch you mentioned.

For Genius keyboard I got these messages:
    Bus 001 Device 003: ID 1c4f:0026 USB Keyboard
    usb-keyboard usb1-0-2: USB keyboard found
    usb-keyboard usb1-0-2: Timeout poll on interrupt endpoint
    usb-keyboard usb1-0-2: got wrong buffer back (00000000 instead of 82130fd8)

And tons of such messages:
    usb-keyboard usb1-0-2: usb_submit_int_msg() failed. Keyboard disconnect?
when there was screen scrolling (which is very slow in my case).

Can you elaborate a little what does not work in your case?

PS. Right now, I'm trying to investigate strange behaviour of Genius keyboard.
I think next week I'll send you patch on your patch.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] input: make the Input Core be the last in the poller queue

[Sascha Hauer]

On Fri, Mar 11, 2016 at 03:25:01PM +0300, Aleksey Kuleshov wrote:
> > Here it is. It now works, presumable because of your recent change to
> > the interrupt transactions (ehci-hcd: preserve DTC in QH for interrupt
> > transactions).
> 
> I don't know what you mean, but this patch almost worked. I slightly changed it
> and I get working patch: I tested 4 keyboards, only one of them (some Genius)
> behave strangely, but this is not related to my recent patch you mentioned.
> 
> For Genius keyboard I got these messages:
>     Bus 001 Device 003: ID 1c4f:0026 USB Keyboard
>     usb-keyboard usb1-0-2: USB keyboard found
>     usb-keyboard usb1-0-2: Timeout poll on interrupt endpoint
>     usb-keyboard usb1-0-2: got wrong buffer back (00000000 instead of 82130fd8)
> 
> And tons of such messages:
>     usb-keyboard usb1-0-2: usb_submit_int_msg() failed. Keyboard disconnect?
> when there was screen scrolling (which is very slow in my case).
> 
> Can you elaborate a little what does not work in your case?

You misunderstood me. My patch works for me without issues. The question
is why it does, because last time I tested it it didn't work. I assume
that your "ehci-hcd: preserve DTC in QH for interrupt transactions"
patch which is now in my tree *fixes* the behaviour for me.

> 
> PS. Right now, I'm trying to investigate strange behaviour of Genius keyboard.
> I think next week I'll send you patch on your patch.

Thanks

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] input: make the Input Core be the last in the poller queue

[Aleksey Kuleshov]

11.03.2016, 17:54, "Sascha Hauer" <s.hauer@pengutronix.de>:
> On Fri, Mar 11, 2016 at 03:25:01PM +0300, Aleksey Kuleshov wrote:
>>  > Here it is. It now works, presumable because of your recent change to

> You misunderstood me.

Silly me. I read it like "It noT works"...

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] input: make the Input Core be the last in the poller queue

[Aleksey Kuleshov]

Genius USB keyboard can't work with interrupt msgs, only control msgs.
Then, with control msgs if there were no report requests during ~10 secs, EHCI stack
in Barebox reports "timeout error" infinitely until 'usb' command will be exectued
(i.e. until EHCI halt + reset).

So:

1) usb_kbd can detect "timeout error" (or some other error) so your patch, as for now,
lacks status checking from idev->poll().

2) I know this is a WIP patch, but

> +static void input_devices_poller_func(struct poller_struct *poller)
>  {
> - struct input_console *ic = ctx;
> + struct input_device *idev;
> + static uint64_t last_poll;
> + struct input_notifier *in;
> +
> + if (!is_timeout(last_poll, 10 * MSECOND))
> + return;

the last_poll var was never set and why 'is_timeout' is here if you have async_calls?
Maybe it should be like this?:

static struct poller_async input_poller;
static void input_devices_poller_func(void *arg)
{
        struct input_device *idev;
        struct input_notifier *in;

        list_for_each_entry(idev, &input_devices, list) {
                idev->poll(idev);

                if (idev->event.value && is_timeout(idev->start, idev->timeout)) {
                        list_for_each_entry(in, &input_consumers, list)
                                in->notify(in, &idev->event);
                        idev->timeout = 40 * MSECOND;
                        idev->start = get_time_ns();
                }
        }

        poller_call_async(&input_poller, 10 * MSECOND, input_devices_poller_func, NULL);
}

And that's all what I can say about EHCI/input/usb_kbd so far.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] input: make the Input Core be the last in the poller queue

[Sascha Hauer]

On Mon, Mar 14, 2016 at 07:26:33PM +0300, Aleksey Kuleshov wrote:
> Genius USB keyboard can't work with interrupt msgs, only control msgs.
> Then, with control msgs if there were no report requests during ~10 secs, EHCI stack
> in Barebox reports "timeout error" infinitely until 'usb' command will be exectued
> (i.e. until EHCI halt + reset).
> 
> So:
> 
> 1) usb_kbd can detect "timeout error" (or some other error) so your patch, as for now,
> lacks status checking from idev->poll().
> 
> 2) I know this is a WIP patch, but
> 
> > +static void input_devices_poller_func(struct poller_struct *poller)
> >  {
> > - struct input_console *ic = ctx;
> > + struct input_device *idev;
> > + static uint64_t last_poll;
> > + struct input_notifier *in;
> > +
> > + if (!is_timeout(last_poll, 10 * MSECOND))
> > + return;
> 
> the last_poll var was never set and why 'is_timeout' is here if you have async_calls?

The intention was to limit input polling to every 10ms. Indeed the above
doesn't work as expected and could be better done with an async poller.

> Maybe it should be like this?:
> 
> static struct poller_async input_poller;
> static void input_devices_poller_func(void *arg)
> {
>         struct input_device *idev;
>         struct input_notifier *in;
> 
>         list_for_each_entry(idev, &input_devices, list) {
>                 idev->poll(idev);
> 
>                 if (idev->event.value && is_timeout(idev->start, idev->timeout)) {
>                         list_for_each_entry(in, &input_consumers, list)
>                                 in->notify(in, &idev->event);
>                         idev->timeout = 40 * MSECOND;
>                         idev->start = get_time_ns();
>                 }
>         }
> 
>         poller_call_async(&input_poller, 10 * MSECOND, input_devices_poller_func, NULL);
> }

Yes, looks better.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] poller: allow safely remove pollers in any time

[Antony Pavlov]

On Thu,  3 Mar 2016 18:17:09 +0300
Aleksey Kuleshov <rndfax@yandex.ru> wrote:

Please add 'Signed-off-by' to your patches.

> ---
>  common/poller.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/common/poller.c b/common/poller.c
> index 32795b6..7cec5d4 100644
> --- a/common/poller.c
> +++ b/common/poller.c
> @@ -15,6 +15,7 @@
>  
>  static LIST_HEAD(poller_list);
>  static int poller_active;
> +static struct poller_struct *next;
>  
>  int poller_register(struct poller_struct *poller)
>  {
> @@ -33,6 +34,10 @@ int poller_unregister(struct poller_struct *poller)
>  		return -ENODEV;
>  
>  
> +	if (&next->list == &poller->list) {
> +		next = list_entry(next->list.next, struct poller_struct, list);
> +	}
> +
>  	list_del(&poller->list);
>  	poller->registered = 0;
>  
> @@ -107,14 +112,14 @@ int poller_async_unregister(struct poller_async *pa)
>  
>  void poller_call(void)
>  {
> -	struct poller_struct *poller, *tmp;
> +	struct poller_struct *poller;
>  
>  	if (poller_active)
>  		return;
>  
>  	poller_active = 1;
>  
> -	list_for_each_entry_safe(poller, tmp, &poller_list, list)
> +	list_for_each_entry_safe(poller, next, &poller_list, list)
>  		poller->func(poller);
>  
>  	poller_active = 0;
> -- 
> 2.6.2
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox


-- 
-- 
Best regards,
  Antony Pavlov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox