Re: [patch] add input_enable_device()

Re: [patch] add input_enable_device()

[Stas Sergeev]

Hello.

Dmitry Torokhov wrote:
> I really believe that instead of shoving this into input core you need to
> split pcspkr driver to allow concurrent access to the hardware.
I split pcspkr and someone else will say that there is
already enough of the midlayers to handle the like things,
to not introduce another one just for the particular driver.
Besides, I don't beleive people will be happy with having
2 modules for just handling the terminal beeps.
The input midlayer looks like the best solution. It allows
to deal with the modules as soon as they are loaded. It has
enough of the information needed to precisely identify the
module (I now use INPUT_DEVICE_ID_MATCH_BUS). The pcspkr *is*
an "input driver" after all, so why not to deal with it via
an input API? If the input should not be used for anything
related to the port IO, then why it carries the information
about the ports and the bus that are used by the device?
Why does it have the INPUT_DEVICE_ID_MATCH_BUS after all?
The input API only lacks a very small piece of the functionality -
disabling the device, which can easily be used by anything else
in the future. Is there a reason not to include that functionality
only because the snd-pcsp is going to use it, or is there any *other*
reason?

Re: [patch] add input_enable_device()

[Dmitry Torokhov]

--- Stas Sergeev <stsp@aknet.ru> wrote:

> Hello.
> 
> Dmitry Torokhov wrote:
> > I really believe that instead of shoving this into input core you need to
> > split pcspkr driver to allow concurrent access to the hardware.
> I split pcspkr and someone else will say that there is
> already enough of the midlayers to handle the like things,
> to not introduce another one just for the particular driver.
> Besides, I don't beleive people will be happy with having
> 2 modules for just handling the terminal beeps.
> The input midlayer looks like the best solution. It allows
> to deal with the modules as soon as they are loaded. It has
> enough of the information needed to precisely identify the
> module (I now use INPUT_DEVICE_ID_MATCH_BUS). The pcspkr *is*
> an "input driver" after all, so why not to deal with it via
> an input API?

pcspkr is still an input driver. Now you are adding another
driver to drive the same piece of hardware and you need
to mediate access to that hardware. It is not responsibility
of the input layer to do that. Like input core does not handle
PS/2 ports or USB hardware directly it should not do that for
the speaker either. Also, even now pcspkr implementation is
deficient - issuing SND_BELL will kill SND_TONE going at the
moment.

> If the input should not be used for anything
> related to the port IO,

Input core itself should not. It is positioned not at physical
device level. It gets and propagates events from individual
drivers which talk to hardware, maybe via port IO. 
  
>  then why it carries the information
> about the ports and the bus that are used by the device?
> Why does it have the INPUT_DEVICE_ID_MATCH_BUS after all?

For userspace benefits.

> The input API only lacks a very small piece of the functionality -
> disabling the device, which can easily be used by anything else
> in the future. Is there a reason not to include that functionality
> only because the snd-pcsp is going to use it, or is there any *other*
> reason?
>

See above. Your module wants to access hardware concurrently with
another module so implement it properly. While you are fine with
disabling beeps while music is playing otherpeoplr might still want
to hear them.
 
--
Dmitry

Re: [patch] add input_enable_device()

[Stas Sergeev]

Dmitry Torokhov wrote:
>> Why does it have the INPUT_DEVICE_ID_MATCH_BUS after all?
> For userspace benefits.
How exactly does the userspace benefit from the
INPUT_DEVICE_ID_MATCH_BUS thing?
And, by the way, why doesn't the input have the
capability of disabling/enabling the device?

> While you are fine with
> disabling beeps while music is playing otherpeoplr might still want
> to hear them.
The only possibility to do this, was to have the grabbing
capability *in input layer*, which you already rejected too.
With this, it was possible to have such a behaviour run-time
configurable, but now my best bet would be to resort to the
Kconfig games, making a note for users that the uinput is now
an only possibility to route the terminal beeps to the snd-pcsp.

Re: [patch] add input_enable_device()

[Dmitry Torokhov]

--- Stas Sergeev <stsp@aknet.ru> wrote:

