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

["Filipe Laíns" <lains@archlinux.org>]

[-- Attachment #1: Type: text/plain, Size: 5252 bytes --]

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.

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

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

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

Benjamin, what do you think?

> Regards,
> 
> Hans

Thank you,
Filipe Laíns

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]