LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] Input: serio - clear serio event queue after sysfs driver rebind
@ 2010-11-22 23:09 Duncan Laurie
  2010-11-27  8:41 ` Dmitry Torokhov
  0 siblings, 1 reply; 7+ messages in thread
From: Duncan Laurie @ 2010-11-22 23:09 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Duncan Laurie

When rebinding a serio driver via sysfs drvctl interface it is possible for
an interrupt to trigger after the disconnect of the existing driver and
before the binding of the new driver.  This will cause the serio interrupt
handler to queue a rescan event which will disconnect the new driver
immediately after it is attached.

This change clears the serio event queue after processing the drvctl
request but before releasing the serio mutex, which will clear any queued
rescans before they can get processed.

Reproduction involves issuing a rebind of device port from psmouse driver
to serio_raw driver while generating input to trigger interrupts.  Then
checking to see if the corresponding i8042/serio4/driver is correctly
attached to the serio_raw driver instead of psmouse.

Signed-off-by: Duncan Laurie <dlaurie@chromium.org>
---
 drivers/input/serio/serio.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
index 405bf21..a66307e 100644
--- a/drivers/input/serio/serio.c
+++ b/drivers/input/serio/serio.c
@@ -454,6 +454,7 @@ static ssize_t serio_rebind_driver(struct device *dev, struct device_attribute *
 		serio_disconnect_port(serio);
 		error = serio_bind_driver(serio, to_serio_driver(drv));
 		put_driver(drv);
+		serio_remove_pending_events(serio);
 	} else {
 		error = -EINVAL;
 	}
-- 
1.7.3.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Input: serio - clear serio event queue after sysfs driver rebind
  2010-11-22 23:09 [PATCH] Input: serio - clear serio event queue after sysfs driver rebind Duncan Laurie
@ 2010-11-27  8:41 ` Dmitry Torokhov
  2010-11-29 21:37   ` Duncan Laurie
  0 siblings, 1 reply; 7+ messages in thread
From: Dmitry Torokhov @ 2010-11-27  8:41 UTC (permalink / raw)
  To: Duncan Laurie; +Cc: linux-input, linux-kernel

Hi Duncan,

On Mon, Nov 22, 2010 at 03:09:50PM -0800, Duncan Laurie wrote:
> When rebinding a serio driver via sysfs drvctl interface it is possible for
> an interrupt to trigger after the disconnect of the existing driver and
> before the binding of the new driver.  This will cause the serio interrupt
> handler to queue a rescan event which will disconnect the new driver
> immediately after it is attached.
> 
> This change clears the serio event queue after processing the drvctl
> request but before releasing the serio mutex, which will clear any queued
> rescans before they can get processed.
> 
> Reproduction involves issuing a rebind of device port from psmouse driver
> to serio_raw driver while generating input to trigger interrupts.  Then
> checking to see if the corresponding i8042/serio4/driver is correctly
> attached to the serio_raw driver instead of psmouse.
> 
> Signed-off-by: Duncan Laurie <dlaurie@chromium.org>
> ---
>  drivers/input/serio/serio.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
> index 405bf21..a66307e 100644
> --- a/drivers/input/serio/serio.c
> +++ b/drivers/input/serio/serio.c
> @@ -454,6 +454,7 @@ static ssize_t serio_rebind_driver(struct device *dev, struct device_attribute *
>  		serio_disconnect_port(serio);
>  		error = serio_bind_driver(serio, to_serio_driver(drv));
>  		put_driver(drv);
> +		serio_remove_pending_events(serio);

Hmm, makes sense, although should we limit events being removed to
rescan events only?

-- 
Dmitry

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Input: serio - clear serio event queue after sysfs driver rebind
  2010-11-27  8:41 ` Dmitry Torokhov
@ 2010-11-29 21:37   ` Duncan Laurie
  2010-12-08  5:12     ` Dmitry Torokhov
  0 siblings, 1 reply; 7+ messages in thread
From: Duncan Laurie @ 2010-11-29 21:37 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: linux-input, linux-kernel

On Sat, Nov 27, 2010 at 12:41 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Duncan,
>
> On Mon, Nov 22, 2010 at 03:09:50PM -0800, Duncan Laurie wrote:
>> When rebinding a serio driver via sysfs drvctl interface it is possible for
>> an interrupt to trigger after the disconnect of the existing driver and
>> before the binding of the new driver.  This will cause the serio interrupt
>> handler to queue a rescan event which will disconnect the new driver
>> immediately after it is attached.
>>
>> This change clears the serio event queue after processing the drvctl
>> request but before releasing the serio mutex, which will clear any queued
>> rescans before they can get processed.
>>
>> Reproduction involves issuing a rebind of device port from psmouse driver
>> to serio_raw driver while generating input to trigger interrupts.  Then
>> checking to see if the corresponding i8042/serio4/driver is correctly
>> attached to the serio_raw driver instead of psmouse.
>>
>> Signed-off-by: Duncan Laurie <dlaurie@chromium.org>
>> ---
>>  drivers/input/serio/serio.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
>> index 405bf21..a66307e 100644
>> --- a/drivers/input/serio/serio.c
>> +++ b/drivers/input/serio/serio.c
>> @@ -454,6 +454,7 @@ static ssize_t serio_rebind_driver(struct device *dev, struct device_attribute *
>>               serio_disconnect_port(serio);
>>               error = serio_bind_driver(serio, to_serio_driver(drv));
>>               put_driver(drv);
>> +             serio_remove_pending_events(serio);
>
> Hmm, makes sense, although should we limit events being removed to
> rescan events only?
>


That seems reasonable.  It would mean adding a new function or a
parameter to the existing serio_remove_pending_events function, do you
have a preference?

Thanks,
-duncan

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Input: serio - clear serio event queue after sysfs driver rebind
  2010-11-29 21:37   ` Duncan Laurie
@ 2010-12-08  5:12     ` Dmitry Torokhov
  2011-02-01  9:21       ` Dmitry Torokhov
  0 siblings, 1 reply; 7+ messages in thread
From: Dmitry Torokhov @ 2010-12-08  5:12 UTC (permalink / raw)
  To: Duncan Laurie; +Cc: linux-input, linux-kernel

On Mon, Nov 29, 2010 at 01:37:35PM -0800, Duncan Laurie wrote:
> On Sat, Nov 27, 2010 at 12:41 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Hi Duncan,
> >
> > On Mon, Nov 22, 2010 at 03:09:50PM -0800, Duncan Laurie wrote:
> >> When rebinding a serio driver via sysfs drvctl interface it is possible for
> >> an interrupt to trigger after the disconnect of the existing driver and
> >> before the binding of the new driver.  This will cause the serio interrupt
> >> handler to queue a rescan event which will disconnect the new driver
> >> immediately after it is attached.
> >>
> >> This change clears the serio event queue after processing the drvctl
> >> request but before releasing the serio mutex, which will clear any queued
> >> rescans before they can get processed.
> >>
> >> Reproduction involves issuing a rebind of device port from psmouse driver
> >> to serio_raw driver while generating input to trigger interrupts.  Then
> >> checking to see if the corresponding i8042/serio4/driver is correctly
> >> attached to the serio_raw driver instead of psmouse.
> >>
> >> Signed-off-by: Duncan Laurie <dlaurie@chromium.org>
> >> ---
> >>  drivers/input/serio/serio.c |    1 +
> >>  1 files changed, 1 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
> >> index 405bf21..a66307e 100644
> >> --- a/drivers/input/serio/serio.c
> >> +++ b/drivers/input/serio/serio.c
> >> @@ -454,6 +454,7 @@ static ssize_t serio_rebind_driver(struct device *dev, struct device_attribute *
> >>               serio_disconnect_port(serio);
> >>               error = serio_bind_driver(serio, to_serio_driver(drv));
> >>               put_driver(drv);
> >> +             serio_remove_pending_events(serio);
> >
> > Hmm, makes sense, although should we limit events being removed to
> > rescan events only?
> >
> 
> 
> That seems reasonable.  It would mean adding a new function or a
> parameter to the existing serio_remove_pending_events function, do you
> have a preference?
> 

I wonder if a boolean parameter (rescan_only) would not be the best
option.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Input: serio - clear serio event queue after sysfs driver rebind
  2010-12-08  5:12     ` Dmitry Torokhov
@ 2011-02-01  9:21       ` Dmitry Torokhov
  2011-02-02 22:41         ` Duncan Laurie
  0 siblings, 1 reply; 7+ messages in thread
From: Dmitry Torokhov @ 2011-02-01  9:21 UTC (permalink / raw)
  To: Duncan Laurie; +Cc: linux-input, linux-kernel

On Tue, Dec 07, 2010 at 09:12:11PM -0800, Dmitry Torokhov wrote:
> On Mon, Nov 29, 2010 at 01:37:35PM -0800, Duncan Laurie wrote:
> > On Sat, Nov 27, 2010 at 12:41 AM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > Hi Duncan,
> > >
> > > On Mon, Nov 22, 2010 at 03:09:50PM -0800, Duncan Laurie wrote:
> > >> When rebinding a serio driver via sysfs drvctl interface it is possible for
> > >> an interrupt to trigger after the disconnect of the existing driver and
> > >> before the binding of the new driver.  This will cause the serio interrupt
> > >> handler to queue a rescan event which will disconnect the new driver
> > >> immediately after it is attached.
> > >>
> > >> This change clears the serio event queue after processing the drvctl
> > >> request but before releasing the serio mutex, which will clear any queued
> > >> rescans before they can get processed.
> > >>
> > >> Reproduction involves issuing a rebind of device port from psmouse driver
> > >> to serio_raw driver while generating input to trigger interrupts.  Then
> > >> checking to see if the corresponding i8042/serio4/driver is correctly
> > >> attached to the serio_raw driver instead of psmouse.
> > >>
> > >> Signed-off-by: Duncan Laurie <dlaurie@chromium.org>
> > >> ---
> > >>  drivers/input/serio/serio.c |    1 +
> > >>  1 files changed, 1 insertions(+), 0 deletions(-)
> > >>
> > >> diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
> > >> index 405bf21..a66307e 100644
> > >> --- a/drivers/input/serio/serio.c
> > >> +++ b/drivers/input/serio/serio.c
> > >> @@ -454,6 +454,7 @@ static ssize_t serio_rebind_driver(struct device *dev, struct device_attribute *
> > >>               serio_disconnect_port(serio);
> > >>               error = serio_bind_driver(serio, to_serio_driver(drv));
> > >>               put_driver(drv);
> > >> +             serio_remove_pending_events(serio);
> > >
> > > Hmm, makes sense, although should we limit events being removed to
> > > rescan events only?
> > >
> > 
> > 
> > That seems reasonable.  It would mean adding a new function or a
> > parameter to the existing serio_remove_pending_events function, do you
> > have a preference?
> > 
> 
> I wonder if a boolean parameter (rescan_only) would not be the best
> option.
> 

Hi Duncan,

I eneded up with the following patch, could you please try and see if
it still works for you? 

Thanks.

-- 
Dmitry

Input: serio - clear pending rescans after sysfs driver rebind

From: Duncan Laurie <dlaurie@chromium.org>

When rebinding a serio driver via sysfs drvctl interface it is
possible for an interrupt to trigger after the disconnect of the
existing driver and before the binding of the new driver.  This will
cause the serio interrupt handler to queue a rescan event which will
disconnect the new driver immediately after it is attached.

This change removes pending rescans from the serio event queue after
processing the drvctl request but before releasing the serio mutex.

Reproduction involves issuing a rebind of device port from psmouse
driver to serio_raw driver while generating input to trigger
interrupts.  Then checking to see if the corresponding
i8042/serio4/driver is correctly attached to the serio_raw driver
instead of psmouse.

Signed-off-by: Duncan Laurie <dlaurie@chromium.org>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/serio/serio.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)


diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
index db5b0bc..7c38d1f 100644
--- a/drivers/input/serio/serio.c
+++ b/drivers/input/serio/serio.c
@@ -188,7 +188,8 @@ static void serio_free_event(struct serio_event *event)
 	kfree(event);
 }
 
