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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 2/13/20 4:07 PM, Benjamin Tissoires wrote:
> [almost forgot about this thread]
> 
> On Thu, Feb 6, 2020 at 8:02 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 2/6/20 7:43 PM, Filipe Laíns wrote:
>>> On Thu, 2020-02-06 at 18:45 +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 2/6/20 6:01 PM, 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.
>>>>
>>>> Ack.
>>>>
>>>>> 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.
>>>>
>>>> Ack.
>>>>
>>>>>>> So how about if we trigger a udev change event on the hid device instead
>>>>>>> when this happens ? That seems like a less invasive change on the kernel
>>>>>>> side and then libratbag could listen for these change events?
>>>>>>
>>>>>> Yes, that is a good idea :) I did not know this was possible but it
>>>>>> seems like a better approach.
>>>>>
>>>>> Not a big fan of that idea personally. This will add yet an other
>>>>> kernel API that we have to maintain.
>>>>> On Filipe's side, the hotplug support is something that has been
>>>>> around for quite a long time now, so we can safely expect applications
>>>>> to handle it properly.
>>>>
>>>> The suggested udev event change would just require a small change
>>>> to the existing hotplug handling, currently it responds to udev
>>>> "add" and "remove" events. With my suggested change in the "add"
>>>> path it will get an error because the device is not connected and
>>>> then stop adding the device. Combine this with treating "change"
>>>> events as "add" events and that is all that has to change on the
>>>> libratbag side.
>>>>
>>>> This assumes that duplicate add events are already filtered out,
>>>> which one has to do anyways to avoid coldplug enumeration vs
>>>> hotplug races.
>>>>
>>>> As for yet another kernel API to maintain, udev change events
>>>> already are an existing kernel API, what would be new is the hidpp
>>>> driver; and just the hidpp driver emitting them.
>>>>
>>>> All that is needed on the kernel side for this is to make the following
>>>> call when we detect a device moves from the paired to the connected state:
>>>>
>>>>       kobject_uevent(&hdev->dev.kobj, KOBJ_CHANGE);
>>>>
>>>> And there even seems to be a precedent for this, drivers/hid/hid-wiimote-core.c
>>>> already does this for what seems to be similar reasons.
> 
> Oooh, so yes, that would be an elegant way of solving this. We don't
> use KOBJ_CHANGE in the input stack, so there should be no harm in
> using it for that purpose.
> 
>>>>
>>>>
>>>>>>>>> 3. Is there a bugreport open about this somewhere?
>>>>>>>>
>>>>>>>> Yes, https://github.com/libratbag/libratbag/issues/785
>>>>>>>>
>>>>>>>>>> The reason we do this is because otherwise we would loose the first
>>>>>>>>>> packets when the device is turned on by key press. When a device is
>>>>>>>>>> turned on we would have to create the device node, and the packets
>>>>>>>>>> received while we are creating the device node would be lost.
>>>>>>>>>
>>>>>>>>> I don't believe that this is the reason, we only create hid child
>>>>>>>>> devices for devices reported by the receiver, but some of the non
>>>>>>>>> unifying hid receiver send a list of all devices paired, rather
>>>>>>>>> then the ones which are actually connected at that time.
>>>>>>>>
>>>>>>>> IIRC from my chats with Benjamin and Peter this is the reason, but
>>>>>>>> please correct me if I'm wrong.
>>>>>
>>>>> Filipe is correct here.
>>>>>
>>>>> For unifying devices, we can have up to 6 devices paired to a
>>>>> receiver, 3 can be used at the same time (connected).
>>>>> For the cheap receivers, we can enumerate 2 paired devices, but they
>>>>> are not necessarily connected too.
>>>>>
>>>>> Historically, when I first wrote the hid-logitech-hidpp driver, I
>>>>> wanted to not export a non connected device. But as mentioned by
>>>>> Filipe, this was posing issues mainly for keyboards, because generally
>>>>> the first thing you type on a keyboard is your password, and you don't
>>>>> necessarily have the feedback to see which keys you typed.
>>>>>
>>>>> So we (Nestor and I) decided to almost always create the input nodes
>>>>> when the device was not connected. The exceptions are when we need
>>>>> some device communication to set up the input node: so just for the
>>>>> touchpads.
>>>>
>>>> Ok.
>>>>
>>>>
>>>>>>> Could be that we can distinguish between "paired" and "connected"
>>>>>>> and that we are enumerating "paired" but not (yet) "connected"
>>>>>>> devices already because of what you say, I've not touched this
>>>>>>> code in a while.
>>>>>
>>>>> That is correct. Paired doesn't mean connected.
>>>>>
>>>>>> We create nodes for all paired devices, no matter if they are connected
>>>>>> or not.
>>>>>>
>>>>>>>>>> This could solved by buffering those packets, but that is a bad solution as
>>>>>>>>>> it would mess up the timings.
>>>>>>>>>>
>>>>>>>>>> At the moment the created node includes both normal HID and vendor
>>>>>>>>>> usages. To solve this problem, I propose that instead of creating a
>>>>>>>>>> single device node that contains all usages, we create one for normal
>>>>>>>>>> HID, which would exist all the time, and one for the vendor usage,
>>>>>>>>>> which would go away when the device disconnects. >
>>>>>>>>>> This slight behavior change will affect userspace. Two hidraw nodes
>>>>>>>>>> would be created instead of one. We need to make sure the current
>>>>>>>>>> userspace stacks interfacing with this would be able to properly handle
>>>>>>>>>> such changes.
>>>>>>>>>>
>>>>>>>>>> What do you think of this approach? Anyone has a better idea?
>>>>>>>>>
>>>>>>>>> The suggested approach sounds fragile and like it adds complexity to
>>>>>>>>> an already not simple driver.
>>>>>
>>>>> OTOH, this is what we have been trying to do in the kernel for years
>>>>> now: have one single node per application/usage, so we can rely on
>>>>> some valid data from the user space.
>>>>>
>>>>> I don't think the complexity of the driver should be a problem here.
>>>>> Yes, it's a complex one, but introducing a new API for that is a no
>>>>> from me.
>>>>
>>>> udev change events are not "adding a new API" there are a well known
>>>> API using e.g. for monitor plug/unplug in the drm subsys, etc. Yes
>>>> using them in the HID subsys this way is somewhat new.
> 
> OK. Would be using the KOBJ_CHANGE for "the device connected"
> something in line with what is done in the drm  subsystem?

Yes, the drm subsys uses this for example for monitor plug/unplug (later more
detailed separate udev events were added which also indicate which connector
changes, but the KOBJ_CHANGE event is still there for userspace which uses
it rather then the detailed events).

>>>>>>>> I understand, that is totally reasonable. I am working on a CI for the
>>>>>>>> driver if that helps.
>>>>>>>>
>>>>>>>>> It would be helpful to first describe the actual problem you are trying
>>>>>>>>> to fix (rather then suggesting a solution without clearly defining the
>>>>>>>>> problem) and then we can see from there.
>>>>>>>>
>>>>>>>> I though I described it good enough in the first paragraph but I guess
>>>>>>>> not, sorry. You should be able to get it from my comments above, if not
>>>>>>>> please let me know :)
>>>>>>>
>>>>>>> No problem, I have enough context now. I personally like my udev change
>>>>>>> event idea, which seems more KISS. But ultimately this is Benjamin's call.
>>>>>>
>>>>>> Yes, I don't know about the application details (I'll have to find out
>>>>>> :P) but it makes more sense to me. It avoids breaking the userspace
>>>>>> behavior.
>>>>>
>>>>> The udev change doesn't "break" userspace, but it is a new API. And
>>>>> that means nightmare from the application point of view:
>>>>> How do they know that the new API will be used? There is a high chance
>>>>> they won't, so for backward compatibility they will start listening to
>>>>> the hidraw node to match the current kernel behavior, and then we
>>>>> would just have added a new API for nothing.
>>>>
>>>> I agree that finding out if the udev change events are supported
>>>> is a bit of a challenge from userspace.
>>>>
>>>> But if I understood you correctly, then libratbag currently does
>>>> not keep listening to detect the connect, but rather atm this just
>>>> does not work, in which case it does not need to know if the new API
>>>> is there it can just assume; and even if it does need to know it
>>>> check the kernel-version number for that. Not pretty but that is
>>>> e.g. what libusb does to detect if certain "undetectable" features
>>>> are there, which admittedly is not ideal.
>>>
>>> Actually, since the latest release, libratbag would not require any
>>> changes. Please note that the proposal is to split the current hidraw
>>> node in two, one with just normal HID events, and one with just HID++.
>>> In 26c534cc742dfdbb14a889287f7771063be834cc (libratbag) we started
>>> parsing the report descriptors to find out the supported report IDs,
>>> the node with the normal HID events will fail on hidpp20_device_new
>>> because it won't support any HID++ reports.
>>>
>>>>>> Benjamin, what do you think?
>>>>>
>>>>> My point of view is:
>>>>> - don't add a new kernel API
>>>>
>>>> Again I believe calling udev change events "a new kernel API"
>>>> is exaggerating things a bit.
>>>>
>>>>> - rely on existing and supported user space behavior
>>>>> - ultimately, teach user space how to deal with the current situation
>>>>>
>>>>> So right now I think Filipe's proposal is the best bad solution. I
>>>>> would rank the udev event as worse than Filipe's solution because that
>>>>> involves both userspace and kernel space changes.
>>>>
>>>> The udev solution might require changes on both sides, but they
>>>> are very small easily reviewable changes. Anyways as I said this
>>>> is your call.
>>>>
>>>>> However, the proposal to add/remove the HID++ hidraw node on
>>>>> connect/disconnect really doesn't appeal to me because I am pretty
>>>>> sure we will have the same kind of issues that we are facing with
>>>>> keyboards. There might be an application that listens to the connect
>>>>> HID++ notification and turns the light on in the room whenever the
>>>>> mouse reconnects (and turns it off when the mouse disconnects because
>>>>> that means you left the room).
>>>>>
>>>>> So right now, as I am writing this, I think we should split the HID++
>>>>> node into its own hidraw node. This will allow application to listen
>>>>> to this node without being a keylogger as we will be filtering the key
>>>>> events in the actual input and the other hidraw nodes.
>>>
>>> Do we pass the HID connection notification to userspace? That is a
>>> receiver notification, and I though the driver was only passing the
>>> device packets.
>>>
>>> I don't understand why hotplugging is an issue? For me it's a feature.
>>> The userspace stack should definitely be able to handle it, that's how
>>> the corded devices work. By creating/removing the device node on device
>>> connect/disconnect we get the same behavior as when plugging/unplugging
>>> the mouse with a cable.
>>>
>>> Am I missing something here?
>>>
>>>> It took my a while to wrap my head around this, what you mean here
>>>> is that each paired device gets 2 nodes:
>>>>
>>>> 1) A full hidraw node which gets send all events from that paired device
>>>
>>> Like I said above, we want to split the node in two. One for the
>>> standard HID events (mouse movement, key press, etc.) and one just for
>>> HID++. From my understanding, this is also what Benjamin means.
>>
>> I see, so how would this work at the kernel level? AFAICT with the
>> current kernel code this would require logitech-dj to create 2 devices
>> under /sys/bus/hid/devices one for the HID++ descriptors + events
>> and one for the rest. But how is the drivers/hid/hid-logitech-hidpp.c
>> driver then supposed to work, AFAICT that needs access to both at
>> the same time.
> 
> Hmm, this is yet to be decided, yes. But hid-logitech-hidpp already
> deals with more than one hid device for a physical peripheral, because
> in the corded case, we might have 2 or 3 HID endpoints for one device.
> 
>>
>> Or is the plan to modify the hidraw driver for this somehow?
> 
> Nope, no changes in hidraw.

Ok.

>> I guess how this will work on the kernel side is mostly a question for Benjamin.
>>
>> I do see how this nicely solves the problem for userspace though,
>> it is a bit weird to have hidraw devices with fake descriptors and
>> which do not get all events, but we already have that with the
>> hidraw devices created for the devices behind the receiver, so I see
>> no harm in splitting these "fake" hidraw devices into 2 fake devices
>> each.
> 
> Yeah, the idea is to have a clean userspace implementation. OTOH, if
> the udev change is sufficient, we might never implement this split
> until the next need.

I think we should be able to make things work with just the KOBJ_CHANGE
event on the kernel side, but that will require some small changes to
the existing userspace code (e.g. to libratbag).

Regards,

Hans