Re: [PATCHv2 0/7] Support inhibiting input devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 6/3/20 3:07 PM, Andrzej Pietrasiewicz wrote:
Hi Hans, hi Dmitry,

<snip>

I'm taking one step back and looking at the ->open() and ->close()
driver callbacks. They are called from input_open_device() and
input_close_device(), respectively:

input_open_device():
"This function should be called by input handlers when they
want to start receive events from given input device."

->open() callback:
"this method is called when the very first user calls
input_open_device(). The driver must prepare the device to start
generating events (start polling thread, request an IRQ, submit
URB, etc.)"

input_close_device():
"This function should be called by input handlers when they
want to stop receive events from given input device."

->close() callback:
"this method is called when the very last user calls
input_close_device()"

It seems to me that the callback names do not reflect their
purpose: their meaning is not to "open" or to "close" but to
give drivers a chance to control when they start or stop
providing events to the input core.

What would you say about changing the callbacks' names?
I'd envsion: ->provide_events() instead of ->open() and
->stop_events() instead of ->close(). Of course drivers can
exploit the fact of knowing that nobody wants any events
from them and do whatever they consider appropriate, for
example go into a low power mode - but the latter is beyond
the scope of the input subsystem and is driver-specific.

I don't have much of an opinion on changing the names,
to me open/close have always means start/stop receiving
events. This follows the everything is a file philosophy,
e.g. you can also not really "open" a serial port,
yet opening /dev/ttyS0 will activate the receive IRQ
of the UART, etc. So maybe we just need to make the
docs clearer rather then do the rename?  Doing the
rename is certainly going to cause a lot of churn.

Anyways as said, I don't have much of an opinion,
so I'll leave commenting (more) on this to Dmitry.

With such a naming change in mind let's consider inhibiting.
We want to be able to control when to disregard events from
a given device. It makes sense to do it at device level, otherwise
such an operation would have to be invoked in all associated
handlers (those that have an open handle associating them with
the device in question). But of course we can do better than
merely ignoring the events received: we can tell the drivers
that we don't want any events from them, and later, at uninhibit
time, tell them to start providing the events again. Conceptually,
the two operations (provide or don't provide envents) are exactly
the same thing we want to be happening at input_open_device() and
input_close_device() time. To me, changing the names of
->open() and ->close() exposes this fact very well.

Consequently, ->inhibit() and ->uninhibit() won't be needed,
and drivers which already implement ->provide_events() (formerly
->open()) and ->stop_events() (formerly ->close()) will receive
full inhibit/uninhibit support for free (subject to how well they
implement ->provide_events()/->stop_events()). Unless we can come
up with what the drivers might be doing on top of ->stop_events()
and ->provide_events() when inhibiting/uninhibiting, but it seems
to me we can't. Can we?

Right. I'm happy that you've come to see that both on open/close
and on inhibit/uninhibit we want to "start receiving events" and
"stop receiving events", so that we only need one set of callbacks.

Optionally ->close() (only the callback, not input_close_device())
can be made return a value, just as Hans suggests. The value
can be ignored in input_close_device() but used in input_inhibit().
No strong opinion here, though. (btw it seems to me that
input_inhibit() should be renamed to input_inhibit_device()).

Ack.

Regards,

Hans




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux