Re: [PATCH v4 0/7] Support inhibiting input devices

[Andrzej Pietrasiewicz]

Hi All,

W dniu 10.06.2020 oÂ12:38, Rafael J. Wysocki pisze:
> On Wed, Jun 10, 2020 at 11:50 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> > Hi All,
> >
> > On 6/8/20 1:22 PM, Andrzej Pietrasiewicz wrote:
> > > This is a quick respin of v3, with just two small changes, please see
> > > the changelog below.
> > >
> > > Userspace might want to implement a policy to temporarily disregard input
> > > from certain devices.
> > >
> > > An example use case is a convertible 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.
> >
> > First of all sorry to start a somewhat new discussion about this
> > while this patch set is also somewhat far along in the review process,
> > but I believe what I discuss below needs to be taken into account.
> >
> > Yesterday I have been looking into why an Asus T101HA would not stay
> > suspended when the LID is closed. The cause is that the USB HID multi-touch
> > touchpad in the base of the device starts sending events when the screen
> > gets close to the touchpad (so when the LID is fully closed) and these
> > events are causing a wakeup from suspend. HID multi-touch devices
> > do have a way to tell them to fully stop sending events, also disabling
> > the USB remote wakeup the device is doing. The question is when to tell
> > it to not send events though ...
> >
> > So now I've been thinking about how to fix this and I believe that there
> > is some interaction between this problem and this patch-set.
> >
> > The problem I'm seeing on the T101HA is about wakeups, so the question
> > which I want to discuss is:
> >
> > 1. How does inhibiting interact with enabling /
> > disabling the device as a wakeup source ?
> >
> > 2. Since we have now made inhibiting equal open/close how does open/close
> > interact with a device being a wakeup source ?
> >
> > And my own initial (to be discussed) answers to these questions:
> >
> > 1. It seems to me that when a device is inhibited it should not be a
> > wakeup source, so where possible a input-device-driver should disable
> > a device's wakeup capabilities on suspend if inhibited
>
> If "inhibit" means "do not generate any events going forward", then
> this must also cover wakeup events, so I agree.

I agree, too.

> > 2. This one is trickier I don't think we have really clearly specified
> > any behavior here. The default behavior of most drivers seems to be
> > using something like this in their suspend callback:
> >
> >           if (device_may_wakeup(dev))
> >                   enable_irq_wake(data->irq);
> >           else if (input->users)
> >                   foo_stop_receiving_events(data);
> >
> > Since this is what most drivers seem to do I believe we should keep
> > this as is and that we should just clearly document that if the
> > input_device has users (has been opened) or not does not matter
> > for its wakeup behavior.
> >
> > Combining these 2 answers leads to this new pseudo code template
> > for an input-device's suspend method:
> >
> >          /*
> >           * If inhibited we have already disabled events and
> >           * we do NOT want to setup the device as wake source.
> >           */
> >          if (input->inhibited)
> >                  return 0;

Right, if a device is inhibited it shouldn't become a wakeup source,
because that would contradict the purpose of being inhibited.

> >           if (device_may_wakeup(dev))
> >                   enable_irq_wake(data->irq);

What would it mean to become a wakeup source if there are no users,
or nobody has ever opened the device? There are no interested
input handlers (users) so what's the point of becoming a wakeup
source? Why would the system need to wake up?

> >           else if (input->users)
> >                   foo_stop_receiving_events(data);
> >
> > ###
>
> Sounds reasonable to me.
>
> > A different, but related issue is how to make devices actually use the
> > new inhibit support on the builtin keyboard + touchpad when say the lid
> > is closed.   Arguably this is an userspace problem, but it is a tricky
> > one. Currently on most modern Linux distributions suspend-on-lid-close
> > is handled by systemd-logind and most modern desktop-environments are
> > happy to have logind handle this for them.
> >
> > But most knowledge about input devices and e.g. heurisitics to decide
> > if a touchpad is internal or external are part of libinput. Now we could
> > have libinput use the new inhibit support (1), but then when the lid
> > closes we get race between whatever process is using libinput trying
> > to inhibit the touchpad (which must be done before to suspend to disable
> > it as wakeup source) and logind trying to suspend the system.
> >
> > One solution here would be to move the setting of the inhibit sysfs
> > attr into logind, but that requires adding a whole bunch of extra
> > knowledge to logind which does not really belong there IMHO.
> >
> > I've been thinking a bit about this and to me it seems that the kernel
> > is in the ideal position to automatically inhibit some devices when
> > some EV_SW transitions from 0->1 (and uninhibit again on 1->0). The
> > issue here is to chose on which devices to enable this. I believe
> > that the auto inhibit on some switches mechanism is best done inside
> > the kernel (disabled by default) and then we can have a sysfs
> > attr called auto_inhibit_ev_sw_mask which can be set to e.g.
> > (1 << SW_LID) to make the kernel auto-inhibit the input-device whenever
> > the lid is closed, or to ((1 << SW_LID) | (1 << SW_TABLET_MODE)) to
> > inhibit both when the lid is closed or when switched to tablet mode.
>
> I agree that the kernel is the right place to handle this, but it
> requires some extra knowledge about dependencies between devices.
>
> It'd be kind of like power resources in ACPI, so for each state of a
> "master" device (in principle, there may be more states of it than
> just two) there would be a list of "dependent" intput devices that
> need to be inhibited when the "master" device goes into that state.
>
> > This could then be combined with a userspace utility run from an
> > udev rule which makes the actual decision what auto_inhibit_ev_sw_mask
> > should be set for a given input device.
> >
> > This will put the mechanism for what we want inside the kernel and
> > leaves the policy on which switches we want this for out of the
> > kernel.
> >
> > Note adding this new auto_inhibit_ev_sw_mask sysfs attr falls
> > somewhat outside the context of this patchset and could be done
> > as a follow up to this patch-set.

Yes, please ;)

But I do believe that we need to
> > figure out how (non ChromeOS) userspace can / will use the new inhibit
> > interface before merging it.

Of course.

Regards,

Andrzej