Hi,

On 6/2/20 8:50 PM, Andrzej Pietrasiewicz wrote:

Hi Dmitry,

W dniu 02.06.2020 o 19:52, Dmitry Torokhov pisze:

quoted

Hi Andrzej,

On Tue, Jun 02, 2020 at 06:56:40PM +0200, Andrzej Pietrasiewicz wrote:

quoted

Hi Dmitry,

W dniu 27.05.2020 o 08:34, Dmitry Torokhov pisze:

quoted

That said, I think the way we should handle inhibit/uninhibit, is that
if we have the callback defined, then we call it, and only call open and
close if uninhibit or inhibit are _not_ defined.

If I understand you correctly you suggest to call either inhibit,
if provided or close, if inhibit is not provided, but not both,
that is, if both are provided then on the inhibit path only
inhibit is called. And, consequently, you suggest to call either
uninhibit or open, but not both. The rest of my mail makes this
assumption, so kindly confirm if I understand you correctly.

Yes, that is correct. If a driver wants really fine-grained control, it
will provide inhibit (or both inhibit and close), otherwise it will rely
on close in place of inhibit.

quoted

In my opinion this idea will not work.

The first question is should we be able to inhibit a device
which is not opened? In my opinion we should, in order to be
able to inhibit a device in anticipation without needing to
open it first.

I agree.

quoted

Then what does opening (with input_open_device()) an inhibited
device mean? Should it succeed or should it fail?

It should succeed.

quoted

If it is not
the first opening then effectively it boils down to increasing
device's and handle's counters, so we can allow it to succeed.
If, however, the device is being opened for the first time,
the ->open() method wants to be called, but that somehow
contradicts the device's inhibited state. So a logical thing
to do is to either fail input_open_device() or postpone ->open()
invocation to the moment of uninhibiting - and the latter is
what the patches in this series currently do.

Failing input_open_device() because of the inhibited state is
not the right thing to do. Let me explain. Suppose that a device
is already inhibited and then a new matching handler appears
in the system. Most handlers (apm-power.c, evbug.c, input-leds.c,
mac_hid.c, sysrq.c, vt/keyboard.c and rfkill/input.c) don't create
any character devices (only evdev.c, joydev.c and mousedev.c do),
so for them it makes no sense to delay calling input_open_device()
and it is called in handler's ->connect(). If input_open_device()
now fails, we have lost the only chance for this ->connect() to
succeed.

Summarizing, IMO the uninhibit path should be calling both
->open() and ->uninhibit() (if provided), and conversely, the inhibit
path should be calling both ->inhibit() and ->close() (if provided).

So what you are trying to say is that you see inhibit as something that
is done in addition to what happens in close. But what exactly do you
want to do in inhibit, in addition to what close is doing?

See below (*).

quoted

In my view, if we want to have a dedicated inhibit callback, then it
will do everything that close does, they both are aware of each other
and can sort out the state transitions between them. For drivers that do
not have dedicated inhibit/uninhibit, we can use open and close
handlers, and have input core sort out when each should be called. That
means that we should not call dev->open() in input_open_device() when
device is inhibited (and same for dev->close() in input_close_device).
And when uninhibiting, we should not call dev->open() when there are no
users for the device, and no dev->close() when inhibiting with no users.

Do you see any problems with this approach?

My concern is that if e.g. both ->open() and ->uninhibit() are provided,
then in certain circumstances ->open() won't be called:

1. users == 0
2. inhibit happens
3. input_open_device() happens, ->open() not called
4. uninhibit happens
5. as part of uninhibit ->uninhibit() is only called, but ->open() is not.

They way I understand your answer is that we implicitly impose requirements
on drivers which choose to implement e.g. both ->open() and ->uninhibit():
in such a case ->uninhibit() should be doing exactly the same things as
->open() does. Which leads to a conclusion that in practice no drivers
should choose to implement both, otherwise they must be aware that
->uninhibit() can be sometimes called instead of ->open(). Then ->open()
becomes synonymous with ->uninhibit(), and ->close() with ->inhibit().
Or, maybe, then ->inhibit() can be a superset of ->close() and
->uninhibit() a superset of ->open().

If such an approach is ok with you, it is ok with me, too.

(*)
Calling both ->inhibit() and ->close() (if they are provided) allows
drivers to go fancy and fail inhibiting (which is impossible using
only ->close() as it does not return a value, but ->inhibit() by design
does). Then ->uninhibit() is mostly for symmetry.

All the complications discussed above are exactly why I still
believe that there should be only open and close.

If error propagation on inhibit is considered as something
really important to have then we can make the input driver close
callback return an error (*), note I'm talking about the
driver close callback here, not the system call.

If the close callback is called for actually closing the fd
referring to the input node, then the new error return code
can be ignored, as we already do for errors on close atm
since the driver close callback returns void.

I still have not seen a very convincing argument for having
separate inhibit and close callbacks and as the messy discussion
above shows, having 2 such very similar yet subtly different
calls seems like a bad idea...

Regards,

Hans


*) This will require a flag day where "return 0" is added
to all current close handlers


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel