On Wed, Dec 29, 2004 at 02:17:36AM -0500, Dmitry Torokhov wrote:
> Hi Vojtech,
>
> Please take a look at the following input patches. Patches 1-9 are already
> in my tree (and were there for quite some time so they should have gotten
> at least some testing as Andrew automatically pulls from me) and I'd like
> see them pushed to Linus together with your last batch. At least patches
> 6, 8 and 9 fix bugs introduced by latest changes. Patch 7 should help owners
> of Toshibas with Synaptics touchpads.
>
> 01-i8042-panicblink-cleanup.patch
> Move panicblink parameter definition together with the rest of i8042
> module parameters, add description and entry in kernel-parameters.txt
I think I prefer the DELAY definition to be outside the function. Other
than that the patch is OK.
> 02-serio-start-stop.patch
> Add serio start() and stop() methods to serio structure that are
> called when serio port is about to be fully registered and when
> serio port is about to be unregistered. These methods are useful
> for drivers that share interrupt among several serio ports. In this
> case interrupt can not be enabled/disabled in open/close methods
> and it is very hard to determine if interrupt shoudl be ignored or
> passed on.
> 03-i8042-use-start-stop.patch
> Make use of new serio start/stop methods to safely activate and
> deactivate ports. Also unify as much as possible port handling
> between KBD, AUX and MUX ports. Rename i8042_values into i8042_port.
Would we need this at all if we made the port registration completely
asynchronous, only binding devices to ports _after_ the port is
completely registered?
I'm rather reluctant to add even more callbacks.
> 04-serio-suppress-dup-events.patch
> Do not submit serio event into event queue if there is already one
> of the same type for the same port in front of it. Also look for
> duplicat events once event is processed. This should help with
> disconnected ports generating alot of data and consuming memory for
> events when kseriod gets behind and prevents constant rescans.
> This also allows to remove special check for 0xaa in reconnect path
> of interrupt handler known to cause problems with Toshibas keyboards.
Ok. Since we'll be usually scanning an empty list, this shouldn't add
any overhead.
Btw, why do we need _both_ to scan for duplicate events on event
completion and check at event insert time? One should be enough - if we
always check, then we cannot have duplicate events and if we always are
able to deal with them, we don't have to care ...
> 05-evdev-buffer-size.patch
> Return -EINVAL from evdev_read when passed buffer is too small.
> Based on patch by James Lamanna.
OK.
> 06-ps2pp-mouse-name.patch
> Set mouse name to "Mouse" instead of leaving it NULL when using
> PS2++ protocol and don't have any other information (Wheel, Touchpad)
> about the mouse.
Already merged.
> 07-synaptics-toshiba-rate.patch
> Toshiba Satellite KBCs have trouble handling data stream coming from
> Synaptics at full rate (80 pps, 480 byte/sec) which causes keyboards
> to pause or even get stuck. Use DMI to detect Satellites and limit
> rate to 40 pps which cures keyboard.
OK.
> 08-atkbd-keycode-size.patch
> Fix keycode table size initialization that got broken by my changes
> that exported 'set' and other settings via sysfs.
> setkeycodes should work again now.
Already merged.
> 09-i8042-sysfs-permissions.patch
> Fix braindamage in sysfs permissions for 'debug' option.
OK.
> 10-twidjoy-build.patch
> Make Kconfig and Makefile agree on the option name so twidjoy
> can be built.
OK.
> 11-input-msecs-to-jiffies.patch
> Use msecs_to_jiffies instead of homegrown ms_to_jiffies
> so everything works when HZ != 1000
OK.
> 12-atkbd-msecs-to-jiffies.patch
> Use msecs_to_jiffies instead of manually calculating
> delay for Toshiba bouncing keys workaround so it works
> when HZ != 1000.
OK.
> 13-serio-drvdata.patch
> Remove serio->private in favor of using driver-specific data
> in device structure, add serio_get_drvdata/serio_put_drvdata
> to access it so serio bus code looks more like other buses.
OK.
> 14-serio-id-match.patch
> Replace serio's type field with serio_id structure and
> add ids table to serio drivers. This will allow splitting
> initial matching and probing routines for better sysfs
> integration.
OK. Maybe we should add a new SPIOCSTYPE ioctl to pass the structure
directly.
> 15-serio-bus-cleanup.patch
> Make serio implementation more in line with standard
> driver model implementations by removing explicit device
> and driver list manipulations and introducing bus_match
> function. serio_register_port is always asynchronous to
> allow freely registering child ports. When deregistering
> serio core still takes care of destroying children ports
> first.
OK. I suppose the synchronous unregister variant is needed for module
unload? I suppose refcounting would be enough there ...
> 16-serio-connect-errcode.patch
> Make serios' connect methods return error code instead of
> void so exact cause of failur can be comminicated upstream.
OK.
--
Vojtech Pavlik
SuSE Labs, SuSE CR
On Thu, 13 Jan 2005 16:36:44 +0100, Vojtech Pavlik <[email protected]> wrote:
> On Wed, Dec 29, 2004 at 02:17:36AM -0500, Dmitry Torokhov wrote:
> >
> > 01-i8042-panicblink-cleanup.patch
> > Move panicblink parameter definition together with the rest of i8042
> > module parameters, add description and entry in kernel-parameters.txt
>
> I think I prefer the DELAY definition to be outside the function. Other
> than that the patch is OK.
Will do.
> > 02-serio-start-stop.patch
> > Add serio start() and stop() methods to serio structure that are
> > called when serio port is about to be fully registered and when
> > serio port is about to be unregistered. These methods are useful
> > for drivers that share interrupt among several serio ports. In this
> > case interrupt can not be enabled/disabled in open/close methods
> > and it is very hard to determine if interrupt shoudl be ignored or
> > passed on.
>
> > 03-i8042-use-start-stop.patch
> > Make use of new serio start/stop methods to safely activate and
> > deactivate ports. Also unify as much as possible port handling
> > between KBD, AUX and MUX ports. Rename i8042_values into i8042_port.
>
> Would we need this at all if we made the port registration completely
> asynchronous, only binding devices to ports _after_ the port is
> completely registered?
>
> I'm rather reluctant to add even more callbacks.
>
The problem is not with binding devices, it is with mostly with
sharing the same IRQ between different ports.
Look at the MUX case, you have 4 ports all sharing the same IRQ. At
startup i8042 enables all on-chip ports and registers them with
serio_register_port(). When first serio driver class i8042_open it
installs IRQ handler. All 4 ports are enabled now (because we want to
do hot-plug) but, especially with async. serio registration, only the
first is fully registered with serio subsystem. If data was to come
from any other port the shared IRQ handler will call serio_reconnect
on half-alive port. The similar issues happen when you try to
unregister port.
With start/stop methods allow serop core signal driver that the port
is fully registered and now is safe to use. If IRQ is not shared
between ports one can safely register/unregister handler in open/close
methods (at the expense of hotplug device support) but with shared IRQ
you need additional callbacks.
> > 04-serio-suppress-dup-events.patch
> > Do not submit serio event into event queue if there is already one
> > of the same type for the same port in front of it. Also look for
> > duplicat events once event is processed. This should help with
> > disconnected ports generating alot of data and consuming memory for
> > events when kseriod gets behind and prevents constant rescans.
> > This also allows to remove special check for 0xaa in reconnect path
> > of interrupt handler known to cause problems with Toshibas keyboards.
>
> Ok. Since we'll be usually scanning an empty list, this shouldn't add
> any overhead.
Plus serio events are rare so unless there are "runaway" serio interrupt this
patch tries to procet from there shoudl not be pretty much any overhead.
> Btw, why do we need _both_ to scan for duplicate events on event
> completion and check at event insert time? One should be enough - if we
> always check, then we cannot have duplicate events and if we always are
> able to deal with them, we don't have to care ...
A duplicate event can be queued while kseriod was processing current event,
checking for a dupe is cheaper then doing duplicate work we'd just done and
the lis is mostr likely empty anyway.
>
> Already merged.
>
> > 09-i8042-sysfs-permissions.patch
> > Fix braindamage in sysfs permissions for 'debug' option.
>
> OK.
This one has already been merged as well I think.
>
> > 14-serio-id-match.patch
> > Replace serio's type field with serio_id structure and
> > add ids table to serio drivers. This will allow splitting
> > initial matching and probing routines for better sysfs
> > integration.
>
> OK. Maybe we should add a new SPIOCSTYPE ioctl to pass the structure
> directly.
I am not sure it is needed ATM and I am a bit fuzzy on 32/64 bit ioctl
issues so I'll leave it alone for now ;)
>
> > 15-serio-bus-cleanup.patch
> > Make serio implementation more in line with standard
> > driver model implementations by removing explicit device
> > and driver list manipulations and introducing bus_match
> > function. serio_register_port is always asynchronous to
> > allow freely registering child ports. When deregistering
> > serio core still takes care of destroying children ports
> > first.
>
> OK. I suppose the synchronous unregister variant is needed for module
> unload? I suppose refcounting would be enough there ...
I don't see how. Kobject refcounting protects data structures but not
code and I can't bump up module's reference count in module_exit
function to delay its removal and we do not want to take module's
reference for every registered port otherwise unload will not be
possible. That's why synchronous removal method is needed.
--
Dmitry
On Thu, 13 Jan 2005 20:25:25 +0100, Vojtech Pavlik <[email protected]> wrote:
> On Thu, Jan 13, 2005 at 12:52:14PM -0500, Dmitry Torokhov wrote:
>
> > The problem is not with binding devices, it is with mostly with
> > sharing the same IRQ between different ports.
> >
> > Look at the MUX case, you have 4 ports all sharing the same IRQ. At
> > startup i8042 enables all on-chip ports and registers them with
> > serio_register_port(). When first serio driver class i8042_open it
> > installs IRQ handler. All 4 ports are enabled now (because we want to
> > do hot-plug) but, especially with async. serio registration, only the
> > first is fully registered with serio subsystem. If data was to come
> > from any other port the shared IRQ handler will call serio_reconnect
> > on half-alive port. The similar issues happen when you try to
> > unregister port.
> >
> > With start/stop methods allow serop core signal driver that the port
> > is fully registered and now is safe to use. If IRQ is not shared
> > between ports one can safely register/unregister handler in open/close
> > methods (at the expense of hotplug device support) but with shared IRQ
> > you need additional callbacks.
>
> Yes, this can result in calling serio_reconnect on a port that's not
> registered yet. But I believe we don't need a callback for this, just a
> flag in the serio struct that will say "live", be set during
> registration, and until that time all serio_interrupt() calls would be
> ignored?
>
You can't have this flag in serio struct because unless this flag is
set you should not access serio struct in question, this is chicken
and egg problem. While you can somewhat control creation the deletion
is especially tricky. You need to service interrupts up until the
close is called but not a moment later as serio structure will be
freed by serio core. Therefore flag should be in the driver and having
code poking in the driver guts is not a giood idea.
start/stop are not mandatory methods so only problem drivers needs to
be adjusted.
>
> > > > 04-serio-suppress-dup-events.patch
> > > > Do not submit serio event into event queue if there is already one
> > > > of the same type for the same port in front of it. Also look for
> > > > duplicat events once event is processed. This should help with
> > > > disconnected ports generating alot of data and consuming memory for
> > > > events when kseriod gets behind and prevents constant rescans.
> > > > This also allows to remove special check for 0xaa in reconnect path
> > > > of interrupt handler known to cause problems with Toshibas keyboards.
> > >
> > > Ok. Since we'll be usually scanning an empty list, this shouldn't add
> > > any overhead.
> >
> > Plus serio events are rare so unless there are "runaway" serio interrupt this
> > patch tries to procet from there shoudl not be pretty much any overhead.
>
> Indeed.
>
> > > Btw, why do we need _both_ to scan for duplicate events on event
> > > completion and check at event insert time? One should be enough - if we
> > > always check, then we cannot have duplicate events and if we always are
> > > able to deal with them, we don't have to care ...
> >
> > A duplicate event can be queued while kseriod was processing current event,
> > checking for a dupe is cheaper then doing duplicate work we'd just done and
> > the list is most likely empty anyway.
>
> Yes. So I'd probably drop the check when we're inserting the event?
>
No because (potentially) while kseriod is busy waiting on semaphore or
doing something else psmouse with synaptics touchpad can generate
rescan events at rate of 480 events/sec consuming memory for no good
reason.
>
> I think it could be done by marking the port dead, but still leaving it
> there when the module exits, but it's probably not worth it.
>
Then you would have to have 2 separate paths for hardware cleanup on
unload and for cleanup on driver disconnect...
--
Dmitry
On Thu, Jan 13, 2005 at 12:52:14PM -0500, Dmitry Torokhov wrote:
> The problem is not with binding devices, it is with mostly with
> sharing the same IRQ between different ports.
>
> Look at the MUX case, you have 4 ports all sharing the same IRQ. At
> startup i8042 enables all on-chip ports and registers them with
> serio_register_port(). When first serio driver class i8042_open it
> installs IRQ handler. All 4 ports are enabled now (because we want to
> do hot-plug) but, especially with async. serio registration, only the
> first is fully registered with serio subsystem. If data was to come
> from any other port the shared IRQ handler will call serio_reconnect
> on half-alive port. The similar issues happen when you try to
> unregister port.
>
> With start/stop methods allow serop core signal driver that the port
> is fully registered and now is safe to use. If IRQ is not shared
> between ports one can safely register/unregister handler in open/close
> methods (at the expense of hotplug device support) but with shared IRQ
> you need additional callbacks.
Yes, this can result in calling serio_reconnect on a port that's not
registered yet. But I believe we don't need a callback for this, just a
flag in the serio struct that will say "live", be set during
registration, and until that time all serio_interrupt() calls would be
ignored?
This is transparent to the port drivers, and doesn't change any
interfaces. What do you think?
> > > 04-serio-suppress-dup-events.patch
> > > Do not submit serio event into event queue if there is already one
> > > of the same type for the same port in front of it. Also look for
> > > duplicat events once event is processed. This should help with
> > > disconnected ports generating alot of data and consuming memory for
> > > events when kseriod gets behind and prevents constant rescans.
> > > This also allows to remove special check for 0xaa in reconnect path
> > > of interrupt handler known to cause problems with Toshibas keyboards.
> >
> > Ok. Since we'll be usually scanning an empty list, this shouldn't add
> > any overhead.
>
> Plus serio events are rare so unless there are "runaway" serio interrupt this
> patch tries to procet from there shoudl not be pretty much any overhead.
Indeed.
> > Btw, why do we need _both_ to scan for duplicate events on event
> > completion and check at event insert time? One should be enough - if we
> > always check, then we cannot have duplicate events and if we always are
> > able to deal with them, we don't have to care ...
>
> A duplicate event can be queued while kseriod was processing current event,
> checking for a dupe is cheaper then doing duplicate work we'd just done and
> the list is most likely empty anyway.
Yes. So I'd probably drop the check when we're inserting the event?
> > > 09-i8042-sysfs-permissions.patch
> > > Fix braindamage in sysfs permissions for 'debug' option.
> >
> > OK.
>
> This one has already been merged as well I think.
Probably yes.
> > > 14-serio-id-match.patch
> > > Replace serio's type field with serio_id structure and
> > > add ids table to serio drivers. This will allow splitting
> > > initial matching and probing routines for better sysfs
> > > integration.
> >
> > OK. Maybe we should add a new SPIOCSTYPE ioctl to pass the structure
> > directly.
>
> I am not sure it is needed ATM and I am a bit fuzzy on 32/64 bit ioctl
> issues so I'll leave it alone for now ;)
OK.
> > > 15-serio-bus-cleanup.patch
> > > Make serio implementation more in line with standard
> > > driver model implementations by removing explicit device
> > > and driver list manipulations and introducing bus_match
> > > function. serio_register_port is always asynchronous to
> > > allow freely registering child ports. When deregistering
> > > serio core still takes care of destroying children ports
> > > first.
> >
> > OK. I suppose the synchronous unregister variant is needed for module
> > unload? I suppose refcounting would be enough there ...
>
> I don't see how. Kobject refcounting protects data structures but not
> code and I can't bump up module's reference count in module_exit
> function to delay its removal and we do not want to take module's
> reference for every registered port otherwise unload will not be
> possible. That's why synchronous removal method is needed.
I think it could be done by marking the port dead, but still leaving it
there when the module exits, but it's probably not worth it.
--
Vojtech Pavlik
SuSE Labs, SuSE CR
Vojtech,
I have dropped the patches that have already been applied and
re-diffed the remaining patches. I have also merged Adrian's global ->
static cleanup and 2 patches from Prarit Bhargava (one re: releasing
resources acquired by i8042_platform_init if controller initialization
fails and the other is re: making educating guess whether controller
is absent or really times out).
There also was couple of additional changes - serio drivers now use
MODULE_DEVICE_TABLE to export information about the ports they can
possible work with; serio core exports port's type, protocol, id and
"extra" data as sysfs attributes and there is hotplug function to
signal userspace about new port. This all was done so that hotplug can
automatically load appropriate driver (for example psmouse) when a new
port is detected. I have patches for module-init-tools and hotplug
scripts that I will sent to Greg and Rusty as soon as you take the
pathes (if you will).
I think that the very first path ("while true; do xset led 3; xset
-led 3; done" makes keyboard miss release events and makes it
unusable) should go in 2.6.11 so please do:
bk pull bk://dtor.bkbits.net/for-2.6.11
That repository has only this change. The rest of the patches are in
my usual repository:
bk pull bk://dtor.bkbits.net/input
I am not sending the patches separately as they had been posted to the
lists couple of times already.
--
Dmitry
On Thu, Jan 27, 2005 at 09:04:06AM -0500, Dmitry Torokhov wrote:
> Vojtech,
>
> I have dropped the patches that have already been applied and
> re-diffed the remaining patches. I have also merged Adrian's global ->
> static cleanup and 2 patches from Prarit Bhargava (one re: releasing
> resources acquired by i8042_platform_init if controller initialization
> fails and the other is re: making educating guess whether controller
> is absent or really times out).
>
> There also was couple of additional changes - serio drivers now use
> MODULE_DEVICE_TABLE to export information about the ports they can
> possible work with; serio core exports port's type, protocol, id and
> "extra" data as sysfs attributes and there is hotplug function to
> signal userspace about new port. This all was done so that hotplug can
> automatically load appropriate driver (for example psmouse) when a new
> port is detected. I have patches for module-init-tools and hotplug
> scripts that I will sent to Greg and Rusty as soon as you take the
> pathes (if you will).
>
> I think that the very first path ("while true; do xset led 3; xset
> -led 3; done" makes keyboard miss release events and makes it
> unusable) should go in 2.6.11 so please do:
>
> bk pull bk://dtor.bkbits.net/for-2.6.11
Pulled, pushed into my tree. I verified the patch, and it is indeed
correct. Before we get an ACK for a command we sent, we still may
receive normal data. After we got the ACK we know for sure that no more
regular data will come, and can expect the command response.
> That repository has only this change. The rest of the patches are in
> my usual repository:
>
> bk pull bk://dtor.bkbits.net/input
>
> I am not sending the patches separately as they had been posted to the
> lists couple of times already.
OK. I'll go through them, and apply as appropriate. I still need to wrap
my mind around the start() and stop() methods and see the necessity. I
still think a variable in the serio struct, only accessed by the serio.c
core driver itself (and never by the port driver) that'd cause all
serio_interrupt() calls to be ignored until set in the asynchronous port
registration would be well enough.
--
Vojtech Pavlik
SuSE Labs, SuSE CR
On Thu, Jan 27, 2005 at 05:15:18PM +0100, Vojtech Pavlik wrote:
> OK. I'll go through them, and apply as appropriate. I still need to wrap
> my mind around the start() and stop() methods and see the necessity. I
> still think a variable in the serio struct, only accessed by the serio.c
> core driver itself (and never by the port driver) that'd cause all
> serio_interrupt() calls to be ignored until set in the asynchronous port
> registration would be well enough.
I've read he start() and stop() code, and I came to the conclusion
again that we don't need them as serio port driver methods. i8042 uses
them to set the exists variable only and uses that variable _solely_ for
the purpose of skipping calls to serio_interrupt(), serio_cleanup() and
serio_unregister().
By instead checking a member of the serio struct in these functions, and
doing nothing if not set, we achieve the same goal, without adding extra
cruft to the interface, making it allowed to call these serio functions
on a non-registered or half-registered serio struct, with the effect
being defined to nothing.
Similarly, it's perfectly allowed to call input_event() on an struct
input_dev that hasn't been registered with the input layer, again with
no effect. It simplifies the driver code a lot.
--
Vojtech Pavlik
SuSE Labs, SuSE CR
On Thu, 27 Jan 2005 17:36:05 +0100, Vojtech Pavlik <[email protected]> wrote:
> On Thu, Jan 27, 2005 at 05:15:18PM +0100, Vojtech Pavlik wrote:
>
> > OK. I'll go through them, and apply as appropriate. I still need to wrap
> > my mind around the start() and stop() methods and see the necessity. I
> > still think a variable in the serio struct, only accessed by the serio.c
> > core driver itself (and never by the port driver) that'd cause all
> > serio_interrupt() calls to be ignored until set in the asynchronous port
> > registration would be well enough.
>
> I've read he start() and stop() code, and I came to the conclusion
> again that we don't need them as serio port driver methods. i8042 uses
> them to set the exists variable only and uses that variable _solely_ for
> the purpose of skipping calls to serio_interrupt(), serio_cleanup() and
> serio_unregister().
>
> By instead checking a member of the serio struct in these functions, and
> doing nothing if not set, we achieve the same goal, without adding extra
> cruft to the interface, making it allowed to call these serio functions
> on a non-registered or half-registered serio struct, with the effect
> being defined to nothing.
>
No, you can not peek into serio structure from a driver, not when it
was dynamically allocated and can be gone at any time. Please consider
the following screnario when shutting down 8042 when you have a MUX
with several ports:
The rough call sequence is:
i8042_exit
serio_unregister_port
driver->disconnect
serio_close
i8042->close
free(serio)
We need to keep interrupts passed to serio core until serio_close is
completed so device properly ACKs/responds to cleanup commands.
Additionally, in i8042 close we do not free IRQ until last port is
unregistered nor we disable the port because we want to support
hotplugging. If interrupt comes after port was freed but before
serio_unregister_port has returned i8042_interrupt will call
serio_interrupt for port that was just free()ed. Special flag in serio
will not help because you need to know that port pointer is valid. You
could try pinning the port in memory buy taking a refernce but then
asynchronous unregister is not possible and it is needed in some
cases.
I think that having these 2 interface functions helps clearly define
these sequence points when port can/can not be used, simplifies logic
and alerts driver authors of this potential problem.
--
Dmitry
On Thursday 27 January 2005 11:15, Vojtech Pavlik wrote:
> > I think that the very first path ("while true; do xset led 3; xset
> > -led 3; done" makes keyboard miss release events and makes it
> > unusable) should go in 2.6.11 so please do:
> >
> > ? ? ? ? bk pull bk://dtor.bkbits.net/for-2.6.11
>
> Pulled, pushed into my tree. I verified the patch, and it is indeed
> correct. Before we get an ACK for a command we sent, we still may
> receive normal data. After we got the ACK we know for sure that no more
> regular data will come, and can expect the command response.
Hi Vojtech,
I have another one that I think needs to be in 2.6.11 - in ps2_command
does not update timeout variable when waiting for the first byte of
response so even if command times out the code still goes and tries to
wait for more data. It actually causes problems with GETID command - we
end up reporting success even if we did not get any response (except for
initial ACK).
Also, now taht wait_event_timeout is available we shoudl use it instead
of wait_event_interruptible_timeout.
--
Dmitry
===================================================================
[email protected], 2005-01-28 01:53:11-05:00, [email protected]
Input: libps2 - fix timeout handling in ps2_command, switch to using
wait_event_timeout instead of wait_event_interruptible_timeout
now that first form is available.
Signed-off-by: Dmitry Torokhov <[email protected]>
libps2.c | 22 +++++++++++-----------
1 files changed, 11 insertions(+), 11 deletions(-)
===================================================================
diff -Nru a/drivers/input/serio/libps2.c b/drivers/input/serio/libps2.c
--- a/drivers/input/serio/libps2.c 2005-01-28 02:12:06 -05:00
+++ b/drivers/input/serio/libps2.c 2005-01-28 02:12:06 -05:00
@@ -60,9 +60,9 @@
serio_continue_rx(ps2dev->serio);
if (serio_write(ps2dev->serio, byte) == 0)
- wait_event_interruptible_timeout(ps2dev->wait,
- !(ps2dev->flags & PS2_FLAG_ACK),
- msecs_to_jiffies(timeout));
+ wait_event_timeout(ps2dev->wait,
+ !(ps2dev->flags & PS2_FLAG_ACK),
+ msecs_to_jiffies(timeout));
serio_pause_rx(ps2dev->serio);
ps2dev->flags &= ~PS2_FLAG_ACK;
@@ -115,8 +115,8 @@
*/
timeout = msecs_to_jiffies(command == PS2_CMD_RESET_BAT ? 4000 : 500);
- wait_event_interruptible_timeout(ps2dev->wait,
- !(ps2dev->flags & PS2_FLAG_CMD1), timeout);
+ timeout = wait_event_timeout(ps2dev->wait,
+ !(ps2dev->flags & PS2_FLAG_CMD1), timeout);
if (ps2dev->cmdcnt && timeout > 0) {
@@ -147,8 +147,8 @@
serio_continue_rx(ps2dev->serio);
}
- wait_event_interruptible_timeout(ps2dev->wait,
- !(ps2dev->flags & PS2_FLAG_CMD), timeout);
+ wait_event_timeout(ps2dev->wait,
+ !(ps2dev->flags & PS2_FLAG_CMD), timeout);
}
if (param)
@@ -259,7 +259,7 @@
ps2dev->flags |= PS2_FLAG_CMD | PS2_FLAG_CMD1;
ps2dev->flags &= ~PS2_FLAG_ACK;
- wake_up_interruptible(&ps2dev->wait);
+ wake_up(&ps2dev->wait);
if (data != PS2_RET_ACK)
ps2_handle_response(ps2dev, data);
@@ -281,12 +281,12 @@
if (ps2dev->flags & PS2_FLAG_CMD1) {
ps2dev->flags &= ~PS2_FLAG_CMD1;
if (ps2dev->cmdcnt)
- wake_up_interruptible(&ps2dev->wait);
+ wake_up(&ps2dev->wait);
}
if (!ps2dev->cmdcnt) {
ps2dev->flags &= ~PS2_FLAG_CMD;
- wake_up_interruptible(&ps2dev->wait);
+ wake_up(&ps2dev->wait);
}
return 1;
@@ -298,7 +298,7 @@
ps2dev->nak = 1;
if (ps2dev->flags & (PS2_FLAG_ACK | PS2_FLAG_CMD))
- wake_up_interruptible(&ps2dev->wait);
+ wake_up(&ps2dev->wait);
ps2dev->flags = 0;
}
On Fri, Jan 28, 2005 at 02:13:12AM -0500, Dmitry Torokhov wrote:
> On Thursday 27 January 2005 11:15, Vojtech Pavlik wrote:
> > > I think that the very first path ("while true; do xset led 3; xset
> > > -led 3; done" makes keyboard miss release events and makes it
> > > unusable) should go in 2.6.11 so please do:
> > >
> > > ? ? ? ? bk pull bk://dtor.bkbits.net/for-2.6.11
> >
> > Pulled, pushed into my tree. I verified the patch, and it is indeed
> > correct. Before we get an ACK for a command we sent, we still may
> > receive normal data. After we got the ACK we know for sure that no more
> > regular data will come, and can expect the command response.
>
> Hi Vojtech,
>
> I have another one that I think needs to be in 2.6.11 - in ps2_command
> does not update timeout variable when waiting for the first byte of
> response so even if command times out the code still goes and tries to
> wait for more data. It actually causes problems with GETID command - we
> end up reporting success even if we did not get any response (except for
> initial ACK).
>
> Also, now taht wait_event_timeout is available we shoudl use it instead
> of wait_event_interruptible_timeout.
OK.
--
Vojtech Pavlik
SuSE Labs, SuSE CR
On Thu, Jan 27, 2005 at 01:18:55PM -0500, Dmitry Torokhov wrote:
> On Thu, 27 Jan 2005 17:36:05 +0100, Vojtech Pavlik <[email protected]> wrote:
> > On Thu, Jan 27, 2005 at 05:15:18PM +0100, Vojtech Pavlik wrote:
> >
> > > OK. I'll go through them, and apply as appropriate. I still need to wrap
> > > my mind around the start() and stop() methods and see the necessity. I
> > > still think a variable in the serio struct, only accessed by the serio.c
> > > core driver itself (and never by the port driver) that'd cause all
> > > serio_interrupt() calls to be ignored until set in the asynchronous port
> > > registration would be well enough.
> >
> > I've read he start() and stop() code, and I came to the conclusion
> > again that we don't need them as serio port driver methods. i8042 uses
> > them to set the exists variable only and uses that variable _solely_ for
> > the purpose of skipping calls to serio_interrupt(), serio_cleanup() and
> > serio_unregister().
> >
> > By instead checking a member of the serio struct in these functions, and
> > doing nothing if not set, we achieve the same goal, without adding extra
> > cruft to the interface, making it allowed to call these serio functions
> > on a non-registered or half-registered serio struct, with the effect
> > being defined to nothing.
> >
>
> No, you can not peek into serio structure from a driver, not when it
> was dynamically allocated and can be gone at any time. Please consider
> the following screnario when shutting down 8042 when you have a MUX
> with several ports:
>
> The rough call sequence is:
> i8042_exit
> serio_unregister_port
> driver->disconnect
> serio_close
> i8042->close
> free(serio)
>
> We need to keep interrupts passed to serio core until serio_close is
> completed so device properly ACKs/responds to cleanup commands.
> Additionally, in i8042 close we do not free IRQ until last port is
> unregistered nor we disable the port because we want to support
> hotplugging. If interrupt comes after port was freed but before
> serio_unregister_port has returned i8042_interrupt will call
> serio_interrupt for port that was just free()ed. Special flag in serio
> will not help because you need to know that port pointer is valid. You
> could try pinning the port in memory buy taking a refernce but then
> asynchronous unregister is not possible and it is needed in some
> cases.
>
> I think that having these 2 interface functions helps clearly define
> these sequence points when port can/can not be used, simplifies logic
> and alerts driver authors of this potential problem.
You're right. I forgot that serio isn't anymore tied to the driver and
can cease to exist on its own asynchronously. However, I'm still not
sure whether it's worth it. We might as well simply drop the unregister
call in i8042_open for AUX completely and forget about asynchronous
unregisters and use normal refcounting. As far as grep knows, it's the
only user.
--
Vojtech Pavlik
SuSE Labs, SuSE CR
On Thursday 27 January 2005 17:16, Vojtech Pavlik wrote:
> On Thu, Jan 27, 2005 at 01:18:55PM -0500, Dmitry Torokhov wrote:
> > On Thu, 27 Jan 2005 17:36:05 +0100, Vojtech Pavlik <[email protected]> wrote:
> > > On Thu, Jan 27, 2005 at 05:15:18PM +0100, Vojtech Pavlik wrote:
> > >
> > > > OK. I'll go through them, and apply as appropriate. I still need to wrap
> > > > my mind around the start() and stop() methods and see the necessity. I
> > > > still think a variable in the serio struct, only accessed by the serio.c
> > > > core driver itself (and never by the port driver) that'd cause all
> > > > serio_interrupt() calls to be ignored until set in the asynchronous port
> > > > registration would be well enough.
> > >
> > > I've read he start() and stop() code, and I came to the conclusion
> > > again that we don't need them as serio port driver methods. i8042 uses
> > > them to set the exists variable only and uses that variable _solely_ for
> > > the purpose of skipping calls to serio_interrupt(), serio_cleanup() and
> > > serio_unregister().
> > >
> > > By instead checking a member of the serio struct in these functions, and
> > > doing nothing if not set, we achieve the same goal, without adding extra
> > > cruft to the interface, making it allowed to call these serio functions
> > > on a non-registered or half-registered serio struct, with the effect
> > > being defined to nothing.
> > >
> >
> > No, you can not peek into serio structure from a driver, not when it
> > was dynamically allocated and can be gone at any time. Please consider
> > the following screnario when shutting down 8042 when you have a MUX
> > with several ports:
> >
> > The rough call sequence is:
> > i8042_exit
> > serio_unregister_port
> > driver->disconnect
> > serio_close
> > i8042->close
> > free(serio)
> >
> > We need to keep interrupts passed to serio core until serio_close is
> > completed so device properly ACKs/responds to cleanup commands.
> > Additionally, in i8042 close we do not free IRQ until last port is
> > unregistered nor we disable the port because we want to support
> > hotplugging. If interrupt comes after port was freed but before
> > serio_unregister_port has returned i8042_interrupt will call
> > serio_interrupt for port that was just free()ed. Special flag in serio
> > will not help because you need to know that port pointer is valid. You
> > could try pinning the port in memory buy taking a refernce but then
> > asynchronous unregister is not possible and it is needed in some
> > cases.
> >
> > I think that having these 2 interface functions helps clearly define
> > these sequence points when port can/can not be used, simplifies logic
> > and alerts driver authors of this potential problem.
>
> You're right. I forgot that serio isn't anymore tied to the driver and
> can cease to exist on its own asynchronously. However, I'm still not
> sure whether it's worth it. We might as well simply drop the unregister
> call in i8042_open for AUX completely and forget about asynchronous
> unregisters and use normal refcounting. As far as grep knows, it's the
> only user.
I am pretty sure I will need asynchronous unregister in some form when
I finish dynamic protocol switching in psmouse (those darned pass-through
ports!). Plus again, having these 2 methods will draw driver writers'
attention to the existence of this particular problem.
--
Dmitry
On Sun, Jan 30, 2005 at 06:35:18PM -0500, Dmitry Torokhov wrote:
> > You're right. I forgot that serio isn't anymore tied to the driver and
> > can cease to exist on its own asynchronously. However, I'm still not
> > sure whether it's worth it. We might as well simply drop the unregister
> > call in i8042_open for AUX completely and forget about asynchronous
> > unregisters and use normal refcounting. As far as grep knows, it's the
> > only user.
>
> I am pretty sure I will need asynchronous unregister in some form when
> I finish dynamic protocol switching in psmouse (those darned pass-through
> ports!). Plus again, having these 2 methods will draw driver writers'
> attention to the existence of this particular problem.
OK. I'm convinced.
--
Vojtech Pavlik
SuSE Labs, SuSE CR