> Dmitry Torokhov wrote:
> >> Why does it have the INPUT_DEVICE_ID_MATCH_BUS after all?
> > For userspace benefits.
> How exactly does the userspace benefit from the
> INPUT_DEVICE_ID_MATCH_BUS thing?
> And, by the way, why doesn't the input have the
> capability of disabling/enabling the device?
> 

What for? If you do not want to get events form a device do not
read it. Or do not compile/load the driver. You can do a lot of
things from userspace.

Your problem is that you want to one piece of kernel to take over
another kernel piece instead of making it work together. With
your enable/disable scheme what happens where there is 3rd module
that wants to do stuff with speaker? Does it also disable snd-pcsp?

> > While you are fine with
> > disabling beeps while music is playing otherpeoplr might still want
> > to hear them.
> The only possibility to do this, was to have the grabbing
> capability *in input layer*, which you already rejected too.
> With this, it was possible to have such a behaviour run-time
> configurable, but now my best bet would be to resort to the
> Kconfig games, making a note for users that the uinput is now
> an only possibility to route the terminal beeps to the snd-pcsp.
> 

You just do not want to implement proper access control for the
hardware, that's it. 

--
Dmitry

Re: [patch] add input_enable_device()

[Stas Sergeev]

Dmitry Torokhov wrote:
>>>> Why does it have the INPUT_DEVICE_ID_MATCH_BUS after all?
>>> For userspace benefits.
>> How exactly does the userspace benefit from the
>> INPUT_DEVICE_ID_MATCH_BUS thing?
This is still not answered. If INPUT_DEVICE_ID_MATCH_BUS
is there, then I don't see the argument that the input
layer is not designed for the like things.

> You just do not want to implement proper access control for the
> hardware, that's it.
Depends on an answer to the question above, whether using
input is the proper way or not.

Re: [patch] add input_enable_device()

[Dmitry Torokhov]

--- Stas Sergeev <stsp@aknet.ru> wrote:

> Dmitry Torokhov wrote:
> >>>> Why does it have the INPUT_DEVICE_ID_MATCH_BUS after all?
> >>> For userspace benefits.
> >> How exactly does the userspace benefit from the
> >> INPUT_DEVICE_ID_MATCH_BUS thing?
> This is still not answered. If INPUT_DEVICE_ID_MATCH_BUS
> is there, then I don't see the argument that the input
> layer is not designed for the like things.

Yes, you are right. INPUT_DEVICE_ID_MATCH_BUS will not likely
benefit anyone. It is highly unlikely that we would have a handler
for devices on specific bus or for devices made only by one vendor.
When I mentioned userspace I was talk ing about exporting bus,
product, vendor and version values to userspace which might be
still benefitial. Although as we move along to sysfsifying the
kernel bus information can be traced through sysfs instead.

> 
> > You just do not want to implement proper access control for the
> > hardware, that's it.
> Depends on an answer to the question above, whether using
> input is the proper way or not.
> 

Consider this: pcspkr is broken at the moment as it does not
handle several simultaneous events well. If you fix it do behave
properly with SND_TONE and SND_BELL arriving at the same time
then adding hooks to the speaker code for snd-pcsp should be
pretty easy. See?

--
Dmitry

Re: [patch] add input_enable_device()

[Stas Sergeev]

Dmitry Torokhov wrote:
> Yes, you are right. INPUT_DEVICE_ID_MATCH_BUS will not likely
> benefit anyone.
And according to the impression I've got, it should
therefore be removed, sure? :)

> Consider this: pcspkr is broken at the moment as it does not
> handle several simultaneous events well. If you fix it do behave
> properly with SND_TONE and SND_BELL arriving at the same time
> then adding hooks to the speaker code for snd-pcsp should be
> pretty easy. See?
I see the point but not the practicle solution. request_region()
won't work as the ports are already claimed by other drivers.
Resolving this inside pcspkr.c won't help snd-pcsp, as it will
add the dependancy if some internal API is used.
The real problem is that whereever I'll solve that, this will be
a hack, because normally two drivers should not compete for the
IO resourses, disable each other, race etc.
So I give it up. I'll just disable pcspkr in the Kconfig and
whoever wants both snd-pcsp and the beeps, will need to install
the user-space daemon which uses uinput. I have yet to find such
a daemon...

