On Thu, 2020-02-13 at 16:12 +0100, Benjamin Tissoires wrote:

On Fri, Feb 7, 2020 at 1:37 AM Filipe Laíns [off-list ref] wrote:

quoted

On Fri, 2020-02-07 at 10:03 +1000, Peter Hutterer wrote:

quoted

On 7/2/20 3:01 am, Benjamin Tissoires wrote:

quoted

On Thu, Feb 6, 2020 at 4:42 PM Filipe Laíns [off-list ref] wrote:

quoted

On Thu, 2020-02-06 at 13:13 +0100, Hans de Goede wrote:

quoted

Hi,

On 2/6/20 12:51 PM, Filipe Laíns wrote:

quoted

On Thu, 2020-02-06 at 12:30 +0100, Hans de Goede wrote:

quoted

HI,

On 2/6/20 12:14 PM, Filipe Laíns wrote:

quoted

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.

quoted

quoted

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."

quoted

quoted

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.

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?

This way, we do not have to deal with hotplug races, but can still get
information from the connect event without reading the input events.

Cheers,
Benjamin

quoted

Cheers,
Filipe Laíns

Thanks,
Filipe Laíns