Linux Input Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] HID: logitech-dj: issue udev change event on device connection
@ 2020-03-18 16:19 Filipe Laíns
  2020-03-18 17:15 ` Mario Limonciello
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Filipe Laíns @ 2020-03-18 16:19 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
	Peter Hutterer, Hans de Goede, Mario Limonciello, Richard Hughes
  Cc: Filipe Laíns

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: logitech-dj: issue udev change event on device connection
  2020-03-18 16:19 [PATCH] HID: logitech-dj: issue udev change event on device connection Filipe Laíns
@ 2020-03-18 17:15 ` Mario Limonciello
  2020-03-18 17:20   ` Hans de Goede
  2020-03-18 19:23 ` Filipe Laíns
  2020-03-18 19:27 ` [PATCH v2] " Filipe Laíns
  2 siblings, 1 reply; 13+ messages in thread
From: Mario Limonciello @ 2020-03-18 17:15 UTC (permalink / raw)
  To: Filipe Laíns
  Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
	Peter Hutterer, Hans de Goede, Richard Hughes

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?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: logitech-dj: issue udev change event on device connection
  2020-03-18 17:15 ` Mario Limonciello
@ 2020-03-18 17:20   ` Hans de Goede
  2020-03-19  2:23     ` Peter Hutterer
  0 siblings, 1 reply; 13+ messages in thread
From: Hans de Goede @ 2020-03-18 17:20 UTC (permalink / raw)
  To: Mario Limonciello, Filipe Laíns
  Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
	Peter Hutterer, Richard Hughes

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: logitech-dj: issue udev change event on device connection
  2020-03-18 16:19 [PATCH] HID: logitech-dj: issue udev change event on device connection Filipe Laíns
  2020-03-18 17:15 ` Mario Limonciello
@ 2020-03-18 19:23 ` Filipe Laíns
  2020-03-18 19:27 ` [PATCH v2] " Filipe Laíns
  2 siblings, 0 replies; 13+ messages in thread
From: Filipe Laíns @ 2020-03-18 19:23 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
	Peter Hutterer, Hans de Goede, Mario Limonciello, Richard Hughes

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

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

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2] HID: logitech-dj: issue udev change event on device connection
  2020-03-18 16:19 [PATCH] HID: logitech-dj: issue udev change event on device connection Filipe Laíns
  2020-03-18 17:15 ` Mario Limonciello
  2020-03-18 19:23 ` Filipe Laíns
@ 2020-03-18 19:27 ` " Filipe Laíns
  2020-03-24 10:20   ` Bastien Nocera
  2 siblings, 1 reply; 13+ messages in thread
From: Filipe Laíns @ 2020-03-18 19:27 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
	Peter Hutterer, Hans de Goede, Mario Limonciello, Richard Hughes
  Cc: Filipe Laíns

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>

---

v2:
  - Issue udev change event on the connected hid device, not on the
  receiver

---
 drivers/hid/hid-logitech-dj.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 48dff5d6b605..282e57dd467d 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -1412,6 +1412,7 @@ static int logi_dj_dj_event(struct hid_device *hdev,
 {
 	struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev);
 	struct dj_report *dj_report = (struct dj_report *) data;
+	struct dj_device *dj_dev;
 	unsigned long flags;
 
 	/*
@@ -1447,7 +1448,9 @@ static int logi_dj_dj_event(struct hid_device *hdev,
 
 	spin_lock_irqsave(&djrcv_dev->lock, flags);
 
-	if (!djrcv_dev->paired_dj_devices[dj_report->device_index]) {
+	dj_dev = djrcv_dev->paired_dj_devices[dj_report->device_index];
+
+	if (!dj_dev) {
 		/* received an event for an unknown device, bail out */
 		logi_dj_recv_queue_notification(djrcv_dev, dj_report);
 		goto out;
@@ -1464,6 +1467,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(&dj_dev->hdev->dev.kobj, KOBJ_CHANGE);
 		}
 		break;
 	default:
-- 
2.25.1

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: logitech-dj: issue udev change event on device connection
  2020-03-18 17:20   ` Hans de Goede
