linuxconsole-dev

Role of close() in struct input_dev

[Johann D. <jo...@Do...>]

Hi !

I am trying to clean up my addition about multiple open-with-write-access.
My first idea was to use a flag, only_one_writer in struct evdev, and then
perform the necessay checks in evdev_open and evdev_release.

Vojtech suggested that we add a parameter to input_dev::open and pass
that parameter to device drivers and let them decide. This would indeed
allow for more flexibility.
I thought that input_dev::close should be modified the same way.

I tried to implement this, and while doing so, I noticed that it could be
that input_dev::open and input_dev::close are not called the same number
of times. This may or may not be a problem, I don't know.

Here is a sample scenario:
1. Application 1 opens /dev/input/event0
2. Application 2 opens /dev/input/event0 (nothing forbids that)
3. evdev_disconnect is called (usb device is unplugged, input_attach is
kiled...)

Steps 1 and 2 call input_dev::open, step 3 calls input_dev::close.

Now the question is: What is the role of input_dev::close/open ?
Are they meant as "init/deinit the device" or "establish/break connection
with processes" (or a mix of both) ?

I would vote for the second possibility.

By the way, if you add a step 4 to my scenario:
4. Application 1 or 2 writes to /dev/input/event0
then the application segfaults, and dmesg tells me someone tried to
dereference a NULL pointer.

-- 
Johann Deneux
CS student at DoCS (www.docs.uu.se/~johannd) and
ESIL (www.esil.univ-mrs.fr/~jdeneux)

Re: Role of close() in struct input_dev

[James S. <jsi...@tr...>]

> Now the question is: What is the role of input_dev::close/open ?
> Are they meant as "init/deinit the device" or "establish/break connection
> with processes" (or a mix of both) ?

If you look at input.c connect and disconnect are used by input_register
and input_unregister which are called when you insmod the device or at
boot time. So they are used to setup the hardware and the data structs for
the device. Now open and close are called input_open_device and
input_close_device. Now if look at what calls input_[opne/close]_device
they are all some kind of userland interface i.e evdev, mousedev, joydev.c
etc. 

P.S

   Lets coordinate our work. I'm working on power management right now
which also changes things like input.c. I have pretty much figured out pm
handling per device. The problem is general powering down a device. How to
handle things like a power power that suspends a PDA. This is much
trickier.

Re: Role of close() in struct input_dev

[Johann D. <jo...@Do...>]

On Tue, 19 Jun 2001, James Simmons wrote:

> 
> > Now the question is: What is the role of input_dev::close/open ?
> > Are they meant as "init/deinit the device" or "establish/break connection
> > with processes" (or a mix of both) ?
> 
> If you look at input.c connect and disconnect are used by input_register
> and input_unregister which are called when you insmod the device or at
> boot time. So they are used to setup the hardware and the data structs for
> the device. Now open and close are called input_open_device and
> input_close_device. Now if look at what calls input_[opne/close]_device
> they are all some kind of userland interface i.e evdev, mousedev, joydev.c
> etc. 

There are parts of evdev functions called by userland, and other parts
called by other modules. Namely, evdev_disconnect can be called by serio,
for example.
The problem is still there: I would expect
input_open_device/input_close_devise to be called at least the same
number of times (perhaps we should even take care of the order).

> 
> P.S
> 
>    Lets coordinate our work. I'm working on power management right now
> which also changes things like input.c. I have pretty much figured out pm
> handling per device. The problem is general powering down a device. How to
> handle things like a power power that suspends a PDA. This is much
> trickier.
> 
Ok, I will play with the files and try to figure out everything
locally. We should not have sync problem, but I will wait for you to be
finished before commiting anything.

-- 
Johann Deneux
CS student at DoCS (www.docs.uu.se/~johannd) and
ESIL (www.esil.univ-mrs.fr/~jdeneux)

Re: Role of close() in struct input_dev

[James S. <jsi...@tr...>]

> There are parts of evdev functions called by userland, and other parts
> called by other modules. Namely, evdev_disconnect can be called by serio,
> for example.
> The problem is still there: I would expect
> input_open_device/input_close_devise to be called at least the same
> number of times (perhaps we should even take care of the order).

Both would be called the same number of time. Well I hope. The order is
what matters. 

> > P.S
[snip]...
> > 
> Ok, I will play with the files and try to figure out everything
> locally. We should not have sync problem, but I will wait for you to be
> finished before commiting anything.

