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

[Hans de Goede]

Hi,

On 6/10/20 3:12 PM, Andrzej Pietrasiewicz wrote:
> 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.

Ack. Note I do think that we need to document this (and more
in general the answer to both questions from above) clearly so
that going forward if there are any questions about how this is
supposed to work we can just point to the docs.

Can you do a follow-up patch, or include a patch in your next
version which documents this (once we agree on what "this"
exactly is) ?

> > > ÂÂÂÂÂÂÂÂÂ 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?

Well this is what we have been doing so far, so arguably we
need to keep doing it to avoid regressions / breaking our ABI.

Lets for example take a laptop, where when suspended the
power-button is the only valid wakeup-source and this is
running good old slackware with fvwm2 or windowmaker as
"desktop environment", then likely no process will have
the power-button input evdev node open.  Still we should
wakeup the laptop on the power-button press, otherwise
it will never wakeup.

Note I agree with you that the way this works is not
ideal, I just do not think that we can change it.

> > > ÂÂÂÂÂÂÂÂÂ else if (input->users)
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ foo_stop_receiving_events(data);
> > >
> > > ###

<snip>

Regards,

Hans