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?
--- Stas Sergeev <[email protected]> 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
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.
--- Stas Sergeev <[email protected]> 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
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.
--- Stas Sergeev <[email protected]> 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
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...