@ 2020-03-19  2:23     ` Peter Hutterer
  2020-03-21  0:05       ` Jiri Kosina
  0 siblings, 1 reply; 13+ messages in thread
From: Peter Hutterer @ 2020-03-19  2:23 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Mario Limonciello, Filipe Laíns, Jiri Kosina,
	Benjamin Tissoires, linux-input, linux-kernel, Peter Hutterer,
	Richard Hughes

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: logitech-dj: issue udev change event on device connection
  2020-03-19  2:23     ` Peter Hutterer
@ 2020-03-21  0:05       ` Jiri Kosina
       [not found]         ` <CA+EcB1P0qW4hdWG1YAYkD6X8jL1OaXZn4Lfu7aCmGBqwOPrJyA@mail.gmail.com>
  0 siblings, 1 reply; 13+ messages in thread
From: Jiri Kosina @ 2020-03-21  0:05 UTC (permalink / raw)
  To: Peter Hutterer
  Cc: Hans de Goede, Mario Limonciello, Filipe Laíns,
	Benjamin Tissoires, linux-input, linux-kernel, Peter Hutterer,
	Richard Hughes

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 Labs


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: logitech-dj: issue udev change event on device connection
       [not found]         ` <CA+EcB1P0qW4hdWG1YAYkD6X8jL1OaXZn4Lfu7aCmGBqwOPrJyA@mail.gmail.com>
@ 2020-03-21  0:29           ` Peter Hutterer
  0 siblings, 0 replies; 13+ messages in thread
From: Peter Hutterer @ 2020-03-21  0:29 UTC (permalink / raw)
  To: Mario Limonciello
  Cc: Jiri Kosina, Hans de Goede, Filipe Laíns,
	Benjamin Tissoires, linux-input, linux-kernel, Peter Hutterer,
	Richard Hughes

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] HID: logitech-dj: issue udev change event on device connection
  2020-03-18 19:27 ` [PATCH v2] " Filipe Laíns
@ 2020-03-24 10:20   ` Bastien Nocera
  2020-03-24 13:46     ` Filipe Laíns
  0 siblings, 1 reply; 13+ messages in thread
From: Bastien Nocera @ 2020-03-24 10:20 UTC (permalink / raw)
  To: Filipe Laíns, Jiri Kosina, Benjamin Tissoires, linux-input,
	linux-kernel, Peter Hutterer, Hans de Goede, Mario Limonciello,
	Richard Hughes

On Wed, 2020-03-18 at 19:27 +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.

Why is it a problem that user-space communication fails? Note that
sending a signal without any way to fetch the state means that it's
always going to be racy.

