Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Hans de Goede <hdegoede@redhat.com>
Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>,
	linux-input@vger.kernel.org, linux-acpi@vger.kernel.org,
	linux-iio@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org, linux-tegra@vger.kernel.org,
	patches@opensource.cirrus.com,
	ibm-acpi-devel@lists.sourceforge.net,
	platform-driver-x86@vger.kernel.org,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Len Brown <lenb@kernel.org>, Jonathan Cameron <jic23@kernel.org>,
	Hartmut Knaack <knaack.h@gmx.de>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	Kukjin Kim <kgene@kernel.org>,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Shawn Guo <shawnguo@kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Fabio Estevam <festevam@gmail.com>,
	NXP Linux Team <linux-imx@nxp.com>,
	Vladimir Zapolskiy <vz@mleia.com>,
	Sylvain Lemieux <slemieux.tyco@gmail.com>,
	Laxman Dewangan <ldewangan@nvidia.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Barry Song <baohua@kernel.org>,
	Michael Hennerich <michael.hennerich@analog.com>,
	Nick Dyer <nick@shmanahar.org>, Ferruh Yigit <fery@cypress.com>,
	Sangwon Jee <jeesw@melfas.com>,
	Henrique de Moraes Holschuh <ibm-acpi@hmh.eng.br>,
	kernel@collabora.com, Peter Hutterer <peter.hutterer@redhat.com>,
	Benjamin Tissoires <btissoir@redhat.com>
Subject: Re: [PATCHv2 0/7] Support inhibiting input devices
Date: Thu, 4 Jun 2020 00:28:53 -0700
Message-ID: <20200604072853.GP89269@dtor-ws> (raw)
In-Reply-To: <01b902dd-8841-e697-5ba7-96fa6b73c1cd@redhat.com>

Hi Hans, Andrzej,

On Wed, Jun 03, 2020 at 09:37:10PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 6/3/20 7:54 PM, Andrzej Pietrasiewicz wrote:
> > W dniu 03.06.2020 o 19:38, Hans de Goede pisze:
> > > 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.
> > 
> > Right, I can see now that the suggestion to change names is
> > too far fetched. (I feel that release() would be better
> > than close(), though). But it exposes the message I wanted to
> > pass.

release() usually means that the object is destroyedm, i.e this action,
unlike close() is irrevocable.

Let's leave the names as is, and adjust kerneldoc comments as needed.

> > 
> > > 
> > > 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.
> > > 
> > 
> > Yeah, that's my conclusion - at least on a conceptual level.
> > 
> > That said, what I can imagine is an existing driver (e.g. elan_i2c)
> > which does not implement neither open() nor close(), but does have
> > suspend() and resume(). Then it is maybe a bit easier to add inhibit()
> > and uninhibit() /they would be similar to suspend and resume/ instead
> > of open() and close(): If only open() and close() are possible, then
> > the probe function needs to be extended to "close" the device before it
> > gets registered, because from the moment it is registered it might be
> > opened right away.
> 
> The probe only needs to "close" it if for some reason it
> starts directly sending events in most cases the driver
> must actively do something to get it to send events.
> 
> So in most cases this should be pretty straight forward,
> as for having to do some init / power-on during probe
> and then power-off at the end of the probe. Yes sometimes
> something like that might be necessary.
> 
> Looking at your suggested elan_i2c changes I think they
> look fine. I have the feeling that with some refactoring
> they can be made a bit cleaner (I did not look a the
> changes in too much detail) but overall I think they
> look ok.
> 
> Note you may also want to look at using the runtime
> suspend framework for this, doing a pm_runtime_get_sync()
> in open() and then letting (runtime) suspend do the power
> off if you set a reasonable timeout for autosuspend after
> the last user is gone then that will also avoid an
> unnecessary suspend / resume cycle between probe()
> exiting and the first open() call and this avoids the
> need to do a poweroff() at the end of probe(), the
> runtime-pm framework will autosuspend the device after
> the timeout expires.

Yes, plugging into runtime PM would be nice, as as it currently stands
the driver will be broken with regard to trying access sysfs for
firmware update/calibration/etc if device happens to be inhibited.