I have to do some testing on the ipaq first. I pretty much hammered the
power down each device. Will send a seperate email on this:

Re: Role of close() in struct input_dev

[Johann D. <jo...@Do...>]

On Tue, 19 Jun 2001, James Simmons wrote:

> 
> > There are parts of evdev functions called by userland, and other parts
> > called by other modules. Namely, evdev_disconnect can be called by serio,
> > for example.
> > The problem is still there: I would expect
> > input_open_device/input_close_devise to be called at least the same
> > number of times (perhaps we should even take care of the order).
> 
> Both would be called the same number of time. Well I hope. The order is
> what matters. 

After further checks, yes they actually are called the same number of
times.
In fact, I realised the driver doesn't really know when a process does an
open(). Indeed, input_open_device is called only when the first process
opens the device, and input_close_device when all processes closed it.
I would however need a function to be called every time a process accesses
an i-force device.
Two solutions:

- I modify the existing input_{open,close}_device
Frankly, I don't really like the fact that evdev can be kfreed both by
evdev_disconnect and evdev_connect. Furthermore, the choice is done after
the value of evdev->exist, whose access is not protected (possible bug if
the last process releases the device while the device is being
disconnected, on a smp machine (weird case, though))

- I add new callbacks to input_dev
I need to make modifications to this struct, and having two different kind
of open functions may be confusing. If driver writers need to make a
distinction between the first open(), and subsequent open(), they can
handle it themselves.



Anyway, it seems we have to check evdev->exist before doing anything in
evdev_{ioctl,read,write...}, I guess this is what caused the oops I got.


-- 
Johann Deneux
CS student at DoCS (www.docs.uu.se/~johannd) and
ESIL (www.esil.univ-mrs.fr/~jdeneux)

Re: Role of close() in struct input_dev

[James S. <jsi...@tr...>]

> In fact, I realised the driver doesn't really know when a process does an
> open(). Indeed, input_open_device is called only when the first process
> opens the device, and input_close_device when all processes closed it.

Are you sure about that? For one thing each time you open /dev/mouse and
/dev/event both devices call input_open_device. This is a case where the
same hardware is opened twice. Also in input_close_device we have 

handle->open--;

I'm actually using this for power management.

> Frankly, I don't really like the fact that evdev can be kfreed both by
> evdev_disconnect and evdev_connect. Furthermore, the choice is done after
> the value of evdev->exist, whose access is not protected (possible bug if
> the last process releases the device while the device is being
> disconnected, on a smp machine (weird case, though))

I haven't thought about that problem but I have noticed the handle list
isn't protected with a lock. On a SMP machine you the list being scanned
and another thread removing or adding a new handle. 

> Anyway, it seems we have to check evdev->exist before doing anything in
> evdev_{ioctl,read,write...}, I guess this is what caused the oops I got.

Please post the result of oops processed by ksymoops. This will help us
find the problem.

Re: Role of close() in struct input_dev

[Johann D. <jo...@Do...>]

On Tue, 19 Jun 2001, James Simmons wrote:

> 
> > In fact, I realised the driver doesn't really know when a process does an
> > open(). Indeed, input_open_device is called only when the first process
> > opens the device, and input_close_device when all processes closed it.
> 
> Are you sure about that? For one thing each time you open /dev/mouse and
> /dev/event both devices call input_open_device. This is a case where the
> same hardware is opened twice. [...]

This is the case because from evdev and mousedev, the device is opened
only once. What I meant was in the case where two processes or more 
perform an open one after another. In that case, evdev and mousedev will
call input_open_device only for the first process.
Same thing for input_close_device. It's called only when evdev->open falls
to zero (unless the device is disconnected).

> 
> I'm actually using this for power management.
> 

I never took time to think about pm. My experience in this domain is close
to zero (Especially as pm allways had the bad habit to have strange
effects on smp machine)

> > Frankly, I don't really like the fact that evdev can be kfreed both by
> > evdev_disconnect and evdev_connect. Furthermore, the choice is done after
> > the value of evdev->exist, whose access is not protected (possible bug if
> > the last process releases the device while the device is being
> > disconnected, on a smp machine (weird case, though))
> 
> I haven't thought about that problem but I have noticed the handle list
> isn't protected with a lock. On a SMP machine you the list being scanned
> and another thread removing or adding a new handle.

Indeed.
 