> 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>
> 
> ---
> 
> v2:
>   - Issue udev change event on the connected hid device, not on the
>   receiver
> 
> ---
>  drivers/hid/hid-logitech-dj.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-
> logitech-dj.c
> index 48dff5d6b605..282e57dd467d 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -1412,6 +1412,7 @@ static int logi_dj_dj_event(struct hid_device
> *hdev,
>  {
>  	struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev);
>  	struct dj_report *dj_report = (struct dj_report *) data;
> +	struct dj_device *dj_dev;
>  	unsigned long flags;
>  
>  	/*
> @@ -1447,7 +1448,9 @@ static int logi_dj_dj_event(struct hid_device
> *hdev,
>  
>  	spin_lock_irqsave(&djrcv_dev->lock, flags);
>  
> -	if (!djrcv_dev->paired_dj_devices[dj_report->device_index]) {
> +	dj_dev = djrcv_dev->paired_dj_devices[dj_report->device_index];
> +
> +	if (!dj_dev) {
>  		/* received an event for an unknown device, bail out */
>  		logi_dj_recv_queue_notification(djrcv_dev, dj_report);
>  		goto out;
> @@ -1464,6 +1467,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(&dj_dev->hdev->dev.kobj,
> KOBJ_CHANGE);
>  		}
>  		break;
>  	default:


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] HID: logitech-dj: issue udev change event on device connection
  2020-03-24 10:20   ` Bastien Nocera
@ 2020-03-24 13:46     ` Filipe Laíns
  2020-03-24 14:03       ` Bastien Nocera
  0 siblings, 1 reply; 13+ messages in thread
From: Filipe Laíns @ 2020-03-24 13:46 UTC (permalink / raw)
  To: Bastien Nocera, Jiri Kosina, Benjamin Tissoires, linux-input,
	linux-kernel, Peter Hutterer, Hans de Goede, Mario Limonciello,
	Richard Hughes

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

On Tue, 2020-03-24 at 11:20 +0100, Bastien Nocera wrote:
> On Wed, 2020-03-18 at 19:27 +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.
> 
> Why is it a problem that user-space communication fails? Note that
> sending a signal without any way to fetch the state means that it's
> always going to be racy.

It failing is not the problem. The problem is knowing when the device
is available again. Right now the only way to do that is to listen for
events or periodically ping it.

We want to only export the HID++ hidraw node when the device is
available but that will take a while. We will have to test and sync up
userspace. I also want to write tests for the driver before, to make
sure there are no regressions. We had a thread discussing this, IIRC
you were in CC.

> > 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>
> > 
> > ---
> > 
> > v2:
> >   - Issue udev change event on the connected hid device, not on the
> >   receiver
> > 
> > ---
> >  drivers/hid/hid-logitech-dj.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-
> > logitech-dj.c
> > index 48dff5d6b605..282e57dd467d 100644
> > --- a/drivers/hid/hid-logitech-dj.c
> > +++ b/drivers/hid/hid-logitech-dj.c
> > @@ -1412,6 +1412,7 @@ static int logi_dj_dj_event(struct hid_device
> > *hdev,
> >  {
> >  	struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev);
> >  	struct dj_report *dj_report = (struct dj_report *) data;
> > +	struct dj_device *dj_dev;
> >  	unsigned long flags;
> >  
> >  	/*
> > @@ -1447,7 +1448,9 @@ static int logi_dj_dj_event(struct hid_device
> > *hdev,
> >  
> >  	spin_lock_irqsave(&djrcv_dev->lock, flags);
> >  
> > -	if (!djrcv_dev->paired_dj_devices[dj_report->device_index]) {
> > +	dj_dev = djrcv_dev->paired_dj_devices[dj_report->device_index];
> > +
> > +	if (!dj_dev) {
> >  		/* received an event for an unknown device, bail out */
> >  		logi_dj_recv_queue_notification(djrcv_dev, dj_report);
> >  		goto out;
> > @@ -1464,6 +1467,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(&dj_dev->hdev->dev.kobj,
> > KOBJ_CHANGE);
> >  		}
> >  		break;
> >  	default:
-- 
Filipe Laíns

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] HID: logitech-dj: issue udev change event on device connection
  2020-03-24 13:46     ` Filipe Laíns
@ 2020-03-24 14:03       ` Bastien Nocera
  2020-03-24 14:10         ` Filipe Laíns
  0 siblings, 1 reply; 13+ messages in thread
From: Bastien Nocera @ 2020-03-24 14:03 UTC (permalink / raw)
  To: Filipe Laíns, Jiri Kosina, Benjamin Tissoires, linux-input,
	linux-kernel, Peter Hutterer, Hans de Goede, Mario Limonciello,
	Richard Hughes

