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

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

[Filipe Laíns]

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

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.

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

If you are CCed you might be maintaining one of the affected userspace
stacks, if so, please let me know your project is able to handle this.
If not, I can help you adapt to the new behavior.

Thank you,
Filipe Laíns

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

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

[Hans de Goede]

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?
2. Can't the userspace tools involved by modified to handle the errors
they are getting gracefully?
3. Is there a bugreport open about this somewhere?

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

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

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.

Regards,

Hans

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

[Filipe Laíns]

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

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.

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

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

> > 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 :)

> Regards,
> 
> Hans

Cheers, 
Filipe Laíns

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

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

[Hans de Goede]

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.

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?


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

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

Regards,

Hans

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

[Filipe Laíns]

[-- 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 --]

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

[Benjamin Tissoires]

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.

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

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

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

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

>
> Benjamin, what do you think?

My point of view is:
- don't add a new kernel API
- 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.

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.

But then, applications will have to learn how to listen to the HID++
node, especially given that they are talking HID++ in the first place.

Cheers,
Benjamin

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

[Hans de Goede]

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.


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

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

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

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
2) A special HID++ node which only gets forwarded HID++ events

And that second node is only present when the device is connected?

Oh, I guess not, I guess you want them both to always be present, but
now apps can safely keep the HID++ one open because that would not
make the app a keylogger.

> But then, applications will have to learn how to listen to the HID++
> node, especially given that they are talking HID++ in the first place.

So this:
1. Would still require changes to both the kernel and userspace side
2. Would require some magic way for the app to detect which hidraw node is which
3. Would require some magic way for the app to detect that the kernel is new
    enough to support the new spearate HID++ node to avoid waiting for it to show
    up indefinitely
4. Would use existing kernel API in a new way in the form of a special type of
    hidraw node which we did not have before.

IOW AFAICT this has all the disadvantages of my udev change proposal.

I guess the etxra hidraw node proposal does have the advantage that it would give
us a fd which is safe(r) (*) to pass around for doing HID++ things and with some
filtering we might even make it completely safe, that would be a good reason to
go that route.

*) safer, not entirely safe as HID++ can send at least the media keys through HID++
instead of the normal input reports, see e.g. the CONSUMER_VENDOR_KEYS handling
in hid-logitech-hidpp.c and if IIRC it might even be possible to request the
normal keys to be send through HID++.

Regards,

Hans

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

[Filipe Laíns]

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

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

> 2) A special HID++ node which only gets forwarded HID++ events
> 
> And that second node is only present when the device is connected?
> 
> Oh, I guess not, I guess you want them both to always be present, but
> now apps can safely keep the HID++ one open because that would not
> make the app a keylogger.
> 
> > But then, applications will have to learn how to listen to the HID++
> > node, especially given that they are talking HID++ in the first place.
> 
> So this:
> 1. Would still require changes to both the kernel and userspace side
> 2. Would require some magic way for the app to detect which hidraw node is which
> 3. Would require some magic way for the app to detect that the kernel is new
>     enough to support the new spearate HID++ node to avoid waiting for it to show
>     up indefinitely
> 4. Would use existing kernel API in a new way in the form of a special type of
>     hidraw node which we did not have before.
> 
> IOW AFAICT this has all the disadvantages of my udev change proposal.
> 
> I guess the etxra hidraw node proposal does have the advantage that it would give
> us a fd which is safe(r) (*) to pass around for doing HID++ things and with some
> filtering we might even make it completely safe, that would be a good reason to
> go that route.
> 
> *) safer, not entirely safe as HID++ can send at least the media keys through HID++
> instead of the normal input reports, see e.g. the CONSUMER_VENDOR_KEYS handling
> in hid-logitech-hidpp.c and if IIRC it might even be possible to request the
> normal keys to be send through HID++.

Thank you,
Filipe Laíns

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

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

[Hans de Goede]

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

Or is the plan to modify the hidraw driver for this somehow?

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.

If someone wants to e.g. capture the full stream for debugging they
can use the hidraw of the receiver for that. So I guess that this is
an elegant solution to the problem.

Regards,

Hans

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

[Filipe Laíns]

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

On Thu, 2020-02-06 at 20:02 +0100, Hans de Goede 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.
> > > 
> > > 
> > > > > > > > 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.
> > > 
> > > > > > > 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.
> 
> Or is the plan to modify the hidraw driver for this somehow?

I was not really thinking about that. I guess Benjamin could clarify on
how to proceed if we decide to do this.

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

Yes, it's nothing far from what we are already doing. I like that both
wired and wireless devices would behave exactly the same.

> If someone wants to e.g. capture the full stream for debugging they
> can use the hidraw of the receiver for that. So I guess that this is
> an elegant solution to the problem.

Yes, the receiver node is always available if someone needs it.
Actually, some userspace apps use it instead of the hidraw nodes
exported by this driver.

Thank you,
Filipe Laíns

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

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

[Peter Hutterer]

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.

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.

Cheers,
   Peter

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

[Filipe Laíns]

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

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.

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

Cheers,
Filipe Laíns 

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

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

[Benjamin Tissoires]

[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?

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

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

Cheers,
Benjamin

>
> If someone wants to e.g. capture the full stream for debugging they
> can use the hidraw of the receiver for that. So I guess that this is
> an elegant solution to the problem.
>
> Regards,
>
> Hans
>

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

[Benjamin Tissoires]

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
- to solve the "keylogger" issue, we can split the HID devices in 2.

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

>
> Cheers,
> Filipe Laíns

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

[Filipe Laíns]

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

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.

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
> 
> > Cheers,
> > Filipe Laíns

Thanks,
Filipe Laíns

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

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

[Hans de Goede]

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

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

[Benjamin Tissoires]

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