> 
> > Anyway, it seems we have to check evdev->exist before doing anything in
> > evdev_{ioctl,read,write...}, I guess this is what caused the oops I got.
> 
> Please post the result of oops processed by ksymoops. This will help us
> find the problem.
> 
No need for that, I know where this problem comes from. evdev calls
functions from, say iforce (or any other driver), without checking if the
driver is still loaded and running. There are cases when this is not true
(if you disconnect the device while it's being used by a process). The
memory allocated for the input_dev structure has then been kfreed, and
should not be accesses. By luck (or is it done in purpose in kfree ?), the
memory area was cleaned with zeros, and the bug showed up immediately,
causing a null dereferecing.
Simply testing the value of evdev->exist at the beginning of each
userland-reachable function would solve this problem.

I made some modifications yesterday night to evdev.c. You can find them at
http://www.esil.univ-mrs.fr/~jdeneux/projects/ff/ruby-evdev-changes.tar.gz

There is still a bug in these modifications, though (input_close_device is
called too many times), so I am not happy with it. It's not ready, but
perhaps it can help you see what I would expect from these open/close
functions.
Among the changes, note:
- evdev->exist moved to list->exist, list being a member of the 
list evdev->list
- new parameter file to input_{open,close}_device and corresponding
evdev::{open/close}. I may need this one.
- only evdev_release kfrees memory kallocated in evdev_open
- if (--evdev->open) tests disappeared. Actually, evdev->open disappeared
(double use with handle->open in input ?)

About pm, I guess it could be interesting to store the state of the
device: active, suspended, shutdown, waking up, ...
Two other states could be: driver_loaded_and_ready,
driver_disconnected_from_process
The state of a process could be: process_ready, process_gone

The evdev->list element created in evdev_open would be freed only once we
have driver_disconnected_from_process and process_gone

-- 
Johann Deneux
CS student at DoCS (www.docs.uu.se/~johannd) and
ESIL (www.esil.univ-mrs.fr/~jdeneux)

Re: Role of close() in struct input_dev

[Johann D. <jo...@Do...>]

On Wed, 20 Jun 2001, Johann Deneux wrote:

> On Tue, 19 Jun 2001, James Simmons wrote:
> 
> > 
> > > In fact, I realised the driver doesn't really know when a process does an
> > > open(). Indeed, input_open_device is called only when the first process
> > > opens the device, and input_close_device when all processes closed it.
> > 
> > Are you sure about that? For one thing each time you open /dev/mouse and
> > /dev/event both devices call input_open_device. This is a case where the
> > same hardware is opened twice. [...]
> 
> This is the case because from evdev and mousedev, the device is opened
> only once. What I meant was in the case where two processes or more 
> perform an open one after another. In that case, evdev and mousedev will
> call input_open_device only for the first process.
> Same thing for input_close_device. It's called only when evdev->open falls
> to zero (unless the device is disconnected).

Which is, I have just understood that, the wanted behavior. I mistook
release and flush. I added a new input_dev::flush() and 
input_flush_device() functions to do what I need.
Still, evdev_open can be called several times by a process. evdev_release
is called only once, when the last close on the device file is done
(according to "Linux 2.4 Internals"). Some list elements could then be
left unfreed, thus causing a memory leak. Or did I miss something ???

I also added a input_dev::accept() and input_accept_device() functions
(hmm, any idea about a better name ? the name misleads the reader into
thinking it's the device that gets accepted)
The goal of this function is to decide whether an open() call should be
accepted or not. I use this to prevent several processes from using
force-feedback at the same time.

-- 
Johann Deneux
CS student at DoCS (www.docs.uu.se/~johannd) and
ESIL (www.esil.univ-mrs.fr/~jdeneux)

Re: Role of close() in struct input_dev

[James S. <jsi...@tr...>]

> Which is, I have just understood that, the wanted behavior. I mistook
> release and flush. I added a new input_dev::flush() and 
> input_flush_device() functions to do what I need.

Flush? This is new. How does a userland take advantage and does this
function do what I think it does, flush all data stored for a file?

> Still, evdev_open can be called several times by a process. evdev_release
> is called only once, when the last close on the device file is done
> (according to "Linux 2.4 Internals"). Some list elements could then be
> left unfreed, thus causing a memory leak. Or did I miss something ???

Only if it does free all the list elements for this particular device when
close is called.
 
> I also added a input_dev::accept() and input_accept_device() functions
> (hmm, any idea about a better name ? the name misleads the reader into
> thinking it's the device that gets accepted)

Yeah it is misleading. 

> The goal of this function is to decide whether an open() call should be
> accepted or not. I use this to prevent several processes from using
> force-feedback at the same time.

I suggest you add comments about this that can be used for the docs.

Re: Role of close() in struct input_dev

[Johann D. <jo...@Do...>]

On Wed, 20 Jun 2001, James Simmons wrote:

> 
> > Which is, I have just understood that, the wanted behavior. I mistook
> > release and flush. I added a new input_dev::flush() and 
> > input_flush_device() functions to do what I need.
> 
> Flush? This is new. How does a userland take advantage and does this
> function do what I think it does, flush all data stored for a file?

It's called by close(). And so is input_dev::release(). So what's the
difference, you may ask ?
flush() is called at every close(), release() only on the last
close(). That's what the doc says, which leaves a few questions open. I
will play with little programs to check how it exactly behaves.

> 
> > Still, evdev_open can be called several times by a process. evdev_release
> > is called only once, when the last close on the device file is done
> > (according to "Linux 2.4 Internals"). Some list elements could then be
> > left unfreed, thus causing a memory leak. Or did I miss something ???
> 
> Only if it does free all the list elements for this particular device when
> close is called.
>  

We could use evdev_flush() to individually free every list element as it's
closed, and evdev_close() to free all the remaining ones (which should
not exist, btw, as they should have been freed by a evdev_flush())

> > I also added a input_dev::accept() and input_accept_device() functions
> > (hmm, any idea about a better name ? the name misleads the reader into
> > thinking it's the device that gets accepted)
> 
> Yeah it is misleading.

accept_process() would be better. input_accept_process() would be nice,
too, but it brakes the input_{something}_device "rule"

-- 
Johann Deneux
CS student at DoCS (www.docs.uu.se/~johannd) and
ESIL (www.esil.univ-mrs.fr/~jdeneux)

Re: Role of close() in struct input_dev

[Vojtech P. <vo...@su...>]

On Tue, Jun 19, 2001 at 05:39:45PM -0700, James Simmons wrote:
> 
> > In fact, I realised the driver doesn't really know when a process does an
> > open(). Indeed, input_open_device is called only when the first process
> > opens the device, and input_close_device when all processes closed it.
> 
> Are you sure about that? For one thing each time you open /dev/mouse and
> /dev/event both devices call input_open_device. This is a case where the
> same hardware is opened twice. Also in input_close_device we have 

But it's a per-driver open, not per-process open.
 
> handle->open--;
> 
> I'm actually using this for power management.
> 
> > Frankly, I don't really like the fact that evdev can be kfreed both by
> > evdev_disconnect and evdev_connect. Furthermore, the choice is done after
> > the value of evdev->exist, whose access is not protected (possible bug if
> > the last process releases the device while the device is being
> > disconnected, on a smp machine (weird case, though))
> 
> I haven't thought about that problem but I have noticed the handle list
> isn't protected with a lock. On a SMP machine you the list being scanned
> and another thread removing or adding a new handle. 

Yep. It ain't SMP safe.

> > Anyway, it seems we have to check evdev->exist before doing anything in
> > evdev_{ioctl,read,write...}, I guess this is what caused the oops I got.
> 
> Please post the result of oops processed by ksymoops. This will help us
> find the problem.

-- 
Vojtech Pavlik
SuSE Labs

Re: Role of close() in struct input_dev

[Vojtech P. <vo...@su...>]

On Tue, Jun 19, 2001 at 11:41:14PM +0200, Johann Deneux wrote:
> On Tue, 19 Jun 2001, James Simmons wrote:
> 
> > 
> > > There are parts of evdev functions called by userland, and other parts
> > > called by other modules. Namely, evdev_disconnect can be called by serio,
> > > for example.
> > > The problem is still there: I would expect
> > > input_open_device/input_close_devise to be called at least the same
> > > number of times (perhaps we should even take care of the order).
> > 
> > Both would be called the same number of time. Well I hope. The order is
> > what matters. 
> 
> After further checks, yes they actually are called the same number of
> times.
> In fact, I realised the driver doesn't really know when a process does an
> open(). Indeed, input_open_device is called only when the first process
> opens the device, and input_close_device when all processes closed it.
> I would however need a function to be called every time a process accesses
> an i-force device.
> Two solutions:
> 
> - I modify the existing input_{open,close}_device
> Frankly, I don't really like the fact that evdev can be kfreed both by
> evdev_disconnect and evdev_connect. Furthermore, the choice is done after

You meant evdev_disconnect and evdev_release, right?

> the value of evdev->exist, whose access is not protected (possible bug if
> the last process releases the device while the device is being
> disconnected, on a smp machine (weird case, though))

Yes, it's most likely not completely SMP safe. It's simple, though - as
long as the device is connetced *or* is opened, we need struct evdev.

> - I add new callbacks to input_dev
> I need to make modifications to this struct, and having two different kind
> of open functions may be confusing. If driver writers need to make a
> distinction between the first open(), and subsequent open(), they can
> handle it themselves.
> 
> Anyway, it seems we have to check evdev->exist before doing anything in
> evdev_{ioctl,read,write...}, I guess this is what caused the oops I got.

Most likely, yes. read() should be fine, though, as it doesn't do
anything with the device.

-- 
Vojtech Pavlik
SuSE Labs

Re: Role of close() in struct input_dev

[Vojtech P. <vo...@su...>]

On Tue, Jun 19, 2001 at 10:42:27PM +0200, Johann Deneux wrote:
> On Tue, 19 Jun 2001, James Simmons wrote:
> 
> > 
> > > Now the question is: What is the role of input_dev::close/open ?
> > > Are they meant as "init/deinit the device" or "establish/break connection
> > > with processes" (or a mix of both) ?
> > 
> > If you look at input.c connect and disconnect are used by input_register
> > and input_unregister which are called when you insmod the device or at
> > boot time. So they are used to setup the hardware and the data structs for
> > the device. Now open and close are called input_open_device and
> > input_close_device. Now if look at what calls input_[opne/close]_device
> > they are all some kind of userland interface i.e evdev, mousedev, joydev.c
> > etc. 
> 
> There are parts of evdev functions called by userland, and other parts
> called by other modules. Namely, evdev_disconnect can be called by serio,
> for example.

No, evdev_disconnect will always be called by input.c, which will always
be called by some input device driver calling the unregister() function.

It can be triggered by serio/usb/rmmod/whatever else, though.

> The problem is still there: I would expect
> input_open_device/input_close_devise to be called at least the same
> number of times (perhaps we should even take care of the order).

We can't take care of the order - that is not possible. But they indeed
should be called the same number of times. If they are not, it's a bug.

-- 
Vojtech Pavlik
SuSE Labs

Re: Role of close() in struct input_dev

[Vojtech P. <vo...@su...>]

On Tue, Jun 19, 2001 at 09:01:30PM +0200, Johann Deneux wrote:
> Hi !
> 
> I am trying to clean up my addition about multiple open-with-write-access.
> My first idea was to use a flag, only_one_writer in struct evdev, and then
> perform the necessay checks in evdev_open and evdev_release.
> 
> Vojtech suggested that we add a parameter to input_dev::open and pass
> that parameter to device drivers and let them decide. This would indeed
> allow for more flexibility.
> I thought that input_dev::close should be modified the same way.
> 
> I tried to implement this, and while doing so, I noticed that it could be
> that input_dev::open and input_dev::close are not called the same number
> of times. This may or may not be a problem, I don't know.
> 
> Here is a sample scenario:
> 1. Application 1 opens /dev/input/event0
> 2. Application 2 opens /dev/input/event0 (nothing forbids that)
> 3. evdev_disconnect is called (usb device is unplugged, input_attach is
> kiled...)
> 
> Steps 1 and 2 call input_dev::open, step 3 calls input_dev::close.

No. As it is now (or was the last time I remember it), step 2 doesn't
call input_dev->open.

> Now the question is: What is the role of input_dev::close/open ?
> Are they meant as "init/deinit the device" or "establish/break connection
> with processes" (or a mix of both) ?

Originally it was intended to init/deinit the device so that it can
have the USB IRQ running only when the device is used, same with
joysticks, which eat a considerable amount of CPU while polling.

> I would vote for the second possibility.

It's beginning to sound more reasonable, though I think there will be
problems arising with it.

> By the way, if you add a step 4 to my scenario:
> 4. Application 1 or 2 writes to /dev/input/event0
> then the application segfaults, and dmesg tells me someone tried to
> dereference a NULL pointer.

Interesting, gotta try ...

-- 
Vojtech Pavlik
SuSE Labs