-static void serio_remove_duplicate_events(struct serio_event *event)
+static void serio_remove_duplicate_events(void *object,
+					  enum serio_event_type type)
 {
 	struct serio_event *e, *next;
 	unsigned long flags;
@@ -196,13 +197,13 @@ static void serio_remove_duplicate_events(struct serio_event *event)
 	spin_lock_irqsave(&serio_event_lock, flags);
 
 	list_for_each_entry_safe(e, next, &serio_event_list, node) {
-		if (event->object == e->object) {
+		if (object == e->object) {
 			/*
 			 * If this event is of different type we should not
 			 * look further - we only suppress duplicate events
 			 * that were sent back-to-back.
 			 */
-			if (event->type != e->type)
+			if (type != e->type)
 				break;
 
 			list_del_init(&e->node);
@@ -245,7 +246,7 @@ static void serio_handle_event(struct work_struct *work)
 			break;
 		}
 
-		serio_remove_duplicate_events(event);
+		serio_remove_duplicate_events(event->object, event->type);
 		serio_free_event(event);
 	}
 
@@ -436,10 +437,12 @@ static ssize_t serio_rebind_driver(struct device *dev, struct device_attribute *
 	} else if (!strncmp(buf, "rescan", count)) {
 		serio_disconnect_port(serio);
 		serio_find_driver(serio);
+		serio_remove_duplicate_events(serio, SERIO_RESCAN_PORT);
 	} else if ((drv = driver_find(buf, &serio_bus)) != NULL) {
 		serio_disconnect_port(serio);
 		error = serio_bind_driver(serio, to_serio_driver(drv));
 		put_driver(drv);
+		serio_remove_duplicate_events(serio, SERIO_RESCAN_PORT);
 	} else {
 		error = -EINVAL;
 	}

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Input: serio - clear serio event queue after sysfs driver rebind
  2011-02-01  9:21       ` Dmitry Torokhov
@ 2011-02-02 22:41         ` Duncan Laurie
  2011-02-02 22:58           ` Dmitry Torokhov
  0 siblings, 1 reply; 7+ messages in thread
From: Duncan Laurie @ 2011-02-02 22:41 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: linux-input, linux-kernel

On Tue, Feb 1, 2011 at 1:21 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Tue, Dec 07, 2010 at 09:12:11PM -0800, Dmitry Torokhov wrote:
>> On Mon, Nov 29, 2010 at 01:37:35PM -0800, Duncan Laurie wrote:
>> > On Sat, Nov 27, 2010 at 12:41 AM, Dmitry Torokhov
>> > <dmitry.torokhov@gmail.com> wrote:
>> > > Hi Duncan,
>> > >
>> > > On Mon, Nov 22, 2010 at 03:09:50PM -0800, Duncan Laurie wrote:
>> > >> When rebinding a serio driver via sysfs drvctl interface it is possible for
>> > >> an interrupt to trigger after the disconnect of the existing driver and
>> > >> before the binding of the new driver.  This will cause the serio interrupt
>> > >> handler to queue a rescan event which will disconnect the new driver
>> > >> immediately after it is attached.
>> > >>
>> > >> This change clears the serio event queue after processing the drvctl
>> > >> request but before releasing the serio mutex, which will clear any queued
>> > >> rescans before they can get processed.
>> > >>
>> > >> Reproduction involves issuing a rebind of device port from psmouse driver
>> > >> to serio_raw driver while generating input to trigger interrupts.  Then
>> > >> checking to see if the corresponding i8042/serio4/driver is correctly
>> > >> attached to the serio_raw driver instead of psmouse.
>> > >>
>> > >> Signed-off-by: Duncan Laurie <dlaurie@chromium.org>
>> > >> ---
>> > >>  drivers/input/serio/serio.c |    1 +
>> > >>  1 files changed, 1 insertions(+), 0 deletions(-)
>> > >>
>> > >> diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
>> > >> index 405bf21..a66307e 100644
>> > >> --- a/drivers/input/serio/serio.c
>> > >> +++ b/drivers/input/serio/serio.c
>> > >> @@ -454,6 +454,7 @@ static ssize_t serio_rebind_driver(struct device *dev, struct device_attribute *
>> > >>               serio_disconnect_port(serio);
>> > >>               error = serio_bind_driver(serio, to_serio_driver(drv));
>> > >>               put_driver(drv);
>> > >> +             serio_remove_pending_events(serio);
>> > >
>> > > Hmm, makes sense, although should we limit events being removed to
>> > > rescan events only?
>> > >
>> >
>> >
>> > That seems reasonable.  It would mean adding a new function or a
>> > parameter to the existing serio_remove_pending_events function, do you
>> > have a preference?
>> >
>>
>> I wonder if a boolean parameter (rescan_only) would not be the best
>> option.
>>
>
> Hi Duncan,
>
> I eneded up with the following patch, could you please try and see if
> it still works for you?
>


Hi Dmitry,

I tested the patch on a CR-48 device (which is where I was seeing the
problem because we switch to serio_raw at boot for the trackpad) and
it does fix the problem.

Thanks for sending this, it had slipped off my todo list.

-duncan

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Input: serio - clear serio event queue after sysfs driver rebind
  2011-02-02 22:41         ` Duncan Laurie
@ 2011-02-02 22:58           ` Dmitry Torokhov
  0 siblings, 0 replies; 7+ messages in thread
From: Dmitry Torokhov @ 2011-02-02 22:58 UTC (permalink / raw)
  To: Duncan Laurie; +Cc: linux-input, linux-kernel

On Wed, Feb 02, 2011 at 02:41:38PM -0800, Duncan Laurie wrote:
> On Tue, Feb 1, 2011 at 1:21 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Tue, Dec 07, 2010 at 09:12:11PM -0800, Dmitry Torokhov wrote:
> >> On Mon, Nov 29, 2010 at 01:37:35PM -0800, Duncan Laurie wrote:
> >> > On Sat, Nov 27, 2010 at 12:41 AM, Dmitry Torokhov
> >> > <dmitry.torokhov@gmail.com> wrote:
> >> > > Hi Duncan,
> >> > >
> >> > > On Mon, Nov 22, 2010 at 03:09:50PM -0800, Duncan Laurie wrote:
> >> > >> When rebinding a serio driver via sysfs drvctl interface it is possible for
> >> > >> an interrupt to trigger after the disconnect of the existing driver and
> >> > >> before the binding of the new driver.  This will cause the serio interrupt
> >> > >> handler to queue a rescan event which will disconnect the new driver
> >> > >> immediately after it is attached.
> >> > >>
> >> > >> This change clears the serio event queue after processing the drvctl
> >> > >> request but before releasing the serio mutex, which will clear any queued
> >> > >> rescans before they can get processed.
> >> > >>
> >> > >> Reproduction involves issuing a rebind of device port from psmouse driver
> >> > >> to serio_raw driver while generating input to trigger interrupts.  Then
> >> > >> checking to see if the corresponding i8042/serio4/driver is correctly
> >> > >> attached to the serio_raw driver instead of psmouse.
> >> > >>
> >> > >> Signed-off-by: Duncan Laurie <dlaurie@chromium.org>
> >> > >> ---
> >> > >>  drivers/input/serio/serio.c |    1 +
> >> > >>  1 files changed, 1 insertions(+), 0 deletions(-)
> >> > >>
> >> > >> diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
> >> > >> index 405bf21..a66307e 100644
> >> > >> --- a/drivers/input/serio/serio.c
> >> > >> +++ b/drivers/input/serio/serio.c
> >> > >> @@ -454,6 +454,7 @@ static ssize_t serio_rebind_driver(struct device *dev, struct device_attribute *
> >> > >>               serio_disconnect_port(serio);
> >> > >>               error = serio_bind_driver(serio, to_serio_driver(drv));
> >> > >>               put_driver(drv);
> >> > >> +             serio_remove_pending_events(serio);
> >> > >
> >> > > Hmm, makes sense, although should we limit events being removed to
> >> > > rescan events only?
> >> > >
> >> >
> >> >
> >> > That seems reasonable.  It would mean adding a new function or a
> >> > parameter to the existing serio_remove_pending_events function, do you
> >> > have a preference?
> >> >
> >>
> >> I wonder if a boolean parameter (rescan_only) would not be the best
> >> option.
> >>
> >
> > Hi Duncan,
> >
> > I eneded up with the following patch, could you please try and see if
> > it still works for you?
> >
> 
> 
> Hi Dmitry,
> 
> I tested the patch on a CR-48 device (which is where I was seeing the
> problem because we switch to serio_raw at boot for the trackpad) and
> it does fix the problem.
> 
> Thanks for sending this, it had slipped off my todo list.
> 

Great, thank you for testing.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2011-02-02 22:58 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-22 23:09 [PATCH] Input: serio - clear serio event queue after sysfs driver rebind Duncan Laurie
2010-11-27  8:41 ` Dmitry Torokhov
2010-11-29 21:37   ` Duncan Laurie
2010-12-08  5:12     ` Dmitry Torokhov
2011-02-01  9:21       ` Dmitry Torokhov
2011-02-02 22:41         ` Duncan Laurie
2011-02-02 22:58           ` Dmitry Torokhov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).