On Tue, 2020-03-24 at 13:46 +0000, Filipe Laíns wrote:
> On Tue, 2020-03-24 at 11:20 +0100, Bastien Nocera wrote:
> > On Wed, 2020-03-18 at 19:27 +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.
> > 
> > Why is it a problem that user-space communication fails? Note that
> > sending a signal without any way to fetch the state means that it's
> > always going to be racy.
> 
> It failing is not the problem. The problem is knowing when the device
> is available again. Right now the only way to do that is to listen
> for
> events or periodically ping it.
> 
> We want to only export the HID++ hidraw node when the device is
> available but that will take a while. We will have to test and sync
> up
> userspace. I also want to write tests for the driver before, to make
> sure there are no regressions. We had a thread discussing this, IIRC
> you were in CC.

If I need to remember some old thread to know what we're talking about,
then that means that the commit message is probably not good enough...

Please add some links to the relevant discussion on bug forums if
there's interesting data there too.


> 
> > > 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>
> > > 
> > > ---
> > > 
> > > v2:
> > >   - Issue udev change event on the connected hid device, not on
> > > the
> > >   receiver
> > > 
> > > ---
> > >  drivers/hid/hid-logitech-dj.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-
> > > logitech-dj.c
> > > index 48dff5d6b605..282e57dd467d 100644
> > > --- a/drivers/hid/hid-logitech-dj.c
> > > +++ b/drivers/hid/hid-logitech-dj.c
> > > @@ -1412,6 +1412,7 @@ static int logi_dj_dj_event(struct
> > > hid_device
> > > *hdev,
> > >  {
> > >  	struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev);
> > >  	struct dj_report *dj_report = (struct dj_report *) data;
> > > +	struct dj_device *dj_dev;
> > >  	unsigned long flags;
> > >  
> > >  	/*
> > > @@ -1447,7 +1448,9 @@ static int logi_dj_dj_event(struct
> > > hid_device
> > > *hdev,
> > >  
> > >  	spin_lock_irqsave(&djrcv_dev->lock, flags);
> > >  
> > > -	if (!djrcv_dev->paired_dj_devices[dj_report->device_index]) {
> > > +	dj_dev = djrcv_dev->paired_dj_devices[dj_report->device_index];
> > > +
> > > +	if (!dj_dev) {
> > >  		/* received an event for an unknown device, bail out */
> > >  		logi_dj_recv_queue_notification(djrcv_dev, dj_report);
> > >  		goto out;
> > > @@ -1464,6 +1467,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(&dj_dev->hdev->dev.kobj,
> > > KOBJ_CHANGE);
> > >  		}
> > >  		break;
> > >  	default:


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] HID: logitech-dj: issue udev change event on device connection
  2020-03-24 14:03       ` Bastien Nocera
@ 2020-03-24 14:10         ` Filipe Laíns
  2020-03-24 14:24           ` Bastien Nocera
  0 siblings, 1 reply; 13+ messages in thread
From: Filipe Laíns @ 2020-03-24 14:10 UTC (permalink / raw)
  To: Bastien Nocera, Jiri Kosina, Benjamin Tissoires, linux-input,
	linux-kernel, Peter Hutterer, Hans de Goede, Mario Limonciello,
	Richard Hughes

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

On Tue, 2020-03-24 at 15:03 +0100, Bastien Nocera wrote:
> On Tue, 2020-03-24 at 13:46 +0000, Filipe Laíns wrote:
> > On Tue, 2020-03-24 at 11:20 +0100, Bastien Nocera wrote:
> > > On Wed, 2020-03-18 at 19:27 +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.
> > > 
> > > Why is it a problem that user-space communication fails? Note that
> > > sending a signal without any way to fetch the state means that it's
> > > always going to be racy.
> > 
> > It failing is not the problem. The problem is knowing when the device
> > is available again. Right now the only way to do that is to listen
> > for
> > events or periodically ping it.
> > 
> > We want to only export the HID++ hidraw node when the device is
> > available but that will take a while. We will have to test and sync
> > up
> > userspace. I also want to write tests for the driver before, to make
> > sure there are no regressions. We had a thread discussing this, IIRC
> > you were in CC.
> 
> If I need to remember some old thread to know what we're talking about,
> then that means that the commit message is probably not good enough...

Should I put in the commit message what is planned next?

> Please add some links to the relevant discussion on bug forums if
> there's interesting data there too.

You can find the discussion in [1][2].

[1] https://www.spinics.net/lists/linux-input/thrd2.html#65615
[2] https://www.spinics.net/lists/linux-input/msg65615.html

> > > > 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>
> > > > 
> > > > ---
> > > > 
> > > > v2:
> > > >   - Issue udev change event on the connected hid device, not on
> > > > the
> > > >   receiver
> > > > 
> > > > ---
> > > >  drivers/hid/hid-logitech-dj.c | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-
> > > > logitech-dj.c
> > > > index 48dff5d6b605..282e57dd467d 100644
> > > > --- a/drivers/hid/hid-logitech-dj.c
> > > > +++ b/drivers/hid/hid-logitech-dj.c
> > > > @@ -1412,6 +1412,7 @@ static int logi_dj_dj_event(struct
> > > > hid_device
> > > > *hdev,
> > > >  {
> > > >  	struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev);
> > > >  	struct dj_report *dj_report = (struct dj_report *) data;
> > > > +	struct dj_device *dj_dev;
> > > >  	unsigned long flags;
> > > >  
> > > >  	/*
> > > > @@ -1447,7 +1448,9 @@ static int logi_dj_dj_event(struct
> > > > hid_device
> > > > *hdev,
> > > >  
> > > >  	spin_lock_irqsave(&djrcv_dev->lock, flags);
> > > >  
> > > > -	if (!djrcv_dev->paired_dj_devices[dj_report->device_index]) {
> > > > +	dj_dev = djrcv_dev->paired_dj_devices[dj_report->device_index];
> > > > +
> > > > +	if (!dj_dev) {
> > > >  		/* received an event for an unknown device, bail out */
> > > >  		logi_dj_recv_queue_notification(djrcv_dev, dj_report);
> > > >  		goto out;
> > > > @@ -1464,6 +1467,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(&dj_dev->hdev->dev.kobj,
> > > > KOBJ_CHANGE);
> > > >  		}
> > > >  		break;
> > > >  	default:
-- 
Filipe Laíns

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] HID: logitech-dj: issue udev change event on device connection
  2020-03-24 14:10         ` Filipe Laíns
