On Thu, Feb 13, 2020 at 4:25 PM Filipe Laíns <lains@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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
