Re: Make the hid-logitech-dj driver remove the HID++ nodes when the device disconnects

[Benjamin Tissoires <btissoir@redhat.com>]

On Thu, Feb 13, 2020 at 4:25 PM Filipe Laíns <lains@archlinux.org> wrote:
>
> On Thu, 2020-02-13 at 16:12 +0100, Benjamin Tissoires wrote:
> > On Fri, Feb 7, 2020 at 1:37 AM Filipe Laíns <lains@archlinux.org> wrote:
> > > On Fri, 2020-02-07 at 10:03 +1000, Peter Hutterer wrote:
> > > > On 7/2/20 3:01 am, Benjamin Tissoires wrote:
> > > > > On Thu, Feb 6, 2020 at 4:42 PM Filipe Laíns <lains@archlinux.org> wrote:
> > > > > > On Thu, 2020-02-06 at 13:13 +0100, Hans de Goede wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On 2/6/20 12:51 PM, Filipe Laíns wrote:
> > > > > > > > On Thu, 2020-02-06 at 12:30 +0100, Hans de Goede wrote:
> > > > > > > > > HI,
> > > > > > > > >
> > > > > > > > > On 2/6/20 12:14 PM, Filipe Laíns wrote:
> > > > > > > > > > Hello,
> > > > > > > > > >
> > > > > > > > > > Right now the hid-logitech-dj driver will export one node for each
> > > > > > > > > > connected device, even when the device is not connected. That causes
> > > > > > > > > > some trouble because in userspace we don't have have any way to know if
> > > > > > > > > > the device is connected or not, so when we try to communicate, if the
> > > > > > > > > > device is disconnected it will fail.
> > > > > > > > >
> > > > > > > > > I'm a bit reluctant to make significant changes to how the
> > > > > > > > > hid-logitech-dj driver works. We have seen a number of regressions
> > > > > > > > > when it was changed to handle the non unifying receivers and I would
> > > > > > > > > like to avoid more regressions.
> > > > > > > > >
> > > > > > > > > Some questions:
> > > > > > > > > 1. What is the specific use case where you are hitting this?
> > > > > > > >
> > > > > > > > For example, in libratbag we enumerate the devices and then probe them.
> > > > > > > > Currently if the device is not connected, the communication fails. To
> > > > > > > > get the device to show up we need to replug it, so it it triggers udev,
> > > > > > > > or restart the daemon.
> > > > > > >
> > > > > > > Thanks, that is exactly the sort of context to your suggested changes
> > > > > > > which I need.
> > > > > > >
> > > > > > > > > 2. Can't the userspace tools involved by modified to handle the errors
> > > > > > > > > they are getting gracefully?
> > > > > > > >
> > > > > > > > They can, but the approaches I see are not optimal:
> > > > > > > >     - Wait for HID events coming from the device, which could never
> > > > > > > > happen.
> > > > > > > >     - Poll the device until it wakes up.
> > > > > > >
> > > > > > > I guess we do get some (other or repeated?) event when the device does
> > > > > > > actually connect, otherwise your suggested changes would not be possible.
> > > > > >
> > > > > > No, I was thinking to just send the HID++ version identification
> > > > > > routine and see if the device replies.
> > > > >
> > > > > Hmm, to continue on these questions:
> > > > > - yes, the current approach is to have the users of the HID++ device
> > > > > try to contact the device, get an error from the receiver, then keep
> > > > > the hidraw node open until we get something out of it, and then we can
> > > > > start talking to it
> > > > > - to your question Hans, when a device connects, it emits a HID++
> > > > > notification, which we should be relaying in the hidraw node. If not,
> > > > > well then starting to receive a key or abs event on the input node is
> > > > > a pretty good hint that the device connected.
> > > > >
> > > > > So at any time, the kernel knows which devices are connected among
> > > > > those that are paired, so the kernel knows a lot more than user space.
> > > > >
> > > > > The main problem Filipe is facing here is that we specifically
> > > > > designed libratbag to *not* keep the device nodes opened, and to not
> > > > > poll on the input events. The reason being... we do not want libratbag
> > > > > to be considered as a keylogger.
> > > >
> > > > I'm wondering - can we really get around this long-term? Even if we have
> > > > a separate HID++ node and/or udev change events and/or some other
> > > > notification, in the end you still have some time T between that event
> > > > and userspace opening the actual event node. Where the first key event
> > > > wakes up the physical keyboard, you're now racing.
> > >
> > > Yes but it doesn't really matter in this case. We would only be
> > > potentially losing HID++ events, which are not that important, unlike
> > > normal input events. In fact, libratbag does not care about HID++
> > > events, they are just ignored.
> > >
> > > We would still have the same issue, yes, except here we don't really
> > > care.
> >
> > Well, I guess Peter's point is: "yes, you don't care *right now*, but
> > what if you care in the future, you will have the same race."
> >
> > > > So the separate HID++ node works as long as libratbag *only* listens to
> > > > that node, as soon as we need to start caring about a normal event it
> > > > won't work any longer.
> > >
> > > You mean when libratbag starts caring about normal input events? What
> > > is the point of that? Why would we need to do that? Also, as Benjamin
> > > pointed out, that would classify as a keylogger.
> >
> > For now, I think we are:
> > - to solve the immediate user-space problem, implement the udev events
> > as suggested by Hans. This is minimal code change
>
> Okay, great. So, this would be step 1, it would fix the immediate
> problem with no breakage.
>
> > - to solve the "keylogger" issue, we can split the HID devices in 2.
>
> This can come later. libratbag should be able to handle this change
> perfectly fine but we need to check with the other projects. If needed,
> I will test the change on the impacted projects and try submit the
> required patches to handle the new behavior properly to the upstreams.
>
> Just to make sure: we want to actually split the devices, not just add
> another node for only HID++ or something like that.

I *think* splitting would make more sense. I might be wrong though :)

>
> Quick question: is there any way for us to make userspace able to tell
> the nodes apart without having to get the rdesc via an ioctl? Perhaps
> exporting that information via sysfs?

Nope, not sysfs please. We can have a udev builtin that opens the
rdesc once and tags the device if this is necessary, but the less in
the kernel for that the better.

Cheers,
Benjamin