@ 2020-03-24 14:24           ` Bastien Nocera
  0 siblings, 0 replies; 13+ messages in thread
From: Bastien Nocera @ 2020-03-24 14:24 UTC (permalink / raw)
  To: Filipe Laíns, Jiri Kosina, Benjamin Tissoires, linux-input,
	linux-kernel, Peter Hutterer, Hans de Goede, Mario Limonciello,
	Richard Hughes

On Tue, 2020-03-24 at 14:10 +0000, Filipe Laíns wrote:
> On Tue, 2020-03-24 at 15:03 +0100, Bastien Nocera wrote:
> > On Tue, 2020-03-24 at 13:46 +0000, Filipe Laíns wrote:
> > > On Tue, 2020-03-24 at 11:20 +0100, Bastien Nocera wrote:
> > > > On Wed, 2020-03-18 at 19:27 +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.
> > > > 
> > > > Why is it a problem that user-space communication fails? Note
> > > > that
> > > > sending a signal without any way to fetch the state means that
> > > > it's
> > > > always going to be racy.
> > > 
> > > It failing is not the problem. The problem is knowing when the
> > > device
> > > is available again. Right now the only way to do that is to
> > > listen
> > > for
> > > events or periodically ping it.
> > > 
> > > We want to only export the HID++ hidraw node when the device is
> > > available but that will take a while. We will have to test and
> > > sync
> > > up
> > > userspace. I also want to write tests for the driver before, to
> > > make
> > > sure there are no regressions. We had a thread discussing this,
> > > IIRC
> > > you were in CC.
> > 
> > If I need to remember some old thread to know what we're talking
> > about,
> > then that means that the commit message is probably not good
> > enough...
> 
> Should I put in the commit message what is planned next?

You should put in everything that explains why this change is needed,
and how user-space is supposed to use that information. Nobody should
have to refer back to the mailing-list thread to find this out.

> > Please add some links to the relevant discussion on bug forums if
> > there's interesting data there too.
> 
> You can find the discussion in [1][2].
> 
> [1] https://www.spinics.net/lists/linux-input/thrd2.html#65615
> [2] https://www.spinics.net/lists/linux-input/msg65615.html

I was in CC: because "might be maintaining one of the affected
userspace stacks", but I don't know what that stack might be for me. Is
it for upower (which I only work on seldom and definitely don't
maintain), or something else?

> > > > > 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>
> > > > > 
> > > > > ---
> > > > > 
> > > > > v2:
> > > > >   - Issue udev change event on the connected hid device, not
> > > > > on
> > > > > the
> > > > >   receiver
> > > > > 
> > > > > ---
> > > > >  drivers/hid/hid-logitech-dj.c | 7 ++++++-
> > > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-
> > > > > logitech-dj.c
> > > > > index 48dff5d6b605..282e57dd467d 100644
> > > > > --- a/drivers/hid/hid-logitech-dj.c
> > > > > +++ b/drivers/hid/hid-logitech-dj.c
> > > > > @@ -1412,6 +1412,7 @@ static int logi_dj_dj_event(struct
> > > > > hid_device
> > > > > *hdev,
> > > > >  {
> > > > >  	struct dj_receiver_dev *djrcv_dev =
> > > > > hid_get_drvdata(hdev);
> > > > >  	struct dj_report *dj_report = (struct dj_report *)
> > > > > data;
> > > > > +	struct dj_device *dj_dev;
> > > > >  	unsigned long flags;
> > > > >  
> > > > >  	/*
> > > > > @@ -1447,7 +1448,9 @@ static int logi_dj_dj_event(struct
> > > > > hid_device
> > > > > *hdev,
> > > > >  
> > > > >  	spin_lock_irqsave(&djrcv_dev->lock, flags);
> > > > >  
> > > > > -	if (!djrcv_dev->paired_dj_devices[dj_report-
> > > > > >device_index]) {
> > > > > +	dj_dev = djrcv_dev->paired_dj_devices[dj_report-
> > > > > >device_index];
> > > > > +
> > > > > +	if (!dj_dev) {
> > > > >  		/* received an event for an unknown device,
> > > > > bail out */
> > > > >  		logi_dj_recv_queue_notification(djrcv_dev,
> > > > > dj_report);
> > > > >  		goto out;
> > > > > @@ -1464,6 +1467,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(&dj_dev->hdev->dev.kobj,
> > > > > KOBJ_CHANGE);
> > > > >  		}
> > > > >  		break;
> > > > >  	default:


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-18 16:19 [PATCH] HID: logitech-dj: issue udev change event on device connection Filipe Laíns
2020-03-18 17:15 ` Mario Limonciello
2020-03-18 17:20   ` Hans de Goede
2020-03-19  2:23     ` Peter Hutterer
2020-03-21  0:05       ` Jiri Kosina
     [not found]         ` <CA+EcB1P0qW4hdWG1YAYkD6X8jL1OaXZn4Lfu7aCmGBqwOPrJyA@mail.gmail.com>
2020-03-21  0:29           ` Peter Hutterer
2020-03-18 19:23 ` Filipe Laíns
2020-03-18 19:27 ` [PATCH v2] " Filipe Laíns
2020-03-24 10:20   ` Bastien Nocera
2020-03-24 13:46     ` Filipe Laíns
2020-03-24 14:03       ` Bastien Nocera
2020-03-24 14:10         ` Filipe Laíns
2020-03-24 14:24           ` Bastien Nocera

Linux Input Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-input/0 linux-input/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-input linux-input/ https://lore.kernel.org/linux-input \
		linux-input@vger.kernel.org
	public-inbox-index linux-input

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-input


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git