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)