From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.4 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5D9DAC433E0 for ; Mon, 18 May 2020 14:23:30 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 3BD88207E8 for ; Mon, 18 May 2020 14:23:30 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="gPkmNsYw" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728119AbgEROX3 (ORCPT ); Mon, 18 May 2020 10:23:29 -0400 Received: from us-smtp-1.mimecast.com ([207.211.31.81]:32252 "EHLO us-smtp-delivery-1.mimecast.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727122AbgEROX3 (ORCPT ); Mon, 18 May 2020 10:23:29 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1589811806; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=N4kEWsurXWVRb491n6rW9nQFxH83gE62FpZ7K4CZ4Y8=; b=gPkmNsYwHcODmqrtBJ4fX+q6gqm7hHhnZd1/h8qfobbOeu3YlXQFsAnE2wZZxj0xhaPV51 jNMo9+kCK4Tve+MSZG3mFvkvps5lkf3CIgFUX2fytCqgvRmpIhVhqjOmoYt0A0drDh/WxI jDB/9aqRqq7NkjsQPZKXdg7B0I3A6oQ= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-275-ot5Q_mOTM520aOJ_-6F4hg-1; Mon, 18 May 2020 10:23:25 -0400 X-MC-Unique: ot5Q_mOTM520aOJ_-6F4hg-1 Received: by mail-wm1-f71.google.com with SMTP id l26so3164661wmh.3 for ; Mon, 18 May 2020 07:23:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=N4kEWsurXWVRb491n6rW9nQFxH83gE62FpZ7K4CZ4Y8=; b=mlf56rpgiOUFKuHagn5xwiUELFoQOxeSO/yYfpq574cejmv9VWRQk1rFwAhz/+Sxd+ SsAiryIAEKi4e0t7qosUtXW4XLAzVt+++2fImN+e01dqUwZPAdrX+h0ngoaphFr9z6u2 8DOr/P6QhjvYVh/HuTL2I2XtgF7HmnUDOa8T1PWyjpqhROk0D8uXuNueRJkVMl/1tOzJ rPzev8f83l4Mrao2Z6kBr9Jej9pDfV2hyaBgYLX2Kqe/ulQjLRGFeni6r8lKIxGgZOoP P9zoOKd45HZg4U2OuRmD3DnAOB3NfL6/KsI2HOwUR7hUjvzMH49zjNoYN3DyaAtxVJ7k 1z2g== X-Gm-Message-State: AOAM533DD+1v33Qa1g0z38n4qM9LMlqoLLDPkdK+0W6jZL/wo8H50R4f 3CdUegU/MxTEszD88nXZ4I5G7sAE0Phf473PAINfN8oWNulEpwlzmn9RXngpk+z5KXNFxK+vx0K oBfxp/Q/TxULlCHeaFy17nA== X-Received: by 2002:adf:e388:: with SMTP id e8mr19930285wrm.174.1589811804153; Mon, 18 May 2020 07:23:24 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyi/JVZYfBkPZCJ33i9pkkZ3Iw8Uks7lUYbBZX8naP80oeusj8a8mSkVZW0YdNpNt9SYero8A== X-Received: by 2002:adf:e388:: with SMTP id e8mr19930254wrm.174.1589811803884; Mon, 18 May 2020 07:23:23 -0700 (PDT) Received: from x1.localdomain (2001-1c00-0c0c-fe00-d2ea-f29d-118b-24dc.cable.dynamic.v6.ziggo.nl. [2001:1c00:c0c:fe00:d2ea:f29d:118b:24dc]) by smtp.gmail.com with ESMTPSA id g187sm16732224wmf.30.2020.05.18.07.23.22 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 18 May 2020 07:23:23 -0700 (PDT) Subject: Re: [PATCHv2 0/7] Support inhibiting input devices To: Andrzej Pietrasiewicz , 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 Cc: "Rafael J . Wysocki" , Len Brown , Jonathan Cameron , Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald-Stadler , Kukjin Kim , Krzysztof Kozlowski , Dmitry Torokhov , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , NXP Linux Team , Vladimir Zapolskiy , Sylvain Lemieux , Laxman Dewangan , Thierry Reding , Jonathan Hunter , Barry Song , Michael Hennerich , Nick Dyer , Ferruh Yigit , Sangwon Jee , Henrique de Moraes Holschuh , kernel@collabora.com, Peter Hutterer , Benjamin Tissoires References: <20200506002746.GB89269@dtor-ws> <20200515164943.28480-1-andrzej.p@collabora.com> <842b95bb-8391-5806-fe65-be64b02de122@redhat.com> <6d9921fc-5c2f-beda-4dcd-66d6970a22fe@redhat.com> <09679de4-75d3-1f29-ec5f-8d42c84273dd@collabora.com> From: Hans de Goede Message-ID: Date: Mon, 18 May 2020 16:23:21 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: <09679de4-75d3-1f29-ec5f-8d42c84273dd@collabora.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org Hi, On 5/18/20 3:49 PM, Andrzej Pietrasiewicz wrote: > Hi Hans, > > W dniu 18.05.2020 o 14:24, Hans de Goede pisze: >> Hi, >> >> On 5/18/20 12:48 PM, Andrzej Pietrasiewicz wrote: >>> Hi Hans, >>> >>> W dniu 15.05.2020 o 20:19, Hans de Goede pisze: >>>> Hi Andrezj, >>>> >>>> On 5/15/20 6:49 PM, Andrzej Pietrasiewicz wrote: >>>>> Userspace might want to implement a policy to temporarily disregard input >>>>> from certain devices, including not treating them as wakeup sources. >>>>> >>>>> An example use case is a laptop, whose keyboard can be folded under the >>>>> screen to create tablet-like experience. The user then must hold the laptop >>>>> in such a way that it is difficult to avoid pressing the keyboard keys. It >>>>> is therefore desirable to temporarily disregard input from the keyboard, >>>>> until it is folded back. This obviously is a policy which should be kept >>>>> out of the kernel, but the kernel must provide suitable means to implement >>>>> such a policy. >>>> >>>> Actually libinput already binds together (inside libinput) SW_TABLET_MODE >>>> generating evdev nodes and e.g. internal keyboards on devices with 360° >>>> hinges for this reason. libinput simply closes the /dev/input/event# >>>> node when folded and re-opens it when the keyboard should become active >>>> again. Thus not only suppresses events but allows e.g. touchpads to >>>> enter runtime suspend mode which saves power. Typically closing the >>>> /dev/input/event# node will also disable the device as wakeup source. >>>> >>>> So I wonder what this series actually adds for functionality for >>>> userspace which can not already be achieved this way? >>>> >>>> I also noticed that you keep the device open (do not call the >>>> input_device's close callback) when inhibited and just throw away >>> >>> I'm not sure if I understand you correctly, it is called: >>> >>> +static inline void input_stop(struct input_dev *dev) >>> +{ >>> +    if (dev->poller) >>> +        input_dev_poller_stop(dev->poller); >>> +    if (dev->close) >>> +        dev->close(dev); >>>                  ^^^^^^^^^^^^^^^^ >>> +static int input_inhibit(struct input_dev *dev) >>> +{ >>> +    int ret = 0; >>> + >>> +    mutex_lock(&dev->mutex); >>> + >>> +    if (dev->inhibited) >>> +        goto out; >>> + >>> +    if (dev->users) { >>> +        if (dev->inhibit) { >>> +            ret = dev->inhibit(dev); >>> +            if (ret) >>> +                goto out; >>> +        } >>> +        input_stop(dev); >>>                  ^^^^^^^^^^^^^^^^ >>> >>> It will not be called when dev->users is zero, but if it is zero, >>> then nobody has opened the device yet so there is nothing to close. >> >> Ah, I missed that. >> >> So if the device implements the inhibit call back then on >> inhibit it will get both the inhibit and close callback called? >> > > That's right. And conversely, upon uninhibit open() and uninhibit() > callbacks will be invoked. Please note that just as with open()/close(), > providing inhibit()/uninhibit() is optional. Ack. >> And what happens if the last user goes away and the device >> is not inhibited? > > close() is called as usually. But not inhibit, hmm, see below. >> I'm trying to understand here what the difference between the 2 >> is / what the goal of having a separate inhibit callback ? >> > > Drivers have very different ideas about what it means to suspend/resume > and open/close. The optional inhibit/uninhibit callbacks are meant for > the drivers to know that it is this particular action going on. So the inhibit() callback triggers the "suspend" behavior ? But shouldn't drivers which are capable of suspending the device always do so on close() ? Since your current proposal also calls close() on inhibit() I really see little difference between an inhibit() and the last user of the device closing it and IMHO unless there is a good reason to actually differentiate the 2 it would be better to only stick with the existing close() and in cases where that does not put the device in a low-power mode yet, fix the existing close() callback to do the low-power mode setting instead of adding a new callback. > For inhibit() there's one more argument: close() does not return a value, > so its meaning is "do some last cleanup" and as such it is not allowed > to fail - whatever its effect is, we must deem it successful. inhibit() > does return a value and so it is allowed to fail. Well, we could make close() return an error and at least in the inhibit() case propagate that to userspace. I wonder if userspace is going to do anything useful with that error though... In my experience errors during cleanup/shutdown are best logged (using dev_err) and otherwise ignored, so that we try to clean up as much possible. Unless the very first step of the shutdown process fails the device is going to be in some twilight zone state anyways at this point we might as well try to cleanup as much as possible. > All in all, it is up to the drivers to decide which callback they > provide. Based on my work so far I would say that there are tens > of simple cases where open() and close() are sufficient, out of total > ~400 users of input_allocate_device(): > > $ git grep "input_allocate_device(" | grep -v ^Documentation | \ > cut -f1 -d: | sort | uniq | wc >     390     390   13496 So can you explain a bit more about the cases where only having open/close is not sufficient? So far I have the feeling that those are all we need and that we really do not need separate [un]inhibit callbacks. Regards, Hans