The version of the driver in Chrome OS tree is similarly broken, but
because we control both kernel and the rest of the stack we know that we
do not poke at sysfs when device is inhibited. It will not be acceptable
for mainline (and that is one of reasons why elan_i2c does not have
open/close methods at the moment).

Thanks.

-- 
Dmitry

  reply index

Thread overview: 73+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20200506002746.GB89269@dtor-ws>
2020-05-15 16:49 ` Andrzej Pietrasiewicz
2020-05-15 16:51   ` [PATCHv2 1/7] Input: add input_device_enabled() Andrzej Pietrasiewicz
2020-05-15 16:52   ` [PATCHv2 4/7] iio: adc: exynos: Use input_device_enabled() Andrzej Pietrasiewicz
2020-05-15 16:52   ` [PATCHv2 6/7] Input: Add "inhibited" property Andrzej Pietrasiewicz
2020-05-15 18:19   ` [PATCHv2 0/7] Support inhibiting input devices Hans de Goede
2020-05-17 22:55     ` Peter Hutterer
2020-05-18  2:40       ` Dmitry Torokhov
2020-05-18  7:36         ` Hans de Goede
2020-05-22 15:35         ` Andrzej Pietrasiewicz
2020-05-27  6:13           ` Peter Hutterer
2020-05-18 10:48     ` Andrzej Pietrasiewicz
2020-05-18 12:24       ` Hans de Goede
2020-05-18 13:49         ` Andrzej Pietrasiewicz
2020-05-18 14:23           ` Hans de Goede
2020-05-19  9:02             ` Andrzej Pietrasiewicz
2020-05-19  9:36               ` Hans de Goede
2020-05-27  6:34                 ` Dmitry Torokhov
2020-06-02 16:56                   ` Andrzej Pietrasiewicz
2020-06-02 17:52                     ` Dmitry Torokhov
2020-06-02 18:50                       ` Andrzej Pietrasiewicz
2020-06-02 20:19                         ` Hans de Goede
2020-06-03 13:07                           ` Andrzej Pietrasiewicz
2020-06-03 17:38                             ` Hans de Goede
2020-06-03 17:54                               ` Andrzej Pietrasiewicz
2020-06-03 19:37                                 ` Hans de Goede
2020-06-04  7:28                                   ` Dmitry Torokhov [this message]
2020-06-05 17:33                                     ` [PATCH v3 " Andrzej Pietrasiewicz
2020-06-05 17:33                                       ` [PATCH v3 1/7] Input: add input_device_enabled() Andrzej Pietrasiewicz
2020-06-05 17:33                                       ` [PATCH v3 2/7] Input: use input_device_enabled() Andrzej Pietrasiewicz
2020-06-05 17:33                                       ` [PATCH v3 3/7] ACPI: button: Access input device's users under appropriate mutex Andrzej Pietrasiewicz
2020-06-05 17:33                                       ` [PATCH v3 4/7] ACPI: button: Use input_device_enabled() helper Andrzej Pietrasiewicz
2020-06-05 17:33                                       ` [PATCH v3 5/7] iio: adc: exynos: Use input_device_enabled() Andrzej Pietrasiewicz
2020-06-05 19:49                                         ` Michał Mirosław
2020-06-05 17:33                                       ` [PATCH v3 6/7] platform/x86: thinkpad_acpi: " Andrzej Pietrasiewicz
2020-06-05 17:33                                       ` [PATCH v3 7/7] Input: Add "inhibited" property Andrzej Pietrasiewicz
2020-06-05 17:41                                         ` Hans de Goede
2020-06-08 11:22                                           ` [PATCH v4 0/7] Support inhibiting input devices Andrzej Pietrasiewicz
2020-06-08 11:22                                             ` [PATCH v4 1/7] Input: add input_device_enabled() Andrzej Pietrasiewicz
2020-06-08 11:22                                             ` [PATCH v4 2/7] Input: use input_device_enabled() Andrzej Pietrasiewicz
2020-06-08 11:22                                             ` [PATCH v4 3/7] ACPI: button: Access input device's users under appropriate mutex Andrzej Pietrasiewicz
2020-06-24 15:00                                               ` Rafael J. Wysocki
2020-06-25  5:23                                                 ` Dmitry Torokhov
2020-06-25 10:55                                                   ` Rafael J. Wysocki
2020-06-08 11:22                                             ` [PATCH v4 4/7] ACPI: button: Use input_device_enabled() helper Andrzej Pietrasiewicz
2020-06-25  5:24                                               ` Dmitry Torokhov
2020-06-08 11:22                                             ` [PATCH v4 5/7] iio: adc: exynos: Use input_device_enabled() Andrzej Pietrasiewicz
2020-06-10  1:28                                               ` Michał Mirosław
2020-06-10  7:52                                                 ` [FIXED PATCH " Andrzej Pietrasiewicz
2020-06-08 11:22                                             ` [PATCH v4 6/7] platform/x86: thinkpad_acpi: " Andrzej Pietrasiewicz
2020-06-08 11:22                                             ` [PATCH v4 7/7] Input: Add "inhibited" property Andrzej Pietrasiewicz
2020-06-10  9:49                                             ` [PATCH v4 0/7] Support inhibiting input devices Hans de Goede
2020-06-10 10:38                                               ` Rafael J. Wysocki
2020-06-10 13:12                                                 ` Andrzej Pietrasiewicz
2020-06-10 13:21                                                   ` Hans de Goede
2020-06-10 13:41                                                     ` Andrzej Pietrasiewicz
2020-06-12  8:30                                                       ` Hans de Goede
2020-06-12  8:47                                                         ` Andrzej Pietrasiewicz
2020-06-16 17:29                                                         ` [PATCH] Input: document inhibiting Andrzej Pietrasiewicz
2020-06-16 17:38                                                           ` Randy Dunlap
2020-06-17  7:44                                                           ` Hans de Goede
2020-06-17 10:18                                                             ` [PATCH v2] " Andrzej Pietrasiewicz
2020-06-17 10:21                                                               ` Hans de Goede
2020-06-17 16:52                                                               ` Randy Dunlap
2020-06-23 13:35                                                               ` Pavel Machek
2020-06-10 14:01                                                     ` [PATCH v4 0/7] Support inhibiting input devices Rafael J. Wysocki
2020-06-10 13:52                                                 ` Hans de Goede
2020-06-10 18:28                                                 ` Dmitry Torokhov
2020-06-12  8:14                                                   ` Hans de Goede
2020-06-12  8:17                                             ` Hans de Goede
2020-08-03 14:40                                               ` Andrzej Pietrasiewicz
2020-06-07 20:24                                       ` [PATCH v3 " Pavel Machek
2020-06-08  5:37                                         ` Dmitry Torokhov
2020-06-08  9:28                                           ` Andrzej Pietrasiewicz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200604072853.GP89269@dtor-ws \
    --to=dmitry.torokhov@gmail.com \
    --cc=andrzej.p@collabora.com \
    --cc=baohua@kernel.org \
    --cc=btissoir@redhat.com \
    --cc=fery@cypress.com \
    --cc=festevam@gmail.com \
    --cc=hdegoede@redhat.com \
    --cc=ibm-acpi-devel@lists.sourceforge.net \
    --cc=ibm-acpi@hmh.eng.br \
    --cc=jeesw@melfas.com \
    --cc=jic23@kernel.org \
    --cc=jonathanh@nvidia.com \
    --cc=kernel@collabora.com \
    --cc=kernel@pengutronix.de \
    --cc=kgene@kernel.org \
    --cc=knaack.h@gmx.de \
    --cc=krzk@kernel.org \
    --cc=lars@metafoo.de \
    --cc=ldewangan@nvidia.com \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=michael.hennerich@analog.com \
    --cc=nick@shmanahar.org \
    --cc=patches@opensource.cirrus.com \
    --cc=peter.hutterer@redhat.com \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=pmeerw@pmeerw.net \
    --cc=rjw@rjwysocki.net \
    --cc=s.hauer@pengutronix.de \
    --cc=shawnguo@kernel.org \
    --cc=slemieux.tyco@gmail.com \
    --cc=thierry.reding@gmail.com \
    --cc=vz@mleia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git