Re: [patch] add input_enable_device()

[Stas Sergeev]

[-- Attachment #1: Type: text/plain, Size: 388 bytes --]

Hello.

Andrew Morton wrote:
> Separate functions input_enable_device(struct input_handle *handle) and
> input_disable_device(struct input_handle *handle) would be preferred.
Right, and here it goes.

---
add input_enable_device() and input_disable_device()

Signed-off-by: Stas Sergeev <stsp@aknet.ru>
CC: Dmitry Torokhov <dtor_core@ameritech.net>
CC: Vojtech Pavlik <vojtech@suse.cz>



[-- Attachment #2: input_en2.diff --]
[-- Type: text/x-patch, Size: 1651 bytes --]

--- a/include/linux/input.h	2006-04-05 17:10:01.000000000 +0400
+++ b/include/linux/input.h	2006-04-05 17:36:49.000000000 +0400
@@ -878,7 +878,7 @@
 
 	struct pt_regs *regs;
 	int state;
-
+	int disabled;
 	int sync;
 
 	int abs[ABS_MAX + 1];
@@ -1038,6 +1038,9 @@
 int input_open_device(struct input_handle *);
 void input_close_device(struct input_handle *);
 
+void input_enable_device(struct input_handle *handle);
+void input_disable_device(struct input_handle *handle);
+
 int input_accept_process(struct input_handle *handle, struct file *file);
 int input_flush_device(struct input_handle* handle, struct file* file);
 
--- a/drivers/input/input.c	2006-01-12 11:23:09.000000000 +0300
+++ b/drivers/input/input.c	2006-04-05 17:51:27.000000000 +0400
@@ -37,6 +37,8 @@
 EXPORT_SYMBOL(input_release_device);
 EXPORT_SYMBOL(input_open_device);
 EXPORT_SYMBOL(input_close_device);
+EXPORT_SYMBOL(input_enable_device);
+EXPORT_SYMBOL(input_disable_device);
 EXPORT_SYMBOL(input_accept_process);
 EXPORT_SYMBOL(input_flush_device);
 EXPORT_SYMBOL(input_event);
@@ -53,7 +55,7 @@
 {
 	struct input_handle *handle;
 
-	if (type > EV_MAX || !test_bit(type, dev->evbit))
+	if (type > EV_MAX || !test_bit(type, dev->evbit) || dev->disabled)
 		return;
 
 	add_input_randomness(type, code, value);
@@ -269,6 +271,16 @@
 	mutex_unlock(&dev->mutex);
 }
 
+void input_enable_device(struct input_handle *handle)
+{
+	handle->dev->disabled = 0;
+}
+
+void input_disable_device(struct input_handle *handle)
+{
+	handle->dev->disabled = 1;
+}
+
 static void input_link_handle(struct input_handle *handle)
 {
 	list_add_tail(&handle->d_node, &handle->dev->h_list);

Re: [patch] add input_enable_device()

[Andrew Morton]

Stas Sergeev <stsp@aknet.ru> wrote:
>
> add input_enable_device() and input_disable_device()
> 
>  Signed-off-by: Stas Sergeev <stsp@aknet.ru>
>  CC: Dmitry Torokhov <dtor_core@ameritech.net>
>  CC: Vojtech Pavlik <vojtech@suse.cz>

uh, that's not a complete changelog - it describes what the code does
(which is obvious), but doesn't describe why it does it.

iirc it had to do with the pc-speaker driver, but I don't seem to be able
to locate the original email.

Re: [patch] add input_enable_device()

[Stas Sergeev]

[-- Attachment #1: Type: text/plain, Size: 588 bytes --]

Hello.

Andrew Morton wrote:
> iirc it had to do with the pc-speaker driver, but I don't seem to be able
> to locate the original email.
OK, sorry, I haven't realized its important.

---
The patch below adds input_enable_device() and input_disable_device()
to the input subsystem, that allows to enable and disable the input
devices. The reason to have it, is the snd-pcsp PC-Speaker driver in
an ALSA tree that needs an ability to disable the pcspkr driver.

Signed-off-by: Stas Sergeev <stsp@aknet.ru>
CC: Dmitry Torokhov <dtor_core@ameritech.net>
CC: Vojtech Pavlik <vojtech@suse.cz>


[-- Attachment #2: input_en2.diff --]
[-- Type: text/x-patch, Size: 1651 bytes --]

--- a/include/linux/input.h	2006-04-05 17:10:01.000000000 +0400
+++ b/include/linux/input.h	2006-04-05 17:36:49.000000000 +0400
@@ -878,7 +878,7 @@
 
 	struct pt_regs *regs;
 	int state;
-
+	int disabled;
 	int sync;
 
 	int abs[ABS_MAX + 1];
@@ -1038,6 +1038,9 @@
 int input_open_device(struct input_handle *);
 void input_close_device(struct input_handle *);
 
+void input_enable_device(struct input_handle *handle);
+void input_disable_device(struct input_handle *handle);
+
 int input_accept_process(struct input_handle *handle, struct file *file);
 int input_flush_device(struct input_handle* handle, struct file* file);
 
--- a/drivers/input/input.c	2006-01-12 11:23:09.000000000 +0300
+++ b/drivers/input/input.c	2006-04-05 17:51:27.000000000 +0400
@@ -37,6 +37,8 @@
 EXPORT_SYMBOL(input_release_device);
 EXPORT_SYMBOL(input_open_device);
 EXPORT_SYMBOL(input_close_device);
+EXPORT_SYMBOL(input_enable_device);
+EXPORT_SYMBOL(input_disable_device);
 EXPORT_SYMBOL(input_accept_process);
 EXPORT_SYMBOL(input_flush_device);
 EXPORT_SYMBOL(input_event);
@@ -53,7 +55,7 @@
 {
 	struct input_handle *handle;
 
-	if (type > EV_MAX || !test_bit(type, dev->evbit))
+	if (type > EV_MAX || !test_bit(type, dev->evbit) || dev->disabled)
 		return;
 
 	add_input_randomness(type, code, value);
@@ -269,6 +271,16 @@
 	mutex_unlock(&dev->mutex);
 }
 
+void input_enable_device(struct input_handle *handle)
+{
+	handle->dev->disabled = 0;
+}
+
+void input_disable_device(struct input_handle *handle)
+{
+	handle->dev->disabled = 1;
+}
+
 static void input_link_handle(struct input_handle *handle)
 {
 	list_add_tail(&handle->d_node, &handle->dev->h_list);

Re: [patch] add input_enable_device()

[Chase Venters]

On Tue, 16 May 2006, Stas Sergeev wrote:

> Hello.
>
> Andrew Morton wrote:
>>  iirc it had to do with the pc-speaker driver, but I don't seem to be able
>>  to locate the original email.
> OK, sorry, I haven't realized its important.
>
> ---
> The patch below adds input_enable_device() and input_disable_device()
> to the input subsystem, that allows to enable and disable the input
> devices. The reason to have it, is the snd-pcsp PC-Speaker driver in
> an ALSA tree that needs an ability to disable the pcspkr driver.
>
> Signed-off-by: Stas Sergeev <stsp@aknet.ru>
> CC:  Dmitry Torokhov <dtor_core@ameritech.net>
> CC:  Vojtech Pavlik <vojtech@suse.cz>
>
>

> --- a/include/linux/input.h	2006-04-05 17:10:01.000000000 +0400
> +++ b/include/linux/input.h	2006-04-05 17:36:49.000000000 +0400
> @@ -878,7 +878,7 @@
>
>  	struct pt_regs *regs;
>  	int state;
> -
> +	int disabled;
>  	int sync;
>
>  	int abs[ABS_MAX + 1];

Why eat an entire word here? It seems we already have a "dynalloc" 
boolean/int... perhaps some of these things could be rolled up into a 
bitfield.

Cheers,
Chase