| Message ID | 20200318161906.3340959-1-lains@archlinux.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: logitech-dj: issue udev change event on device connection | expand |
On Wed, Mar 18, 2020 at 11:19 AM Filipe Laíns <lains@archlinux.org> wrote: > > As discussed in the mailing list: > > > 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 solution reached to solve this issue is to trigger an udev change > event when the device connects, this way userspace can just wait on > those connections instead of trying to ping the device. > > Signed-off-by: Filipe Laíns <lains@archlinux.org> > --- > drivers/hid/hid-logitech-dj.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c > index 48dff5d6b605..fcd481a0be1f 100644 > --- a/drivers/hid/hid-logitech-dj.c > +++ b/drivers/hid/hid-logitech-dj.c > @@ -1464,6 +1464,8 @@ static int logi_dj_dj_event(struct hid_device *hdev, > if (dj_report->report_params[CONNECTION_STATUS_PARAM_STATUS] == > STATUS_LINKLOSS) { > logi_dj_recv_forward_null_report(djrcv_dev, dj_report); > + } else { > + kobject_uevent(&hdev->dev.kobj, KOBJ_CHANGE); > } > break; > default: > -- > 2.25.1 The problem that will remain here is the transition period for userspace to start to rely upon this. It will have no idea whether the kernel is expected to send events or not. What do you think about adding a syfs attribute to indicate that events are being sent? Or something similar?
Hi, On 3/18/20 6:15 PM, Mario Limonciello wrote: > On Wed, Mar 18, 2020 at 11:19 AM Filipe Laíns <lains@archlinux.org> wrote: >> >> As discussed in the mailing list: >> >>> 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 solution reached to solve this issue is to trigger an udev change >> event when the device connects, this way userspace can just wait on >> those connections instead of trying to ping the device. >> >> Signed-off-by: Filipe Laíns <lains@archlinux.org> >> --- >> drivers/hid/hid-logitech-dj.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c >> index 48dff5d6b605..fcd481a0be1f 100644 >> --- a/drivers/hid/hid-logitech-dj.c >> +++ b/drivers/hid/hid-logitech-dj.c >> @@ -1464,6 +1464,8 @@ static int logi_dj_dj_event(struct hid_device *hdev, >> if (dj_report->report_params[CONNECTION_STATUS_PARAM_STATUS] == >> STATUS_LINKLOSS) { >> logi_dj_recv_forward_null_report(djrcv_dev, dj_report); >> + } else { >> + kobject_uevent(&hdev->dev.kobj, KOBJ_CHANGE); >> } >> break; >> default: >> -- >> 2.25.1 > > The problem that will remain here is the transition period for > userspace to start to rely upon > this. It will have no idea whether the kernel is expected to send > events or not. What do you > think about adding a syfs attribute to indicate that events are being > sent? Or something similar? Then we would need to support that attribute forever. IMHO the best option is to just make a uname call and check the kernel version, with the code marked to be removed in the future when kernels older then $version are no longer something we want to support. Regards, Hans
On Wed, 2020-03-18 at 16:19 +0000, Filipe Laíns wrote: > As discussed in the mailing list: > > > 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 solution reached to solve this issue is to trigger an udev change > event when the device connects, this way userspace can just wait on > those connections instead of trying to ping the device. > > Signed-off-by: Filipe Laíns <lains@archlinux.org> > --- > drivers/hid/hid-logitech-dj.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid- > logitech-dj.c > index 48dff5d6b605..fcd481a0be1f 100644 > --- a/drivers/hid/hid-logitech-dj.c > +++ b/drivers/hid/hid-logitech-dj.c > @@ -1464,6 +1464,8 @@ static int logi_dj_dj_event(struct hid_device > *hdev, > if (dj_report- > >report_params[CONNECTION_STATUS_PARAM_STATUS] == > STATUS_LINKLOSS) { > logi_dj_recv_forward_null_report(djrcv_dev, > dj_report); > + } else { > + kobject_uevent(&hdev->dev.kobj, KOBJ_CHANGE); > } > break; > default: Just noticed I was issuing the udev event on the receiver instead of the connected device. I will send a v2. Filipe Laíns
On Wed, Mar 18, 2020 at 06:20:03PM +0100, Hans de Goede wrote: > Hi, > > On 3/18/20 6:15 PM, Mario Limonciello wrote: > > On Wed, Mar 18, 2020 at 11:19 AM Filipe Laíns <lains@archlinux.org> wrote: > > > > > > As discussed in the mailing list: > > > > > > > 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 solution reached to solve this issue is to trigger an udev change > > > event when the device connects, this way userspace can just wait on > > > those connections instead of trying to ping the device. > > > > > > Signed-off-by: Filipe Laíns <lains@archlinux.org> > > > --- > > > drivers/hid/hid-logitech-dj.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c > > > index 48dff5d6b605..fcd481a0be1f 100644 > > > --- a/drivers/hid/hid-logitech-dj.c > > > +++ b/drivers/hid/hid-logitech-dj.c > > > @@ -1464,6 +1464,8 @@ static int logi_dj_dj_event(struct hid_device *hdev, > > > if (dj_report->report_params[CONNECTION_STATUS_PARAM_STATUS] == > > > STATUS_LINKLOSS) { > > > logi_dj_recv_forward_null_report(djrcv_dev, dj_report); > > > + } else { > > > + kobject_uevent(&hdev->dev.kobj, KOBJ_CHANGE); > > > } > > > break; > > > default: > > > -- > > > 2.25.1 > > > > The problem that will remain here is the transition period for > > userspace to start to rely upon > > this. It will have no idea whether the kernel is expected to send > > events or not. What do you > > think about adding a syfs attribute to indicate that events are being > > sent? Or something similar? > > Then we would need to support that attribute forever. IMHO the best > option is to just make a uname call and check the kernel version, with > the code marked to be removed in the future when kernels older then > $version are no longer something we want to support. Also note that we may not have access to /sys. Cheers, Peter
On Thu, 19 Mar 2020, Peter Hutterer wrote: > > Then we would need to support that attribute forever. IMHO the best > > option is to just make a uname call and check the kernel version, with > > the code marked to be removed in the future when kernels older then > > $version are no longer something we want to support. Oh, this doesn't work *at all* with distro kernels backporting everything that passess by to kernels with major versions looking years old. I (as one of the "guilty ones" with my distro hat on) am not at all saying it's perfect, but that's the way it is.
On Fri, Mar 20, 2020 at 07:15:38PM -0500, Mario Limonciello wrote: > On Fri, Mar 20, 2020, 19:06 Jiri Kosina <jikos@kernel.org> wrote: > > > On Thu, 19 Mar 2020, Peter Hutterer wrote: > > > > > > Then we would need to support that attribute forever. IMHO the best > > > > option is to just make a uname call and check the kernel version, with > > > > the code marked to be removed in the future when kernels older then > > > > $version are no longer something we want to support. > > > > Oh, this doesn't work *at all* with distro kernels backporting everything > > that passess by to kernels with major versions looking years old. > > > > I (as one of the "guilty ones" with my distro hat on) am not at all saying > > it's perfect, but that's the way it is. > > > > -- > > Jiri Kosina > > SUSE Lab > > > > Another "solution" is to use module versioning bump as part of this patch. > At least when distros backport then you can look at module versioning to > tell the behavior of the driver. tbh, if there is no good solution in the kernel to communicate this, userspace can make do without knowing about it ahead of time. long-term you can just assume you'll get the change event and handle the error case just as you'd have to do now. Sure it'd be nice to know ahead of time but it's not the only thing we don't know until we get the first event. Cheers, Peter
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c index 48dff5d6b605..fcd481a0be1f 100644 --- a/drivers/hid/hid-logitech-dj.c +++ b/drivers/hid/hid-logitech-dj.c @@ -1464,6 +1464,8 @@ static int logi_dj_dj_event(struct hid_device *hdev, if (dj_report->report_params[CONNECTION_STATUS_PARAM_STATUS] == STATUS_LINKLOSS) { logi_dj_recv_forward_null_report(djrcv_dev, dj_report); + } else { + kobject_uevent(&hdev->dev.kobj, KOBJ_CHANGE); } break; default: